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

Export copier and create interface #2161

Closed
wants to merge 1 commit into from
Closed

Export copier and create interface #2161

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
12 changes: 6 additions & 6 deletions libimage/copier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,10 +206,10 @@ func getDockerAuthConfig(name, passwd, creds, idToken string) (*types.DockerAuth
}
}

// newCopier creates a copier. Note that fields in options *may* overwrite the
// NewCopier creates a copier. Note that fields in options *may* overwrite the
// counterparts of the specified system context. Please make sure to call
// `(*copier).close()`.
func (r *Runtime) newCopier(options *CopyOptions) (*copier, error) {
func (r *Runtime) NewCopier(options *CopyOptions) (*copier, error) {
c := copier{extendTimeoutSocket: options.extendTimeoutSocket}
c.systemContext = r.systemContextCopy()

Expand Down Expand Up @@ -325,14 +325,14 @@ func (r *Runtime) newCopier(options *CopyOptions) (*copier, error) {
return &c, nil
}

// close open resources.
func (c *copier) close() error {
// Close open resources.
func (c *copier) Close() error {
return c.policyContext.Destroy()
}

// copy the source to the destination. Returns the bytes of the copied
// Copy the source to the destination. Returns the bytes of the copied
// manifest which may be used for digest computation.
func (c *copier) copy(ctx context.Context, source, destination types.ImageReference) ([]byte, error) {
func (c *copier) Copy(ctx context.Context, source, destination types.ImageReference) ([]byte, error) {
logrus.Debugf("Copying source image %s to destination image %s", source.StringWithinTransport(), destination.StringWithinTransport())

// Avoid running out of time when running inside a systemd unit by
Expand Down
6 changes: 3 additions & 3 deletions libimage/import.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,13 +104,13 @@ func (r *Runtime) Import(ctx context.Context, path string, options *ImportOption
return "", err
}

c, err := r.newCopier(&options.CopyOptions)
c, err := r.NewCopier(&options.CopyOptions)
if err != nil {
return "", err
}
defer c.close()
defer c.Close()

if _, err := c.copy(ctx, srcRef, destRef); err != nil {
if _, err := c.Copy(ctx, srcRef, destRef); err != nil {
return "", err
}

Expand Down
10 changes: 8 additions & 2 deletions libimage/manifest_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ import (
// NOTE: the abstractions and APIs here are a first step to further merge
// `libimage/manifests` into `libimage`.

// Copier is a simple interface declaration for the existing copier
type Copier interface {
Copy(ctx context.Context, source, destination types.ImageReference) ([]byte, error)
Close() error
}
Comment on lines +29 to +33
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to be used anywhere, also in the wrong place? If this is needed it should be defined in copier.go.

And if you have to use the type in order to pass *copier around on the caller side then you should add a a actual check that the interface is satisfied.
Something like var _ Copier = (*copier)(nil) should do that AFAICT.

Copy link
Contributor

@mtrmac mtrmac Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As a general matter of Go API design, I now think libraries exporting objects should not be publishing interface types if at all possible.

(Sure, export an interface type if the caller needs to provide an object, to define what the object is expected to satisfy.)

The reason is that adding a method to an interface breaks all other implementations, so there is really no way to have an API-stable exported interface for an evolving feature set; the only API-stable way to use a such a Go interface which expands over time is to only ever have that one implementation — and at that point there doesn’t need to be an interface at all.

Instead:

  • For the purpose of unit testing / mocking: define a private interface in the test subpackage of the consumer of the object
  • For the purpose of limiting the method set of the exported type (e.g. if the implementation struct must have public methods to conform to some other interface): instead, export a wrapper struct that only has the truly-public methods, and make the actual implementation a private structure not visible to external callers. (An extreme variant of this is going one step further in https://github.com/containers/image/blob/main/internal/signer/signer.go , where the only publicly-visible method is Close; and all other methods can be called internally from c/image. That’s only necessary if the module-private methods need to be called from multiple subpackages.)

Also, if NewCopier is public, the returned type probably should also be public, to allow callers to e.g. store the returned value in a struct field; if the returned type is unnamed, it can only be used via private local := variables — or using an interface type.


// ErrNotAManifestList indicates that an image was found in the local
// containers storage but it is not a manifest list as requested.
var ErrNotAManifestList = errors.New("image is not a manifest list")
Expand Down Expand Up @@ -632,11 +638,11 @@ func (m *ManifestList) Push(ctx context.Context, destination string, options *Ma
// NOTE: we're using the logic in copier to create a proper
// types.SystemContext. This prevents us from having an error prone
// code duplicate here.
copier, err := m.image.runtime.newCopier(&options.CopyOptions)
copier, err := m.image.runtime.NewCopier(&options.CopyOptions)
if err != nil {
return "", err
}
defer copier.close()
defer copier.Close()

pushOptions := manifests.PushOptions{
AddCompression: options.AddCompression,
Expand Down
18 changes: 9 additions & 9 deletions libimage/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,11 +231,11 @@ func nameFromAnnotations(annotations map[string]string) string {
// copyFromDefault is the default copier for a number of transports. Other
// transports require some specific dancing, sometimes Yoga.
func (r *Runtime) copyFromDefault(ctx context.Context, ref types.ImageReference, options *CopyOptions) ([]string, error) {
c, err := r.newCopier(options)
c, err := r.NewCopier(options)
if err != nil {
return nil, err
}
defer c.close()
defer c.Close()

// Figure out a name for the storage destination.
var storageName, imageName string
Expand Down Expand Up @@ -321,7 +321,7 @@ func (r *Runtime) copyFromDefault(ctx context.Context, ref types.ImageReference,
return nil, fmt.Errorf("parsing %q: %w", storageName, err)
}

_, err = c.copy(ctx, ref, destRef)
_, err = c.Copy(ctx, ref, destRef)
return []string{imageName}, err
}

Expand Down Expand Up @@ -387,11 +387,11 @@ func (r *Runtime) copyFromDockerArchive(ctx context.Context, ref types.ImageRefe

// copyFromDockerArchiveReaderReference copies the specified readerRef from reader.
func (r *Runtime) copyFromDockerArchiveReaderReference(ctx context.Context, reader *dockerArchiveTransport.Reader, readerRef types.ImageReference, options *CopyOptions) ([]string, error) {
c, err := r.newCopier(options)
c, err := r.NewCopier(options)
if err != nil {
return nil, err
}
defer c.close()
defer c.Close()

// Get a slice of storage references we can copy.
references, destNames, err := r.storageReferencesReferencesFromArchiveReader(ctx, readerRef, reader)
Expand All @@ -401,7 +401,7 @@ func (r *Runtime) copyFromDockerArchiveReaderReference(ctx context.Context, read

// Now copy all of the images. Use readerRef for performance.
for _, destRef := range references {
if _, err := c.copy(ctx, readerRef, destRef); err != nil {
if _, err := c.Copy(ctx, readerRef, destRef); err != nil {
return nil, err
}
}
Expand Down Expand Up @@ -636,11 +636,11 @@ func (r *Runtime) copySingleImageFromRegistry(ctx context.Context, imageName str
if socketPath, ok := os.LookupEnv("NOTIFY_SOCKET"); ok {
options.extendTimeoutSocket = socketPath
}
c, err := r.newCopier(&options.CopyOptions)
c, err := r.NewCopier(&options.CopyOptions)
if err != nil {
return nil, err
}
defer c.close()
defer c.Close()

var pullErrors []error
for _, candidate := range resolved.PullCandidates {
Expand Down Expand Up @@ -678,7 +678,7 @@ func (r *Runtime) copySingleImageFromRegistry(ctx context.Context, imageName str
}
}
var manifestBytes []byte
if manifestBytes, err = c.copy(ctx, srcRef, destRef); err != nil {
if manifestBytes, err = c.Copy(ctx, srcRef, destRef); err != nil {
logrus.Debugf("Error pulling candidate %s: %v", candidateString, err)
pullErrors = append(pullErrors, err)
continue
Expand Down
6 changes: 3 additions & 3 deletions libimage/push.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,12 @@ func (r *Runtime) Push(ctx context.Context, source, destination string, options
}
}

c, err := r.newCopier(&options.CopyOptions)
c, err := r.NewCopier(&options.CopyOptions)
if err != nil {
return nil, err
}

defer c.close()
defer c.Close()

return c.copy(ctx, srcRef, destRef)
return c.Copy(ctx, srcRef, destRef)
}
12 changes: 6 additions & 6 deletions libimage/save.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,13 +119,13 @@ func (r *Runtime) saveSingleImage(ctx context.Context, name, format, path string
return err
}

c, err := r.newCopier(&options.CopyOptions)
c, err := r.NewCopier(&options.CopyOptions)
if err != nil {
return err
}
defer c.close()
defer c.Close()

_, err = c.copy(ctx, srcRef, destRef)
_, err = c.Copy(ctx, srcRef, destRef)
return err
}

Expand Down Expand Up @@ -204,11 +204,11 @@ func (r *Runtime) saveDockerArchive(ctx context.Context, names []string, path st
copyOpts := options.CopyOptions
copyOpts.dockerArchiveAdditionalTags = local.tags

c, err := r.newCopier(&copyOpts)
c, err := r.NewCopier(&copyOpts)
if err != nil {
return err
}
defer c.close()
defer c.Close()

destRef, err := writer.NewReference(nil)
if err != nil {
Expand All @@ -220,7 +220,7 @@ func (r *Runtime) saveDockerArchive(ctx context.Context, names []string, path st
return err
}

if _, err := c.copy(ctx, srcRef, destRef); err != nil {
if _, err := c.Copy(ctx, srcRef, destRef); err != nil {
return err
}
}
Expand Down