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

fix: default to builder supported platform with a warning instead of erroring #7659

Merged

Conversation

gsquared94
Copy link
Collaborator

Using Skaffold with the buildpacks builder on an M1 Mac against the minikube cluster, there are two scenarios:

  1. User specifies the platform explicitly using --platform=linux/amd64.
    This was previously erroring out, due to Skaffold disallowing builds for a platform that doesn't match the active kubernetes cluster nodes. However, local clusters like minikube and docker-desktop on Apple M1 can run linux/amd64 images using qemu binfmt_misc extensions. So we changed this error condition to a warning message instead in fix: change error to warning for build platform mismatch with cluster node platform #7402.

  2. Users don't specify platform explicitly
    In this case Skaffold selects the platform linux/arm64 based on the active kubernetes cluster node platform type. However, since buildpacks doesn't support building for this platform the build fails. We instead change this to a warning message and select the builder supported platforms instead. This is fixed in this PR.

@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #7659 (4a6c230) into main (290280e) will decrease coverage by 3.84%.
The diff coverage is 53.95%.

@@            Coverage Diff             @@
##             main    #7659      +/-   ##
==========================================
- Coverage   70.48%   66.63%   -3.85%     
==========================================
  Files         515      585      +70     
  Lines       23150    28080    +4930     
==========================================
+ Hits        16317    18711    +2394     
- Misses       5776     8008    +2232     
- Partials     1057     1361     +304     
Impacted Files Coverage Δ
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/test.go 44.44% <0.00%> (ø)
cmd/skaffold/skaffold.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/inspect_tests.go 62.50% <14.28%> (-1.14%) ⬇️
cmd/skaffold/app/cmd/render.go 35.48% <18.18%> (-5.90%) ⬇️
cmd/skaffold/app/cmd/lsp.go 28.12% <28.12%> (ø)
cmd/skaffold/app/cmd/run.go 64.28% <33.33%> (-9.63%) ⬇️
cmd/skaffold/app/cmd/fix.go 55.93% <38.88%> (-20.54%) ⬇️
cmd/skaffold/app/cmd/verify.go 41.17% <41.17%> (ø)
... and 329 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@aaron-prindle aaron-prindle self-requested a review July 20, 2022 17:58
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!

@gsquared94 gsquared94 merged commit 12d9a22 into GoogleContainerTools:main Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants