diff --git a/cmd/skopeo/copy.go b/cmd/skopeo/copy.go index 84298fbd..80742600 100644 --- a/cmd/skopeo/copy.go +++ b/cmd/skopeo/copy.go @@ -130,7 +130,7 @@ func (opts *copyOptions) run(args []string, stdout io.Writer) (retErr error) { } defer func() { if err := policyContext.Destroy(); err != nil { - retErr = fmt.Errorf("(error tearing down policy context: %v): %w", err, retErr) + retErr = noteCloseFailure(retErr, "error tearing down policy context", err) } }() diff --git a/cmd/skopeo/inspect.go b/cmd/skopeo/inspect.go index e3c6e4d2..e5bee9a4 100644 --- a/cmd/skopeo/inspect.go +++ b/cmd/skopeo/inspect.go @@ -105,7 +105,7 @@ func (opts *inspectOptions) run(args []string, stdout io.Writer) (retErr error) defer func() { if err := src.Close(); err != nil { - retErr = errors.Wrapf(retErr, fmt.Sprintf("(could not close image: %v) ", err)) + retErr = noteCloseFailure(retErr, "could not close image", err) } }() diff --git a/cmd/skopeo/layers.go b/cmd/skopeo/layers.go index 484cfe3b..1f878869 100644 --- a/cmd/skopeo/layers.go +++ b/cmd/skopeo/layers.go @@ -86,7 +86,7 @@ func (opts *layersOptions) run(args []string, stdout io.Writer) (retErr error) { } defer func() { if err := src.Close(); err != nil { - retErr = errors.Wrapf(retErr, " (close error: %v)", err) + retErr = noteCloseFailure(retErr, "close error", err) } }() @@ -136,7 +136,7 @@ func (opts *layersOptions) run(args []string, stdout io.Writer) (retErr error) { defer func() { if err := dest.Close(); err != nil { - retErr = errors.Wrapf(retErr, " (close error: %v)", err) + retErr = noteCloseFailure(retErr, "close error", err) } }() diff --git a/cmd/skopeo/sync.go b/cmd/skopeo/sync.go index e57e2828..0772ba43 100644 --- a/cmd/skopeo/sync.go +++ b/cmd/skopeo/sync.go @@ -514,7 +514,7 @@ func (opts *syncOptions) run(args []string, stdout io.Writer) (retErr error) { } defer func() { if err := policyContext.Destroy(); err != nil { - retErr = fmt.Errorf("(error tearing down policy context: %v): %w", err, retErr) + retErr = noteCloseFailure(retErr, "error tearing down policy context", err) } }() diff --git a/cmd/skopeo/utils.go b/cmd/skopeo/utils.go index 2e7b65b6..17b6c990 100644 --- a/cmd/skopeo/utils.go +++ b/cmd/skopeo/utils.go @@ -25,6 +25,27 @@ type errorShouldDisplayUsage struct { error } +// noteCloseFailure returns (possibly-nil) err modified to account for (non-nil) closeErr. +// The error for closeErr is annotated with description (which is not a format string) +// Typical usage: +// +// defer func() { +// if err := something.Close(); err != nil { +// returnedErr = noteCloseFailure(returnedErr, "closing something", err) +// } +// } +func noteCloseFailure(err error, description string, closeErr error) error { + // We don’t accept a Closer() and close it ourselves because signature.PolicyContext has .Destroy(), not .Close(). + // This also makes it harder for a caller to do + // defer noteCloseFailure(returnedErr, …) + // which doesn’t use the right value of returnedErr, and doesn’t update it. + if err == nil { + return fmt.Errorf("%s: %w", description, closeErr) + } + // In this case we prioritize the primary error for use with %w; closeErr is usually less relevant, or might be a consequence of the primary erorr. + return fmt.Errorf("%w (%s: %v)", err, description, closeErr) +} + // commandAction intermediates between the RunE interface and the real handler, // primarily to ensure that cobra.Command is not available to the handler, which in turn // makes sure that the cmd.Flags() etc. flag access functions are not used, diff --git a/cmd/skopeo/utils_test.go b/cmd/skopeo/utils_test.go index 4a3464d6..f5160b22 100644 --- a/cmd/skopeo/utils_test.go +++ b/cmd/skopeo/utils_test.go @@ -1,6 +1,7 @@ package main import ( + "errors" "testing" "github.com/containers/image/v5/manifest" @@ -12,6 +13,27 @@ import ( "github.com/stretchr/testify/require" ) +func TestNoteCloseFailure(t *testing.T) { + const description = "description" + + mainErr := errors.New("main") + closeErr := errors.New("closing") + + // Main success, closing failed + res := noteCloseFailure(nil, description, closeErr) + require.NotNil(t, res) + assert.Contains(t, res.Error(), description) + assert.Contains(t, res.Error(), closeErr.Error()) + + // Both main and closing failed + res = noteCloseFailure(mainErr, description, closeErr) + require.NotNil(t, res) + assert.Contains(t, res.Error(), mainErr.Error()) + assert.Contains(t, res.Error(), description) + assert.Contains(t, res.Error(), closeErr.Error()) + assert.ErrorIs(t, res, mainErr) +} + // fakeGlobalOptions creates globalOptions and sets it according to flags. func fakeGlobalOptions(t *testing.T, flags []string) (*globalOptions, *cobra.Command) { app, opts := createApp()