Skip to content

feat(libartifact): Default to latest tag when pulling artifacts #409

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(libartifact): Default to latest tag when pulling artifacts #409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion common/pkg/libartifact/artifact.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

"github.com/opencontainers/go-digest"
"go.podman.io/common/pkg/libartifact/types"
"go.podman.io/image/v5/docker/reference"
"go.podman.io/image/v5/manifest"
)

Expand Down Expand Up @@ -58,9 +59,22 @@ type ArtifactList []*Artifact
// GetByNameOrDigest returns an artifact, if present, by a given name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Returns an error if not found.
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)
Copy link
Contributor

@mtrmac mtrmac Oct 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.]

}

// Assert that named implements reference.Named interface
namedRef, ok := named.(reference.Named)
if !ok {
return nil, false, fmt.Errorf("reference %q is not a Named reference", nameOrDigest)
}

refStr := reference.TagNameOnly(namedRef).String()

// This is the hot route through
for _, artifact := range al {
if artifact.Name == nameOrDigest {
if artifact.Name == refStr {
return artifact, false, nil
}
}
Expand Down
43 changes: 31 additions & 12 deletions common/pkg/libartifact/store/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"go.podman.io/common/libimage"
"go.podman.io/common/pkg/libartifact"
libartTypes "go.podman.io/common/pkg/libartifact/types"
"go.podman.io/image/v5/docker/reference"
"go.podman.io/image/v5/image"
"go.podman.io/image/v5/manifest"
"go.podman.io/image/v5/oci/layout"
Expand Down Expand Up @@ -98,14 +99,14 @@ func (as ArtifactStore) Remove(ctx context.Context, name string) (*digest.Digest
return nil, err
}

arty, nameIsDigest, err := artifacts.GetByNameOrDigest(name)
arty, _, err := artifacts.GetByNameOrDigest(name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any caller left that consumes the nameIsDigest value? If not, please drop it everywhere. But…

if err != nil {
return nil, err
}
if nameIsDigest {
name = arty.Name
}
ir, err := layout.NewReference(as.storePath, name)

// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

… 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)?

if err != nil {
return nil, err
}
Expand Down Expand Up @@ -186,7 +187,17 @@ func (as ArtifactStore) Push(ctx context.Context, src, dest string, opts libimag
as.lock.Lock()
defer as.lock.Unlock()

srcRef, err := layout.NewReference(as.storePath, src)
artifacts, err := as.getArtifacts(ctx, nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err != nil {
return "", err
}

arty, _, err := artifacts.GetByNameOrDigest(src)
if err != nil {
return "", err
}

srcRef, err := layout.NewReference(as.storePath, arty.Name)
if err != nil {
return "", err
}
Expand Down Expand Up @@ -214,6 +225,18 @@ func (as ArtifactStore) Add(ctx context.Context, dest string, artifactBlobs []li
return nil, ErrEmptyArtifactName
}

named, err := reference.Parse(dest)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err != nil {
return nil, fmt.Errorf("parsing reference %q: %w", dest, err)
}

namedRef, ok := named.(reference.Named)
if !ok {
return nil, fmt.Errorf("reference %q is not a Named reference", dest)
}

dest = reference.TagNameOnly(namedRef).String()

if options.Append && len(options.ArtifactMIMEType) > 0 {
return nil, errors.New("append option is not compatible with type option")
}
Expand Down Expand Up @@ -416,20 +439,16 @@ func getArtifactAndImageSource(ctx context.Context, as ArtifactStore, nameOrDige
return nil, nil, err
}

arty, nameIsDigest, err := artifacts.GetByNameOrDigest(nameOrDigest)
arty, _, err := artifacts.GetByNameOrDigest(nameOrDigest)
if err != nil {
return nil, nil, err
}
name := nameOrDigest
if nameIsDigest {
name = arty.Name
}

if len(arty.Manifest.Layers) == 0 {
return nil, nil, errors.New("the artifact has no blobs, nothing to extract")
}

ir, err := layout.NewReference(as.storePath, name)
ir, err := layout.NewReference(as.storePath, arty.Name)
if err != nil {
return nil, nil, err
}
Expand Down
Loading