Skip to content
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

Add Copy.Options.ReportResolvedReference #2609

Merged
merged 4 commits into from
Nov 5, 2024
Merged

Add Copy.Options.ReportResolvedReference #2609

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
831269d
Rename an options variable to imgOptions
mtrmac Oct 18, 2024
91d22b2
Introduce private.ImageDestination.CommitWithOptions
mtrmac Oct 18, 2024
125f862
Return a precise reference to the created image when writing to conta…
mtrmac Oct 21, 2024
6ba898f
HACK: Only return an image ID from ReportResolvedReference for c/storage
mtrmac Nov 4, 2024
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 copy/copy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,16 @@ type Options struct {
// DestinationCtx.CompressionFormat is used exclusively, and blobs of other
// compression algorithms are not reused.
ForceCompressionFormat bool

// ReportResolvedReference, if set, asks the destination transport to store
// a “resolved” (more detailed) reference to the created image
// into the value this option points to.
// What “resolved” means is transport-specific.
// Most transports don’t support this, and cause the value to be set to nil.
//
// For the containers-storage: transport, the reference contains an image ID,
// so that storage.ResolveReference returns exactly the created image.
ReportResolvedReference *types.ImageReference
Copy link
Member

Choose a reason for hiding this comment

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

I find it somewhat confusing that we use the Options field to return information to the caller. But I guess that is needed to avoid breaking changes in the API? I also get the that this is why it needs a pointer to an interface (which itself is also a pointer) but that seems a bit confusing to use as a caller.

Anyway I don't know the code + history here. I trust you that this is the best design

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we are committed not to change the API.

Alternatively, we could add a copy.ImageWithResolvedReference returning the extra value… and dropping the manifest bytes return value? I don’t know. Most users just don’t need the value.

Sort of hiding this feature in the Options field means we still are committed to API stability, but most users won’t see this and won’t worry about the extra values. And, also, the code implementing the copy can tell whether the caller wants the value or not.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I am fine with this, you are the main maintainer here after all and if you like this approach. We don't use the merge bot here, do we? So feel free to merge and move it along into c/common.

}

// OptionCompressionVariant allows to supply information about
Expand Down Expand Up @@ -337,8 +347,12 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef,
}
}

if options.ReportResolvedReference != nil {
*options.ReportResolvedReference = nil // The default outcome, if not specifically supported by the transport.
}
if err := c.dest.CommitWithOptions(ctx, private.CommitOptions{
UnparsedToplevel: c.unparsedToplevel,
UnparsedToplevel: c.unparsedToplevel,
ReportResolvedReference: options.ReportResolvedReference,
}); err != nil {
return nil, fmt.Errorf("committing the finished image: %w", err)
}
Expand Down
5 changes: 5 additions & 0 deletions internal/private/private.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,11 @@ type CommitOptions struct {
// if PutManifest was only called for the single-arch image with instanceDigest == nil), primarily to allow lookups by the
// original manifest list digest, if desired.
UnparsedToplevel types.UnparsedImage
// ReportResolvedReference, if set, asks the transport to store a “resolved” (more detailed) reference to the created image
// into the value this option points to.
// What “resolved” means is transport-specific.
// Transports which don’t support reporting resolved references can ignore the field; the generic copy code writes "nil" into the value.
ReportResolvedReference *types.ImageReference
}

// ImageSourceChunk is a portion of a blob.
Expand Down
7 changes: 7 additions & 0 deletions storage/storage_dest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1343,6 +1343,13 @@ func (s *storageImageDestination) CommitWithOptions(ctx context.Context, options
}
logrus.Debugf("added name %q to image %q", name, img.ID)
}
if options.ReportResolvedReference != nil {
resolved, err := newReference(s.imageRef.transport, s.imageRef.named, intendedID)
if err != nil {
return fmt.Errorf("creating a resolved reference for (%s, %s): %w", s.imageRef.StringWithinTransport(), intendedID, err)
}
*options.ReportResolvedReference = resolved
}

commitSucceeded = true
return nil
Expand Down