c8d: ImageService.GetImage: fix filtering by platform#47183
Draft
thaJeztah wants to merge 1 commit intomoby:masterfrom
Draft
c8d: ImageService.GetImage: fix filtering by platform#47183thaJeztah wants to merge 1 commit intomoby:masterfrom
thaJeztah wants to merge 1 commit intomoby:masterfrom
Conversation
Before this patch, it would find all images (and platform) are found,
and return the first image (or the old default: "linux/amd64");
With this patch, an error is returned if a platform is set, and no
results were found:
No such image: alpine:latest for platform: windows/s390x
Looking at consumers of this function, there may be something to fix though;
https://github.com/moby/moby/blob/178651733852a26bc11e6fa272b48ff37ecc7447/daemon/create.go#L85-L86
if opts.params.Platform == nil && opts.params.Config.Image != "" {
img, err := daemon.imageService.GetImage(ctx, opts.params.Config.Image, imagetypes.GetImageOpts{Platform: opts.params.Platform})
The code above looks to check for _NO_ platform to be passed in parameters, but
then to pass that platform as `imagetypes.GetImageOpts.Platform`.
That code was added in 300c11c but that was a
refactor, combining multiple branches, so before that we passed platform
unconditionally (but only used the image in some cases, so that was an
optimization).
Should we;
- always lookup the image?
- should `GetImage` use OnlyPlatformWithFallback as default (if no platform
is specified)? To select any image, but _prefer_ local platform?
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before this patch, it would find all images (and platform) are found, and return the first image (or the old default: "linux/amd64");
With this patch, an error is returned if a platform is set, and no results were found:
Looking at consumers of this function, there may be something to fix though;
moby/daemon/create.go
Lines 85 to 86 in 1786517
The code above looks to check for NO platform to be passed in parameters, but then to pass that platform as
imagetypes.GetImageOpts.Platform.That code was added in 300c11c but that was a refactor, combining multiple branches, so before that we passed platform unconditionally (but only used the image in some cases, so that was an optimization).
Should we;
GetImageuse OnlyPlatformWithFallback as default (if no platform is specified)? To select any image, but prefer local platform?- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)