Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: change skaffold base image #8433

Merged

Conversation

ericzzzzzzz
Copy link
Contributor

@ericzzzzzzz ericzzzzzzz commented Feb 13, 2023

related: #8418
Fixes: #nnn
Related: Relevant tracking issues, for context
Merge before/after: Dependent or prerequisite PRs

Test Plan

  • build dependencies image docker build \ -f deploy/skaffold/Dockerfile.deps \ -t abc/build_deps:5b53a3d6c20eadaf870bb6f388c95107bbd731e8 \ .
  • change deploy/skaffold/Dockerfile base image to be abc/build_deps:5b53a3d6c20eadaf870bb6f388c95107bbd731e8
  • run cp deploy/skaffold/Dockerfile . to copy dockerfile to working directory
  • build a skaffold builder image docker build -t abc/builder:1234 --target builder .
  • run docker run --rm -v /var/run/docker.sock:/var/run/docker.sock abc/builder:1234 -e DOCKER_CONFIG=/root/.docker \ make integration-tests to run tests with the new image, we can exit container after seeing a few success.
  • This will get tested when we merge this pr, then the all the integration test will use the new image``

@aaron-prindle
Copy link
Contributor

aaron-prindle commented Feb 14, 2023

Can you add some additional info as to how you manually verified the skaffold image still worked and had similar/identical functionality as it did prior?

@ericzzzzzzz ericzzzzzzz marked this pull request as draft February 14, 2023 05:10
@ericzzzzzzz
Copy link
Contributor Author

Can you add some additional info as to how you manually verified the skaffold image still worked and had similar/identical functionality as it did prior?

My bad! Was thinking this was tested somewhere in our ci, turns out not. Thank you for calling this out! I did some testing and then found some issues, added changes to fix those.

@codecov
Copy link

codecov bot commented Feb 14, 2023

Codecov Report

Merging #8433 (7236fd1) into main (290280e) will decrease coverage by 5.28%.
The diff coverage is 55.02%.

@@            Coverage Diff             @@
##             main    #8433      +/-   ##
==========================================
- Coverage   70.48%   65.21%   -5.28%     
==========================================
  Files         515      603      +88     
  Lines       23150    29898    +6748     
==========================================
+ Hits        16317    19497    +3180     
- Misses       5776     8926    +3150     
- Partials     1057     1475     +418     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/completion.go 13.04% <0.00%> (-1.25%) ⬇️
cmd/skaffold/app/cmd/config/list.go 65.21% <ø> (ø)
cmd/skaffold/app/cmd/config/set.go 88.72% <ø> (ø)
cmd/skaffold/app/cmd/config/util.go 54.28% <ø> (ø)
cmd/skaffold/app/cmd/credits.go 100.00% <ø> (ø)
cmd/skaffold/app/cmd/credits/export.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/deploy.go 40.90% <0.00%> (-12.94%) ⬇️
cmd/skaffold/app/cmd/generate_pipeline.go 60.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_modules.go 65.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_profiles.go 66.66% <ø> (ø)
... and 426 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ericzzzzzzz ericzzzzzzz marked this pull request as ready for review February 14, 2023 13:37
@@ -151,7 +151,8 @@ RUN gcloud auth configure-docker && gcloud components install --quiet \
log-streaming

FROM runtime_deps
RUN apt-get update && apt-get install --no-install-recommends --no-install-suggests -y \
DEBIAN_FRONTEND=noninteractive
Copy link
Contributor Author

@ericzzzzzzz ericzzzzzzz Feb 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some package requires interaction after installation, set up this to skip it

@@ -151,7 +151,8 @@ RUN gcloud auth configure-docker && gcloud components install --quiet \
log-streaming

FROM runtime_deps
RUN apt-get update && apt-get install --no-install-recommends --no-install-suggests -y \
ENV DEBIAN_FRONTEND=noninteractive
RUN apt-get update && apt-get dist-upgrade -y && apt-get install --no-install-recommends --no-install-suggests -y \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add curl to the packages installed here? It seems this tool is not installed in Debian Bullseye but is in Ubuntu

This likely needs to be done in all of the Dockerfile.deps* files

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this got partially reverted in #8460 it looks like this lost curl as we went from gcr.io/gcp-runtimes/ubuntu_20_0_4 which included curl to ubuntu:20.04 which doesn't.

Can curl be added back into the released Skaffold images please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomelliff thanks for flagging, this. I've created this issue #8668 tracking this and added it to Skaffold's next release -v2.4.0 which is targeting release for 4/17

Copy link
Contributor

@aaron-prindle aaron-prindle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@aaron-prindle aaron-prindle merged commit 6b3f673 into GoogleContainerTools:main Feb 16, 2023
ericzzzzzzz added a commit to ericzzzzzzz/skaffold that referenced this pull request Feb 22, 2023
aaron-prindle pushed a commit that referenced this pull request Feb 23, 2023
* chore: Revert "chore: change skaffold base image (#8433)"

This reverts commit 6b3f673.

* chore: update skaffold base image to ubuntu:20.04

* chore: add noninteractive
aaron-prindle pushed a commit to aaron-prindle/skaffold that referenced this pull request Mar 1, 2023
* chore: Revert "chore: change skaffold base image (GoogleContainerTools#8433)"

This reverts commit 6b3f673.

* chore: update skaffold base image to ubuntu:20.04

* chore: add noninteractive

(cherry picked from commit 9847653)
aaron-prindle added a commit that referenced this pull request Mar 1, 2023
* chore: upgrade go in dockerfile (#8420)

(cherry picked from commit 6289a4b)

* chore: Update skaffold base image (#8460)

* chore: Revert "chore: change skaffold base image (#8433)"

This reverts commit 6b3f673.

* chore: update skaffold base image to ubuntu:20.04

* chore: add noninteractive

(cherry picked from commit 9847653)

---------

Co-authored-by: ericzzzzzzz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants