libartifact: strong type where possible #463
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| //go:build !remote | ||
|
|
||
| package store | ||
|
|
||
| import ( | ||
| "context" | ||
|
|
||
| "go.podman.io/image/v5/docker/reference" | ||
| ) | ||
|
|
||
| type ArtifactReference struct { | ||
| reference.Named | ||
baude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| // NewArtifactReference is a theoretical reference to an artifact. It needs to be | ||
| // a fully qualified oci reference except for tag, where we add | ||
| // "latest" as the tag if tag is empty. Valid references: | ||
| // | ||
| // quay.io/podman/machine-os:latest | ||
| // quay.io/podman/machine-os | ||
| // quay.io/podman/machine-os@sha256:916ede4b2b9012f91f63100f8ba82d07ed81bf8a55d23c1503285a22a9759a1e | ||
| // | ||
| // Note: Partial sha references and digests (IDs) are not allowed. | ||
| func NewArtifactReference(input string) (ArtifactReference, error) { | ||
baude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ar := ArtifactReference{} | ||
| named, err := stringToNamed(input) | ||
| if err != nil { | ||
| return ArtifactReference{}, err | ||
| } | ||
| ar.Named = named | ||
| return ar, nil | ||
| } | ||
|
|
||
| func (ar ArtifactReference) IsDigested() bool { | ||
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment. The way Go works, it is generally better to define interfaces at callers than callees, unless the callee is really an abstraction over multiple implementations.
Member
There was a problem hiding this comment. Thanks, I didn't realize that and learned something new.
Contributor
There was a problem hiding this comment. Oh, and:
|
||
| _, isDigested := ar.Named.(reference.Digested) | ||
| return isDigested | ||
| } | ||
|
|
||
| type ArtifactStoreReference struct { | ||
baude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ArtifactFromStore *Artifact | ||
| IsDigested bool | ||
baude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Ref reference.Named | ||
baude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| // NewArtifactStorageReference refers to an object already in the artifact store. It | ||
| // can be a name or a full or partial digest. Conveniently, it also embeds the artifact | ||
| // as part of its return. | ||
| func NewArtifactStorageReference(nameOrDigest string, as *ArtifactStore) (ArtifactStoreReference, error) { | ||
|
Member
There was a problem hiding this comment. I think it would be good to mention that these type of functions require the caller to hold a lock. |
||
| lookupInput := nameOrDigest | ||
| asf := ArtifactStoreReference{} | ||
| al, err := as.getArtifacts(context.Background(), nil) | ||
| if err != nil { | ||
| return ArtifactStoreReference{}, err | ||
| } | ||
|
|
||
| // Try to parse as a valid OCI reference | ||
| named, parseErr := stringToNamed(nameOrDigest) | ||
| if parseErr == nil { | ||
| lookupInput = named.String() | ||
| } | ||
|
|
||
| // Lookup in the store | ||
| a, isDigest, err := al.getByNameOrDigest(lookupInput) | ||
|
Contributor
There was a problem hiding this comment. By this time, the But I’m not saying it doesn’t work, but it feels wasteful (doesn’t really matter) and hard to follow (what I worry about). |
||
| if err != nil { | ||
| return ArtifactStoreReference{}, err | ||
| } | ||
|
|
||
| // If parsing failed, parse the artifact's name instead | ||
| if parseErr != nil { | ||
| fqName, err := a.GetName() | ||
|
Contributor
There was a problem hiding this comment. [MULTI], see elsewhere. … this assigns some of possibly several names matching the digest to the returned value. And then the name is used as the primary lookup key, e.g. in That does something, but it’s not clear that it’s what the user wanted. Again, can we entirely delete the “ID” lookup case? Or at least have it fail if there are multiple matching objects? Or would we want to have it fail only in some situations? … but this also applies to digest lookups: |
||
| if err != nil { | ||
| return ArtifactStoreReference{}, err | ||
| } | ||
| named, err = stringToNamed(fqName) | ||
| if err != nil { | ||
| return ArtifactStoreReference{}, err | ||
| } | ||
| } | ||
|
|
||
| asf.Ref = named | ||
| asf.IsDigested = isDigest | ||
| asf.ArtifactFromStore = a | ||
| return asf, nil | ||
| } | ||
|
|
||
| // stringToNamed converts a string to a reference.Named. | ||
| func stringToNamed(s string) (reference.Named, error) { | ||
| named, err := reference.ParseNamed(s) | ||
| if err != nil { | ||
| return ArtifactReference{}, err | ||
| } | ||
| // If the supplied input is neither tagged nor has | ||
| // a digest, then add "latest" | ||
| _, isTagged := named.(reference.Tagged) | ||
| _, isDigested := named.(reference.Digested) | ||
| if !isTagged && !isDigested { | ||
| named = reference.TagNameOnly(named) | ||
| } | ||
|
Comment on lines
+92
to
+98
Contributor
There was a problem hiding this comment. This can just be |
||
| return named, nil | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,186 @@ | ||
| package store | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| "go.podman.io/image/v5/types" | ||
| ) | ||
|
|
||
| func TestNewArtifactReference(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. (Consider using a table-driven test.)
Member
Author
There was a problem hiding this comment. do you have a pointer to something you hold in high regard ?
Contributor
There was a problem hiding this comment. (This is a non-blocking comment either way. Having verbose tests is better than not having tests.) I don’t care that much about the structure… the valuable part is avoiding repetition in the common parts.
|
||
| // Test valid reference | ||
| ar, err := NewArtifactReference("quay.io/podman/machine-os:5.1") | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, ar.Named) | ||
| assert.Equal(t, "quay.io/podman/machine-os:5.1", ar.Named.String()) | ||
|
|
||
| // Test another valid reference | ||
| ar, err = NewArtifactReference("docker.io/library/nginx:latest") | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, ar.Named) | ||
|
|
||
baude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // Test invalid reference - empty string | ||
| _, err = NewArtifactReference("") | ||
| assert.Error(t, err) | ||
|
|
||
| // Test invalid reference - malformed | ||
| _, err = NewArtifactReference("invalid::reference") | ||
| assert.Error(t, err) | ||
|
|
||
| // Test latest is added when no tag is provided | ||
| ar, err = NewArtifactReference("quay.io/machine-os/podman") | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, "quay.io/machine-os/podman:latest", ar.Named.String()) | ||
|
|
||
| // Input with a digest is good | ||
| ar, err = NewArtifactReference("quay.io/machine-os/podman@sha256:8b96f36deaf1d2713858eebd9ef2fee9610df8452fbd083bbfa7dca66d6fcd0b") | ||
| assert.NoError(t, err) | ||
| assert.True(t, ar.IsDigested()) | ||
|
|
||
| // Partial digests are a no-go | ||
| _, err = NewArtifactReference("quay.io/machine-os/podman@sha256:8b96f36deaf1d2") | ||
| assert.Error(t, err) | ||
|
|
||
| // "IDs" are also a no-go | ||
| _, err = NewArtifactReference("84ddb405470e733d0202d6946e48fc75a7ee231337bdeb31a8579407a7052d9e") | ||
| assert.Error(t, err) | ||
| } | ||
|
|
||
| func TestArtifactReference_IsDigested(t *testing.T) { | ||
| // Test reference with tag (not digested) | ||
| ar, err := NewArtifactReference("quay.io/podman/machine-os:5.1") | ||
| require.NoError(t, err) | ||
| assert.False(t, ar.IsDigested()) | ||
|
|
||
| // Test reference with digest (digested) | ||
| ar, err = NewArtifactReference("quay.io/podman/machine-os@sha256:8b96f36deaf1d2713858eebd9ef2fee9610df8452fbd083bbfa7dca66d6fcd0b") | ||
| require.NoError(t, err) | ||
| assert.True(t, ar.IsDigested()) | ||
|
|
||
| // Test reference with latest tag (not digested) | ||
| ar, err = NewArtifactReference("quay.io/podman/machine-os:latest") | ||
| require.NoError(t, err) | ||
| assert.False(t, ar.IsDigested()) | ||
| } | ||
|
|
||
| func TestNewArtifactStorageReference_ValidReference(t *testing.T) { | ||
| repo := "quay.io/podman/machine-os" | ||
| tag := "5.1" | ||
| ref := fmt.Sprintf("%s:%s", repo, tag) | ||
| as, artifactDigest := setupNewStore(t, ref, nil, nil) | ||
|
|
||
| // Test with a valid named reference - should find the artifact in the store | ||
| asr, err := NewArtifactStorageReference(ref, as) | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, asr.Ref) | ||
| assert.Equal(t, "quay.io/podman/machine-os:5.1", asr.Ref.String()) | ||
| assert.False(t, asr.IsDigested) | ||
| assert.NotNil(t, asr.ArtifactFromStore) | ||
| assert.Equal(t, "quay.io/podman/machine-os:5.1", asr.ArtifactFromStore.Name) | ||
|
|
||
| // Lookup by Digest | ||
| asr, err = NewArtifactStorageReference(fmt.Sprintf("%s@%s", repo, artifactDigest.String()), as) | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, asr.ArtifactFromStore) | ||
| assert.True(t, asr.IsDigested) | ||
| assert.NotNil(t, asr.ArtifactFromStore) | ||
| } | ||
|
|
||
| func TestNewArtifactStorageReference_AutoTagLatest(t *testing.T) { | ||
| repoNameOnly := "quay.io/podman/machine-os" | ||
| as, _ := setupNewStore(t, repoNameOnly, nil, nil) | ||
baude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // Test with a reference without a tag (should auto-add :latest) | ||
| asr, err := NewArtifactStorageReference(repoNameOnly, as) | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, asr.Ref) | ||
| assert.Equal(t, fmt.Sprintf("%s:latest", repoNameOnly), asr.Ref.String()) | ||
| assert.False(t, asr.IsDigested) | ||
| } | ||
|
|
||
| func TestNewArtifactStorageReference_InvalidReference(t *testing.T) { | ||
| storePath := filepath.Join(t.TempDir(), "store") | ||
| sc := &types.SystemContext{} | ||
|
|
||
| // Create an artifact store | ||
| as, err := NewArtifactStore(storePath, sc) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, as) | ||
|
|
||
| // Test with an invalid reference that also doesn't exist in the store | ||
| // This should fail both as a reference parse and as a store lookup | ||
| _, err = NewArtifactStorageReference("nonexistent-digest-12345", as) | ||
| assert.Error(t, err) | ||
| } | ||
|
|
||
| func TestNewArtifactStorageReference_EmptyString(t *testing.T) { | ||
| storePath := filepath.Join(t.TempDir(), "store") | ||
| sc := &types.SystemContext{} | ||
|
|
||
| // Create an artifact store | ||
| as, err := NewArtifactStore(storePath, sc) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, as) | ||
|
|
||
| // Test with empty string | ||
| _, err = NewArtifactStorageReference("", as) | ||
| assert.Error(t, err) | ||
| } | ||
|
|
||
| func TestStringToNamed(t *testing.T) { | ||
| // Test valid named reference | ||
| named, err := stringToNamed("quay.io/podman/machine-os:5.1") | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, named) | ||
| assert.Equal(t, "quay.io/podman/machine-os:5.1", named.String()) | ||
|
|
||
| // Test reference without tag (should add :latest) | ||
| named, err = stringToNamed("quay.io/podman/machine-os") | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, named) | ||
| assert.Equal(t, "quay.io/podman/machine-os:latest", named.String()) | ||
|
|
||
| // Test reference with digest | ||
| named, err = stringToNamed("quay.io/podman/machine-os@sha256:8b96f36deaf1d2713858eebd9ef2fee9610df8452fbd083bbfa7dca66d6fcd0b") | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, named) | ||
| assert.Equal(t, "quay.io/podman/machine-os@sha256:8b96f36deaf1d2713858eebd9ef2fee9610df8452fbd083bbfa7dca66d6fcd0b", named.String()) | ||
|
|
||
| // Test invalid reference | ||
| _, err = stringToNamed("invalid::reference") | ||
| assert.Error(t, err) | ||
|
|
||
| // Test empty string | ||
| _, err = stringToNamed("") | ||
| assert.Error(t, err) | ||
| } | ||
|
|
||
| func TestNewArtifactStore(t *testing.T) { | ||
| // Test with valid absolute path | ||
| storePath := filepath.Join(t.TempDir(), "store") | ||
| sc := &types.SystemContext{} | ||
|
|
||
| as, err := NewArtifactStore(storePath, sc) | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, as) | ||
| assert.Equal(t, storePath, as.storePath) | ||
|
|
||
| // Verify the index file was created | ||
| indexPath := filepath.Join(storePath, "index.json") | ||
| _, err = os.Stat(indexPath) | ||
| assert.NoError(t, err) | ||
|
|
||
| // Test with empty path | ||
| _, err = NewArtifactStore("", sc) | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), "store path cannot be empty") | ||
|
|
||
| // Test with relative path | ||
| _, err = NewArtifactStore("relative/path", sc) | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), "must be absolute") | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see there is a lot of iteration over lists. In extreme cases, when the store has a lot of artifacts, this will cost us some time. I think there could be an optimization of this search.