- Notifications
You must be signed in to change notification settings - Fork18.8k
c8d: Adjust "image list" to return only a single item for each image store entry#45967
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
I don't think we should merge this, the current API response might not be backwards compatible forsome clients (it is for the docker cli), but at least it gives information about all the images that are pulled/built. This change would hide that information. I think we should hold on while we get the final shape of the multi-platform aware API and a new version of the cli that knows how to show all the information aboutall the local images. |
I do think that the current situation is confusing, and that this PR brings us one step closer to the intended outcome; docker pull busybox:latest1fa89c01cd04: Already existsfc9db2894f4e: Already exists3fbc63216742: Already existsdocker.io/library/busybox:latestdocker image lsREPOSITORY TAG IMAGE ID CREATED SIZEbusybox latest 3fbc63216742 5 seconds ago 6.09MBdocker image inspect --format'{{.Architecture}}' 3fbc63216742arm64docker image inspect --format'{{.Size}}' 3fbc632167421920927docker pull --platform=linux/amd64 busybox:latest3fbc63216742: Already exists023917ec6a88: Downloadcompletea416a98b71e2: Downloadcomplete3f4d90098f5b: Downloadcompletedocker.io/library/busybox:latestdocker image lsREPOSITORY TAG IMAGE ID CREATED SIZEbusybox latest 3fbc63216742 5 seconds ago 6.09MBbusybox latest 3fbc63216742 9 days ago 6.61MBdocker image inspect --format'{{.Architecture}}' 3fbc63216742arm64docker image inspect --format'{{.Size}}' 3fbc632167421920927 Even though there's 2 images shown, there's no way to interact with them individually; removing It currently also breaks Desktop's Dashboard; here's the overview of images (the overview does not show the right size for each):  Search for "busybox", and you now have 4 images:  Search again, and now you have 7 images:  |
Exactly why my proposal was totemporarily show the manifest ID and not the manifest list ID. Also adding theplatform to the images list would be a nice way to tell them appart
I agree that it's confusing currently, but I don't agree that this takes us any closer to the end goal. This change is also confusing, building a multi-platofrm image yields only one image in |
That's why it's a multi-platform image. If you want multiple platform images, you would give them a different name. |
…store entryThis will return a single entry for each name/value pair, and for nowall the "image specific" metadata (labels, config, size) should beeither "default platform" or "first platform we have locally" (whichthen matches the logic for commands like `docker image inspect`, etc)with everything else (just ID, maybe?) coming from the manifestlist/index.That leaves room for the longer-term implementation to add new fields todescribe the _other_ images that are part of the manifest list/index.Co-authored-by: Tianon Gravi <admwiggin@gmail.com>Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
vvoland commentedFeb 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I went ahead and rebased this, addressed some of the TODOs. Windows failures expected. |
| // TODO we should probably show *something* for images we've pulled | ||
| // but are 100% shallow or an empty manifest list/index | ||
| // ("tianon/scratch:index" is an empty example image index and | ||
| // "tianon/scratch:list" is an empty example manifest list) |
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.
Ah, yes, was about to ask if we could test this. As these are minimal images, perhaps we could include them in our "frozen images"?
I'm fine with doing that in a follow-up if it's complicated though.
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.
Not sure if it makes sense to handle this right now.
- It's impossible to pull such image, you can only load it via
docker load(or going via containerd store directly). - Even if it is loaded, there's nothing that can be done with that image (except removing/pushing it).
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.
Oh! I should read properly, I thought this one also described "multi-arch" image to test. But this was only for the "lots' of emptiness" case.
I mostly meant; we should have a test to verify that multiple arches of an image are properly grouped into a single image when usingdocker image ls (i.e., with this PR merged)
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.
LGTM
Gave this one another spin; docker pull busyboxUsing default tag: latestlatest: Pulling from library/busyboxc2bf9493c1bf: DownloadcompleteDigest: sha256:6d9ac9237a84afe1516540f40a0fafdc86859b2141954b4d643af7066d598b74Status: Downloaded newer imagefor busybox:latestdocker.io/library/busybox:latestdocker pull --platform=linux/amd64 busyboxUsing default tag: latestlatest: Pulling from library/busybox9ad63333ebc9: DownloadcompleteDigest: sha256:6d9ac9237a84afe1516540f40a0fafdc86859b2141954b4d643af7066d598b74Status: Image is up to datefor busybox:latestdocker.io/library/busybox:latest Before this PR: docker image lsREPOSITORY TAG IMAGE ID CREATED SIZEbusybox latest 6d9ac9237a84 5 weeks ago 6.61MBbusybox latest 6d9ac9237a84 5 weeks ago 6.09MB After this PR: docker image lsREPOSITORY TAG IMAGE ID CREATED SIZEbusybox latest 6d9ac9237a84 5 weeks ago 6.61MB As a follow-up (?) we should probably look at the |
vvoland commentedFeb 26, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Good call, I added test and combined the image size. Do we want to merge this as-is and then review the follow up separately? |
This will return a single entry for each name/value pair, and for now all the "image specific" metadata (labels, config, size) should be either "default platform" or "first platform we have locally" (which then matches the logic for commands like
docker image inspect, etc) with everything else (just ID, maybe?) coming from the manifest list/index. That leaves room for the longer-term implementation to add new fields to describe theother images that are part of the manifest list/index.Refs#45948,#45735 (especially#45735 (comment))