Skip to content

fix: specifying platforms in ko builder #7135

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

Merged
merged 1 commit into from
Feb 22, 2022

Conversation

gsquared94
Copy link
Contributor

Related: #6120

Description

The ko builder already supports building multi-platform and cross-platform images. This PR removes the property artifact.ko.platforms from the config schema and uses artifact.platforms which makes it conform with all the other builders.

@gsquared94 gsquared94 requested a review from halvards February 22, 2022 01:06
@gsquared94 gsquared94 requested a review from a team as a code owner February 22, 2022 01:06
@codecov
Copy link

codecov bot commented Feb 22, 2022

Codecov Report

Merging #7135 (25842bc) into main (290280e) will decrease coverage by 2.07%.
The diff coverage is 56.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7135      +/-   ##
==========================================
- Coverage   70.48%   68.40%   -2.08%     
==========================================
  Files         515      560      +45     
  Lines       23150    26239    +3089     
==========================================
+ Hits        16317    17950    +1633     
- Misses       5776     7054    +1278     
- Partials     1057     1235     +178     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/deploy.go 52.00% <ø> (-1.85%) ⬇️
cmd/skaffold/app/cmd/dev.go 84.61% <0.00%> (ø)
cmd/skaffold/app/cmd/render.go 36.66% <0.00%> (-4.72%) ⬇️
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/lsp.go 28.12% <28.12%> (ø)
cmd/skaffold/app/cmd/fix.go 68.85% <40.00%> (-7.62%) ⬇️
cmd/skaffold/app/cmd/lint.go 42.85% <42.85%> (ø)
cmd/skaffold/app/cmd/find_configs.go 48.88% <50.00%> (+0.24%) ⬆️
cmd/skaffold/app/skaffold.go 76.19% <70.00%> (-8.43%) ⬇️
... and 215 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9de79cd...25842bc. Read the comment docs.

@halvards
Copy link
Contributor

Thanks for doing this @gsquared94.

There's one regression: The ko builder supports the value all in the platforms field: https://skaffold.dev/docs/pipeline-stages/builders/ko/#multi-platform-images

Using all results in ko building for all platforms supported by the base image (ko docs here: https://github.com/google/ko#multi-platform-images). It's a useful feature, and it'd be great to retain it. I appreciate that this might mean having to support it for all the builders, or at least handle all in a sensible way.

@gsquared94
Copy link
Contributor Author

gsquared94 commented Feb 22, 2022

Thanks for doing this @gsquared94.

There's one regression: The ko builder supports the value all in the platforms field: https://skaffold.dev/docs/pipeline-stages/builders/ko/#multi-platform-images

Using all results in ko building for all platforms supported by the base image (ko docs here: https://github.com/google/ko#multi-platform-images). It's a useful feature, and it'd be great to retain it. I appreciate that this might mean having to support it for all the builders, or at least handle all in a sensible way.

Thanks for the quick review @halvards . I've fixed the missing parsing logic for all platform value.
Tested against the sample integration/examples/ko, by running:

skaffold build --platform=all --default-repo=gcr.io/k8s-skaffold

@halvards
Copy link
Contributor

LGTM. We'll have to update the ko builder documentation page before the next release, since it references the old config field.

@gsquared94
Copy link
Contributor Author

LGTM. We'll have to update the ko builder documentation page before the next release, since it references the old config field.

updated the docs.

@gsquared94 gsquared94 enabled auto-merge (squash) February 22, 2022 19:19
Copy link
Contributor

@tejal29 tejal29 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 disabled auto-merge February 22, 2022 20:33
@gsquared94 gsquared94 merged commit 067fa70 into GoogleContainerTools:main Feb 22, 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.

4 participants