-
Notifications
You must be signed in to change notification settings - Fork 406
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
Error if image platform does not match desired #785
Conversation
If the user specifies an image manifest and its platform does not match the platforms specified or defaulted, error out. Signed-off-by: Ben Moss <[email protected]>
a5ed557
to
8211102
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mostly looks good, thanks for the contribution!
If there was an additional test in the e2e tests that checked that ko build --platform=linux/fake
resulted in an error, I'd award extra bonus points 😁🏅
@@ -437,6 +437,7 @@ func TestGoBuildNoKoData(t *testing.T) { | |||
WithBaseImages(func(context.Context, string) (name.Reference, Result, error) { return baseRef, base, nil }), | |||
withBuilder(writeTempFile), | |||
withSBOMber(fauxSBOM), | |||
WithPlatforms("all"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused a bit why this addition is needed for tests. I would have expected random.Image
to produce a linux/amd64
image by default, and for NewGo
to assume that platform if none is provided, so that platform matcher check you added wouldn't be necessary.
Does this change require callers of the Go API to explicitly pass WithPlatforms
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
random.Image
has no platform (or any config at all), so you get this somewhat odd error
gobuild_test.go:448: Build() = base image platform "" does not match desired platforms []
It looks like from empty.Image
it just gets an empty Platform struct:
*v1.Platform{Architecture: "", OS: "", OSVersion: "", OSFeatures: []string len: 0, cap: 0, nil, Variant: "", Features: []string len: 0, cap: 0, nil}
The CLI calls gobuildOptions which handles the defaulting of desired platforms.
My understanding from reading over the code is that the Go API (NewGo
) doesn't do any defaulting, so if the user didn't set any desired platforms the current behavior would be:
- if given an image index/manifest list, no platforms would match
- if given an image manifest, it would copy the platform from the base image
The tests were relying on that latter behavior, so the resulting image would also have a blank platform.
We'd have to set |
Fixes #699
If the user specifies an image manifest and its platform does not match the platforms specified or defaulted, error out.
With a
.ko.yaml
specifying the linux/amd64 image manifest instead of the multi-platform index:Prior behavior
New behavior
As mentioned in #699
--platform=all
is a lil ambiguous so we're keeping that behavior: