Skip to content

Add unit tests for artifact store #554

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

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

Add unit tests for artifact store #554

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
73 changes: 55 additions & 18 deletions common/pkg/libartifact/store/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,9 @@ func (as ArtifactStore) Add(ctx context.Context, dest ArtifactReference, artifac
if options.Append && len(options.ArtifactMIMEType) > 0 {
return nil, errors.New("append option is not compatible with type option")
}
if options.Append && options.Replace {
return nil, errors.New("append and replace options are mutually exclusive")
}

locked := true
as.lock.Lock()
Expand All @@ -246,9 +249,58 @@ func (as ArtifactStore) Add(ctx context.Context, dest ArtifactReference, artifac
fileNames := map[string]struct{}{}

arty, lookupErr := as.lookupArtifactLocked(ctx, dest.ToArtifactStoreReference())
if !options.Append {
// Check if artifact exists; in GetByName not getting an
// error means it exists

switch {
case options.Append:
// Append to existing artifact
if lookupErr != nil {
return nil, lookupErr
}
artifactManifest = arty.Manifest.Manifest
var err error
oldDigest, err = arty.GetDigest()
if err != nil {
return nil, err
}
for _, layer := range artifactManifest.Layers {
if value, ok := layer.Annotations[specV1.AnnotationTitle]; ok && value != "" {
fileNames[value] = struct{}{}
}
}
case options.Replace:
// Replace existing artifact - delete old one if it exists, then create new
if lookupErr == nil {
ir, err := layout.NewReference(as.storePath, dest.String())
if err != nil {
return nil, err
}
if err := ir.DeleteImage(ctx, as.SystemContext); err != nil {
return nil, err
}
var err2 error
oldDigest, err2 = arty.GetDigest()
Copy link
Contributor

@mtrmac mtrmac Dec 12, 2025

Choose a reason for hiding this comment

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

AFAICS oldDigest is later used for deleting the old version; if we already fully delete it here, isn’t that sufficient?

If this were required: It might work just fine as is — just, as a reader, reading metadata about an object after deleting it would require an extra look, and ordering it the other way would trivially remove that concern.

if err2 != nil {
return nil, err2
}
}

// Create new manifest
annotations := make(map[string]string)
if options.Annotations != nil {
annotations = maps.Clone(options.Annotations)
}
annotations[specV1.AnnotationCreated] = time.Now().UTC().Format(time.RFC3339Nano)

artifactManifest = specV1.Manifest{
Versioned: specs.Versioned{SchemaVersion: ManifestSchemaVersion},
MediaType: specV1.MediaTypeImageManifest,
ArtifactType: options.ArtifactMIMEType,
Config: specV1.DescriptorEmptyJSON,
Layers: make([]specV1.Descriptor, 0),
Annotations: annotations,
}
Comment on lines +287 to +301
Copy link
Contributor

@mtrmac mtrmac Dec 12, 2025

Choose a reason for hiding this comment

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

Would it be worthwhile to share this with the default: case? This new version already dropped a comment, the are very likely to get of sync over time.

default:
// Add new artifact - error if it already exists
if lookupErr == nil {
return nil, fmt.Errorf("%s: %w", dest.String(), libartTypes.ErrArtifactAlreadyExists)
}
Expand All @@ -269,21 +321,6 @@ func (as ArtifactStore) Add(ctx context.Context, dest ArtifactReference, artifac
Layers: make([]specV1.Descriptor, 0),
Annotations: annotations,
}
} else {
if lookupErr != nil {
return nil, lookupErr
}
artifactManifest = arty.Manifest.Manifest
var err error
oldDigest, err = arty.GetDigest()
if err != nil {
return nil, err
}
for _, layer := range artifactManifest.Layers {
if value, ok := layer.Annotations[specV1.AnnotationTitle]; ok && value != "" {
fileNames[value] = struct{}{}
}
}
}

for _, artifact := range artifactBlobs {
Expand Down
Loading
Loading