image list: return all associated names #7654
Conversation
openshift-ci-robot
added
the
approved
|
LGTM |
There was a problem hiding this comment.
- If this is the last caller of
ReposToMap, shouldn’t it be removed? That would EDITallowavoid similar confusion for any future callers. - Is it really correct to put digests to the RepoTags field? (I can’t quickly find documentation of the type). See also
ImageToImageSummary. OTOHtokenRepoTagdoes seem to be ready to handle digests.
Good catch, just removed it.
I think that's okay. RepoTags is part of the entities return type where we have full control over. We can change the name to better reflect the semantics (and also document the fields) in the future. |
AFAICT it is visible in the JSON output of Either way, displaying unexpected digests is better than not displaying anything at all, so this is clearly an improvement. |
test/e2e/images_test.go
Outdated
| // Prevent regressing on issue #7651. | ||
| result := podmanTest.Podman([]string{"images", "--all"}) | ||
| result.WaitWithDefaultTimeout() | ||
| Expect(result).Should(Exit(0)) |
There was a problem hiding this comment.
You know my feelings about "run a test and only check exit code, and just blindly la-la-la about its actual output". I'm writing my own system test anyway, so this will be covered, but I do wish I could instill the habit of checking results.
There was a problem hiding this comment.
Thanks for the reminder, @edsantiago! It was not on purpose but happened in a hurry. Since it's not merged, I will improve the test.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, vrothberg 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 |
|
@edsantiago Probably a bug, but a separate one. IMO, file it and we can fix later. |
|
And targets: https://bugzilla.redhat.com/show_bug.cgi?id=1879622 |
|
Changes LGTM |
Thanks! I referenced the BZ in the backport. |
Good catch! Docker prints the a single item only (in case there's a "real" tag). |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
Always return all associated names / repo tags of an image and fix a bug with malformed repo tags. Previously, Podman returned all names only with `--all` but this flag only instructs to list intermediate images and should not alter associated names. With `--all` Podman queried the repo tags of an image which splits all *tagged* names into repository and tag which is then reassembled to eventually be parsed again in the frontend. Lot's of redundant CPU heat and buggy as the reassembly didn't consider digests which ultimately broke parsing in the frontend. Fixes: containers#7651 Signed-off-by: Valentin Rothberg <[email protected]>
Got that sorted out as well. |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Always return all associated names / repo tags of an image and fix a bug
with malformed repo tags.
Previously, Podman returned all names only with
--allbut this flagonly instructs to list intermediate images and should not alter
associated names. With
--allPodman queried the repo tags of an imagewhich splits all tagged names into repository and tag which is then
reassembled to eventually be parsed again in the frontend. Lot's of
redundant CPU heat and buggy as the reassembly didn't consider digests
which ultimately broke parsing in the frontend.
Fixes: #7651
Signed-off-by: Valentin Rothberg [email protected]