support tag@digest notation #579
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
|
Nice other then gofmt error. LGTM - Makes us bug for bug compatible. @mtrmac @containers/podman-maintainers PTAL |
libimage/pull.go
Outdated
| // (e.g., fedora:latest@sha256...). In that case, the tag is stripped | ||
| // off and entirely ignored. The digest is the sole source of truth. | ||
| name = normalizeTaggedDigestedString(name) | ||
|
|
There was a problem hiding this comment.
This location doesn’t make sense to me: the code should first decide what the syntax means (alltransports.ParseImageName + the other heuristics, then transform the input (and fail on errors instead of silently doing something unexpected).
That’s a bit awkward with the current dockerRef variable, but that’s because that dockerRef variable really shouldn’t exist when (looking at the strings.TrimPrefix(name, "docker://")and copyFromRegistry the code really wants to maintain a possiblyUnqualifiedDockerlikeName and not a types.ImageReference for that kind of syntax (and BTW doing that would show more explicitly that copyFromRegistry’s options.AllTags case interprets short names differently from the rest, it defaults to docker.io).
I am wondering a bit whether this shouldn’t happen in c/image/pkg/shortnames.Resolve. OTOH doing it here is consistent with the NormalizeName callers (which don’t want search), and Resolve is currently explicitly documented to preserve tags/digests, so there’s a good case for the code to live here.
libimage/pull.go
Outdated
| // (e.g., fedora:latest@sha256...). In that case, the tag is stripped | ||
| // off and entirely ignored. The digest is the sole source of truth. | ||
| name = normalizeTaggedDigestedString(name) | ||
|
|
||
| ref, err := alltransports.ParseImageName(name) | ||
| if err != nil { | ||
| // If the image clearly refers to a local one, we can look it up directly. |
There was a problem hiding this comment.
… I might be missing something; how do short names like busybox vs explicit docker://docker.io/busybox pulls work? AFAICS this code parses both into a docker://busybox ImageReference, losing the distinction — and then may try to do short name lookup on explicit docker://… formats, which should, I think, never happen — short name lookup is used only for Docker-like non-transport-qualified formats, so that the c/image-transport-qualified strings always mean the same thing in all callers.
There was a problem hiding this comment.
That's right. I agree that docker:// shouldn't do all the short-name limbo. Will fix it in another PR.
Thanks for checking. I think you're right. It's wise to make it more explicit. |
|
Addressed @mtrmac's comments ✔️ |
For the sake of Docker compatibility, support the tag@digest notation. In that case, the tag is stripped off the reference and the digest is the sole source of truth. Add a number of tests to make sure we're behaving as expected. Context: containers/podman/issues/6721 Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
For the sake of Docker compatibility, support the tag@digest notation.
In that case, the tag is stripped off the reference and the digest is
the sole source of truth.
Add a number of tests to make sure we're behaving as expected.
Context: containers/podman/issues/6721
Signed-off-by: Valentin Rothberg rothberg@redhat.com
@rhatdan @nalind PTAL