Fix ID and Digest filters #1676
Conversation
The id and digest filters for podman images was broken as it would only match on the full ID or Digest, so when users would give a substring of either not results would be given even though there were matches. Fix to check on prefix now instead. Signed-off-by: Urvashi Mohnani <umohnani@redhat.com>
|
LGTM |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, rhatdan, umohnani8 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 |
There was a problem hiding this comment.
The problem with partial matches is that more than one object may match. Podman will hence error out if multiple containers match during name resolution.
We need to do the same here. There is no guarantee (necessarily) in which order images are being filtered so if we have two images starting with the same ID or digest, any of them may be returned.
Hence, for partial matches we need to filter all objects and error out if we have more than one.
@umohnani8 can you follow up on that?
| {string(busybox.Digest()), 1, busybox.ID()}, | ||
| {string(alpine.Digest()), 1, alpine.ID()}, | ||
| {string(busybox.Digest()[:10]), 1, busybox.ID()}, | ||
| {string(alpine.Digest()[:10]), 1, alpine.ID()}, |
There was a problem hiding this comment.
Nit: now it seems we're not testing an exact but only a partial match.
|
I was thinking about my comment above again and believe I was wrong. Assuming there are multiple matches, it's perfectly fine to return all matching objects. If there are callers expecting to find exactly one match, they need to deal with it. So please ignore my comment. |
The id and digest filters for podman images was broken as it would only match on the full ID or Digest, so when users would give a substring of either not results would be given even though there were matches.
Fix to check on prefix now instead.
Helps fix containers/podman#19966