-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add os, arch, and ismanifest to libpod image list #22266
Conversation
test/apiv2/12-imagesMore.at
Outdated
t GET libpod/images/json 200 \ .[0].IsManifestList=true | ||
t GET libpod/images/json 200 \ .[1].IsManifestList=false \ | ||
.[1].Arch=amd64 \ | ||
.[1].Os=linux |
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.
Hmm, I don't think my comment went through (because of the push?). Sorry if it's a dup.
Is this ordering guaranteed? If so, I think you can save yourself a push by deleting lines 95, 103, 108, and 109
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.
it looks fifo, and also dependent on what else might be in the store; this is why i was kind of asking about things.
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 don't see a sort
option in the API docs. We can't just go with "it looks fifo", we need to have a guarantee. If it is guaranteed to be fifo, or alphabetical, or any guarantee, then we can simplify this test a bit. If there are no guarantees, we need to rewrite it in a much more complicated way, with ?id=SHA
queries. @mheon do you know what guarantees (if any) there are with this endpoint?
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.
If order is guaranteed, here's a quicker and possibly safer test:
# test podman image SCP
# ssh needs to work so we can validate that the failure is past argument parsing
conn=apiv2test-temp-connection
podman system connection add --default $conn \
ssh://$USER@localhost/run/user/$UID/podman/podman.sock
# should fail but need to check the output...
# status 125 here means that the save/load fails due to
# cirrus weirdness with exec.Command. All of the args have been parsed successfully.
t POST "libpod/images/scp/$IMAGE?destination=QA::" 500 \
.cause="exit status 125"
# Clean up
podman system connection rm $conn
stop_registry
# Manifest stuff
#
# list images through the libpod endpoint should not return
# IsManifestList (bool), Arch (string), and Os (string)
t GET libpod/images/json 200 .[0].IsManifestList=false \
.[0].Arch=amd64 \
.[0].Os=linux
# if an image is a manifest list, it should not have
# anything for arch or os
podman manifest create foobar
t GET libpod/images/json 200 .[0].IsManifestList=false \
.[0].Arch=amd64 \
.[0].Os=linux \
.[1].IsManifestList=true \
.[1].Arch=null \
.[1].Os=null \
'.[1].RepoDigests | length=1'
# if a manifest list and an image are returned with libpod images
# endpoint, then one should be a manifest with IsManifest only; and
# the other image should have IsManifestList, Arch, and Os.
# FIXME: 'manifest add' does not change foobar in any way
podman manifest add --arch amd64 foobar $IMAGE
t GET libpod/images/json 200 .[0].IsManifestList=false \
.[0].Arch=amd64 \
.[0].Os=linux \
.[1].IsManifestList=true \
.[1].Arch=null \
.[1].Os=null \
'.[1].RepoDigests | length=2'
(I took the liberty of nuking and rearranging a lot of stuff that makes no sense otherwise)
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.
It is also missing the test case to call the compat API and make sure these fields are not set.
added and done |
30f9518
to
10559a5
Compare
Ephemeral COPR build failed. @containers/packit-build please check. |
when listing images through the restful service, consumers want to know if the image they are listing is a manifest or not because the libpod endpoint returns both images and manifest lists. in addition, we now add `arch` and `os` as fields in the libpod endpoint for image listing as well. Fixes: containers#22184 Fixes: containers#22185 Signed-off-by: Brent Baude <bbaude@redhat.com>
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude, Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
8a7c3ea
into
containers:main
when listing images through the restful service, consumers want to know if the image they are listing is a manifest or not because the libpod endpoint returns both images and manifest lists.
in addition, we now add
arch
andos
as fields in the libpod endpoint for image listing as well.Fixes: #22184
Fixes: #22185
Does this PR introduce a user-facing change?