feat(libartifact): Default to latest tag when pulling artifacts #409
Conversation
When an artifact is referenced by name but without a specific tag, the code should default to using the 'latest' tag. This aligns with the behavior of image pulls in Podman and other container tools. This change modifies the artifact store to correctly resolve untagged names to the 'latest' tag, ensuring a more intuitive and consistent user experience. Related: containers/podman#27083 Signed-off-by: Lewis Denny <[email protected]>
|
✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6445 |
This change implements a feature where podman artifacts will default to using the 'latest' tag if no tag is provided by the user. This aligns the behavior of artifacts with that of container images, providing a more consistent user experience. Fixes: containers#27083 Requires: containers/container-libs#409 Signed-off-by: Lewis Denny <[email protected]>
There was a problem hiding this comment.
By no means a full review, I was just lazily looking at the code changes here, as related to c/image API. There’s a lot I don’t know (starting with the “item without a name” situation).
Hoping this might be useful as is, or I’ll do a more detailed review if necessary.
| return nil, ErrEmptyArtifactName | ||
| } | ||
|
|
||
| named, err := reference.Parse(dest) |
There was a problem hiding this comment.
Just a note: reference.Parse is typically the wrong thing to do…
I don’t think it’s strongly documented, but c/image generally expects reference.Named values to contain a registry name (typically from reference.ParseNormalizedNamed).
In this case, where the input can ~legitimately be a short name[1], doing this and immediately returning back to a string form is, at least, locally consistent, and, in that sense, fine.
[1] There is a fairly strong consensus that we should forbid short names for artifacts entirely, e.g. #406 . But that’s not this PR.
| } | ||
|
|
||
| arty, nameIsDigest, err := artifacts.GetByNameOrDigest(name) | ||
| arty, _, err := artifacts.GetByNameOrDigest(name) |
There was a problem hiding this comment.
Is there any caller left that consumes the nameIsDigest value? If not, please drop it everywhere. But…
| defer as.lock.Unlock() | ||
|
|
||
| srcRef, err := layout.NewReference(as.storePath, src) | ||
| artifacts, err := as.getArtifacts(ctx, nil) |
There was a problem hiding this comment.
Non-blocking? getArtifacts does I/O … or, well, at least system calls.
It might be worth integrating this more with GetByNameOrDigest, so that the full listing only happens if it is necessary for the digest lookup.
| func (al ArtifactList) GetByNameOrDigest(nameOrDigest string) (*Artifact, bool, error) { | ||
| named, err := reference.Parse(nameOrDigest) | ||
| if err != nil { | ||
| return nil, false, fmt.Errorf("parsing reference %q: %w", nameOrDigest, err) |
There was a problem hiding this comment.
Uh… the code below seems to support digests (without repo names), and digest prefixes. Such inputs are… only accidentally accepted through reference.Parse (sha256:foo is parsed as “repository sha256, tag foo).
Does that work? There are no unit tests.
[I’m also aesthetically uneasy about matching names as either named references or digests, primarily due to the parsing ambiguity above. But that’s not this PR.]
|
|
||
| type ArtifactList []*Artifact | ||
|
|
||
| // GetByNameOrDigest returns an artifact, if present, by a given name |
There was a problem hiding this comment.
Unrelated and should not be fixed in this PR, just something I happened to notice: GetDigest re-creates a JSON from parsed data, that’s not guaranteed to compute the right digest.
|
|
||
| // Use the canonical name from the found artifact, which will include the tag | ||
| // if one was resolved by GetByNameOrDigest. | ||
| ir, err := layout.NewReference(as.storePath, arty.Name) |
There was a problem hiding this comment.
… I suppose this is pre-existing, but the code in getArtifacts seems to be set up for repositories where an item might not have a name at all.
What should happen in that case? Is that something that should never be possible via libartifact, and thus can be ignored / reported as an error — or is that a legitimate possibility that needs to be accounted for (via using layout.NewIndexReference)?
3 participants
When an artifact is referenced by name but without a specific tag, the code should default to using the 'latest' tag. This aligns with the behavior of image pulls in Podman and other container tools.
This change modifies the artifact store to correctly resolve untagged names to the 'latest' tag, ensuring a more intuitive and consistent user experience.
Related: containers/podman#27083