allow lookup of artifact by digest#589
Conversation
|
✅ A new PR has been created in buildah to vendor these changes: podman-container-tools/buildah#6635 |
mtrmac
left a comment
There was a problem hiding this comment.
ACK otherwise.
(I’m sighing about the AI verbosity but I’ll leave those stylistic choices entirely to the primary authors of these packages.)
|
Packit jobs failed. @containers/packit-build please check. |
mtrmac
left a comment
There was a problem hiding this comment.
ACK overall. Primarily the “do a string match first” comment — I don’t feel strongly that it is blocking, but I did want to at least discuss that.
| continue | ||
| } | ||
| if storedRef.Name() == lookupRef.Name() { | ||
| artifactDigest := a.GetDigest() |
There was a problem hiding this comment.
This only matches against digest.Canonical. I think not supporting arbitrary digests (and having to compute them from rawManifest for now is perfectly fine. In particular it’s unclear what the registry API will look like, it seems they are leaning towards every object on the registry having an associated digest algorithm, with no expectation of cross-digest references (push by sha256; pull by sha512) to work, so maybe references by arbitrary digests will never be necessary.
One thing that we can cheaply do is do the a.Name == lookupName loop first, unconditionally; that might might match a digest cheaply, and it might match a non-sha256 digest if the artifact was pulled using one. Only if that fails, do this loop that parses references. That would not allow (pull by tag; reference by sha512 digest) but (pull by sha512 digest; reference by sha512 digest) would work, a small step forward and a microoptimization :) (A downside is that it would not be easy to unit-test.)
|
this pr revealed a problem in podman ... i'm going to fix that and repush. when the draft status ahs been removed, its ready for merge |
|
From another discussion
LGTM to that as well. |
Issue podman-container-tools#408 describes libartifact not working for use cases where consumers want to look up an artifact by it's digest. This allows, among other things, for inspect to be done by digest. As part of this fix, did some refactoring of the package itself for the convenience of the expanded Artifact struct. Artifacts now have the digest and raw manifest as a []byte within its structure. There are accessors for both. Moreover, GetDigest no longer returns an error. Fixes podman-container-tools#408 Signed-off-by: Brent Baude <bbaude@redhat.com>
|
LGTM |
Issue #408 describes libartifact not working for use cases where consumers want to look up an artifact by it's digest. This allows, among other things, for inspect to be done by digest.
Fixes #408