Pre-parse platform string with StringSliceVar #551
Conversation
This allows users to declare --platform multiple times and have the values appended, i.e.: ko build --platform=linux/amd64 --platform=linux/arm64 is equivalent to ko build --platform=linux/amd64,linux/arm64 As a side effect, platformMatcher.spec and gobuildOpener.platforms are now of type []string (instead of string) to maintain structure of information from flag parsing.
pkg/commands/config.go
Outdated
| allPlatforms := len(bo.Platforms) == 1 && bo.Platforms[0] == "all" | ||
| selectiveMultiplatform := len(bo.Platforms) > 1 | ||
| multiplatform := allPlatforms || selectiveMultiplatform | ||
| if len(bo.Platforms) > 0 && !multiplatform { |
There was a problem hiding this comment.
I like the new descriptive variable names!
I think bo.Platforms is always non-empty at this point, because we set it to some default if unset. Can this just be if !multiplatform? That makes the bo.Platforms[0] easier to understand, since we'll know there's only one value.
There was a problem hiding this comment.
@imjasonh less than 24 hours is not "late" 😄 — thank you for reviewing! Revision incoming in a bit, though would like confirmation on the --platform=all,linux/arm64 scenario.
Codecov Report
@@ Coverage Diff @@
## main #551 +/- ##
==========================================
+ Coverage 48.82% 48.88% +0.06%
==========================================
Files 42 42
Lines 2204 2207 +3
==========================================
+ Hits 1076 1079 +3
Misses 946 946
Partials 182 182
Continue to review full report at Codecov.
|
|
The diff in Verify Codegen seems expected: - --platform strings Which platform to use when pulling a multi-platform base. Format: all | <os>[/<arch>[/<variant>]][,platform]*
+ --platform string Which platform to use when pulling a multi-platform base. Format: all | <os>[/<arch>[/<variant>]][,platform]*Should I run Edit: running it seems only to update the expected lines, so I'm going to push it. |
Internally cobra/pflag defines StringSliceVar as "strings" whereas StringVar is defined as "string". This change is updated by running hack/update-codegen.sh script.
pkg/build/options.go
Outdated
| @@ -89,9 +89,9 @@ func WithConfig(buildConfigs map[string]Config) Option { | |||
| // | |||
| // platform = <os>[/<arch>[/<variant>]] | |||
| // allowed = all | platform[,platform]* | |||
| func WithPlatforms(platforms string) Option { | |||
| func WithPlatforms(platforms []string) Option { | |||
There was a problem hiding this comment.
@jonjohnsonjr had the good idea that this change could be WithPlatforms(platforms string...) and not break existing users. (Thanks Jon!)
If you feel like doing this change in this PR that'd be great, if not I can do it in a followup before we release this.
Otherwise, lgtm!
There was a problem hiding this comment.
Ah yes! I felt silly I didn't think of it in the first place. Fixing...
There was a problem hiding this comment.
🤔 actually, using ...string would not break compile-time for existing users, but it would break the behavior, unless we do some sort of strings.Spilt here.
Do we want to add that logic as well? Something like:
func WithPlatforms(platforms ...string) Option {
if len(platforms) == 1 {
platforms = strings.Split(platforms, ",")
}
...
}There is an itch on me that this code is waiting to be deleted, but happy to make changes if maintainers are ok with maintaining it 😄
There was a problem hiding this comment.
Yeah let's add the behavior you describe, for folks using WithPlatforms("linux/amd64,linux/arm64"), and document that this behavior will change in a future release. This is a fairly small concession we should make to existing library users.
Update comments to reflect implementation as well.
|
thanks for the review folks! 🎉 |
|
Thanks for working on this, and sorry for the holiday-related slowness in reviewing it again. Looks like there's a build failure, otherwise LGTM. |
|
That was a silly mistake, apologies! Happy new year! 🎉 |
Hello! First time contributing to
kohere 👋🏼BuildOptions.Platformwas renamed toBuildOptions.Platformsto reflect the actual data type, though I'm not sure how comfortable are we in breaking changes of (supposedly) public structs.platformMatcher.specandgobuildOpener.platformsare now of type[]string(instead ofstring).Happy to discuss and make changes as desired by maintainers!
Closes #537