Merge pull request #2365 from mtrmac/multierr

Add a helper for formatting multiple errors

docker/docker_client.go

@@ -18,6 +18,7 @@ import (
 
 	"github.com/containers/image/v5/docker/reference"
 	"github.com/containers/image/v5/internal/iolimits"
+	"github.com/containers/image/v5/internal/multierr"
 	"github.com/containers/image/v5/internal/set"
 	"github.com/containers/image/v5/internal/useragent"
 	"github.com/containers/image/v5/manifest"
@@ -1011,11 +1012,7 @@ func (c *dockerClient) getExternalBlob(ctx context.Context, urls []string) (io.R
 	if remoteErrors == nil {
 		return nil, 0, nil // fallback to non-external blob
 	}
-	err := fmt.Errorf("failed fetching external blob from all urls: %w", remoteErrors[0])
-	for _, e := range remoteErrors[1:] {
-		err = fmt.Errorf("%s, %w", err, e)
-	}
-	return nil, 0, err
+	return nil, 0, fmt.Errorf("failed fetching external blob from all urls: %w", multierr.Format("", ", ", "", remoteErrors))
 }
 
 func getBlobSize(resp *http.Response) int64 {

go.mod

@@ -17,7 +17,6 @@ require (
 	github.com/docker/go-connections v0.5.0
 	github.com/go-openapi/strfmt v0.23.0
 	github.com/go-openapi/swag v0.23.0
-	github.com/hashicorp/go-multierror v1.1.1
 	github.com/hashicorp/go-retryablehttp v0.7.5
 	github.com/klauspost/compress v1.17.7
 	github.com/klauspost/pgzip v1.2.6
@@ -91,6 +90,7 @@ require (
 	github.com/hashicorp/errwrap v1.1.0 // indirect
 	github.com/hashicorp/go-cleanhttp v0.5.2 // indirect
 	github.com/hashicorp/go-hclog v1.2.0 // indirect
+	github.com/hashicorp/go-multierror v1.1.1 // indirect
 	github.com/josharian/intern v1.0.0 // indirect
 	github.com/json-iterator/go v1.1.12 // indirect
 	github.com/letsencrypt/boulder v0.0.0-20230907030200-6d76a0f91e1e // indirect

internal/multierr/multierr.go

@@ -0,0 +1,34 @@
+package multierr
+
+import (
+	"fmt"
+	"strings"
+)
+
+// Format creates an error value from the input array (which should not be empty)
+// If the input contains a single error value, it is returned as is.
+// If there are multiple, they are formatted as a multi-error (with Unwrap() []error) with the provided initial, separator, and ending strings.
+//
+// Typical usage:
+//
+//	var errs []error
+//	// …
+//	errs = append(errs, …)
+//	// …
+//	if errs != nil { return multierr.Format("Failures doing $FOO", "\n* ", "", errs)}
+func Format(first, middle, last string, errs []error) error {
+	switch len(errs) {
+	case 0:
+		return fmt.Errorf("internal error: multierr.Format called with 0 errors")
+	case 1:
+		return errs[0]
+	default:
+		// We have to do this — and this function only really exists — because fmt.Errorf(format, errs...) is invalid:
+		// []error is not a valid parameter to a function expecting []any
+		anyErrs := make([]any, 0, len(errs))
+		for _, e := range errs {
+			anyErrs = append(anyErrs, e)
+		}
+		return fmt.Errorf(first+"%w"+strings.Repeat(middle+"%w", len(errs)-1)+last, anyErrs...)
+	}
+}

internal/multierr/multierr_test.go

@@ -0,0 +1,39 @@
+package multierr
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+type errA struct{}
+
+func (errA) Error() string { return "A" }
+
+func TestFormat(t *testing.T) {
+	errB := errors.New("B")
+
+	// Single-item Format preserves the error type
+	res := Format("[", ",", "]", []error{errA{}})
+	assert.Equal(t, "A", res.Error())
+	var aTarget errA
+	assert.ErrorAs(t, res, &aTarget)
+
+	// Single-item Format preserves the error identity
+	res = Format("[", ",", "]", []error{errB})
+	assert.Equal(t, "B", res.Error())
+	assert.ErrorIs(t, res, errB)
+
+	// Multi-item Format preserves both
+	res = Format("[", ",", "]", []error{errA{}, errB})
+	assert.Equal(t, "[A,B]", res.Error())
+	assert.ErrorAs(t, res, &aTarget)
+	assert.ErrorIs(t, res, errB)
+
+	// This is invalid, but make sure we don’t misleadingly suceeed
+	res = Format("[", ",", "]", []error{})
+	assert.Error(t, res)
+	res = Format("[", ",", "]", nil)
+	assert.Error(t, res)
+}

openshift/openshift-copies.go

@@ -18,6 +18,7 @@ import (
 	"time"
 
 	"dario.cat/mergo"
+	"github.com/containers/image/v5/internal/multierr"
 	"github.com/containers/storage/pkg/homedir"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/exp/slices"
@@ -459,12 +460,6 @@ func (config *directClientConfig) getCluster() clientcmdCluster {
 	return mergedClusterInfo
 }
 
-// aggregateErr is a modified copy of k8s.io/apimachinery/pkg/util/errors.aggregate.
-// This helper implements the error and Errors interfaces.  Keeping it private
-// prevents people from making an aggregate of 0 errors, which is not
-// an error, but does satisfy the error interface.
-type aggregateErr []error
-
 // newAggregate is a modified copy of k8s.io/apimachinery/pkg/util/errors.NewAggregate.
 // NewAggregate converts a slice of errors into an Aggregate interface, which
 // is itself an implementation of the error interface.  If the slice is empty,
@@ -485,29 +480,9 @@ func newAggregate(errlist []error) error {
 	if len(errs) == 0 {
 		return nil
 	}
-	return aggregateErr(errs)
+	return multierr.Format("[", ", ", "]", errs)
 }
 
-// Error is a modified copy of k8s.io/apimachinery/pkg/util/errors.aggregate.Error.
-// Error is part of the error interface.
-func (agg aggregateErr) Error() string {
-	if len(agg) == 0 {
-		// This should never happen, really.
-		return ""
-	}
-	if len(agg) == 1 {
-		return agg[0].Error()
-	}
-	result := fmt.Sprintf("[%s", agg[0].Error())
-	for i := 1; i < len(agg); i++ {
-		result += fmt.Sprintf(", %s", agg[i].Error())
-	}
-	result += "]"
-	return result
-}
-
-// REMOVED: aggregateErr.Errors
-
 // errConfigurationInvalid is a modified? copy of k8s.io/kubernetes/pkg/client/unversioned/clientcmd.errConfigurationInvalid.
 // errConfigurationInvalid is a set of errors indicating the configuration is invalid.
 type errConfigurationInvalid []error

pkg/docker/config/config.go

@@ -13,14 +13,14 @@ import (
 	"strings"
 
 	"github.com/containers/image/v5/docker/reference"
+	"github.com/containers/image/v5/internal/multierr"
 	"github.com/containers/image/v5/internal/set"
 	"github.com/containers/image/v5/pkg/sysregistriesv2"
 	"github.com/containers/image/v5/types"
 	"github.com/containers/storage/pkg/homedir"
 	"github.com/containers/storage/pkg/ioutils"
 	helperclient "github.com/docker/docker-credential-helpers/client"
 	"github.com/docker/docker-credential-helpers/credentials"
-	"github.com/hashicorp/go-multierror"
 	"github.com/sirupsen/logrus"
 )
 
@@ -231,7 +231,7 @@ func getCredentialsWithHomeDir(sys *types.SystemContext, key, homeDir string) (t
 		return types.DockerAuthConfig{}, err
 	}
 
-	var multiErr error
+	var multiErr []error
 	for _, helper := range helpers {
 		var (
 			creds          types.DockerAuthConfig
@@ -253,7 +253,7 @@ func getCredentialsWithHomeDir(sys *types.SystemContext, key, homeDir string) (t
 		}
 		if err != nil {
 			logrus.Debugf("Error looking up credentials for %s in credential helper %s: %v", helperKey, helper, err)
-			multiErr = multierror.Append(multiErr, err)
+			multiErr = append(multiErr, err)
 			continue
 		}
 		if creds != (types.DockerAuthConfig{}) {
@@ -266,7 +266,7 @@ func getCredentialsWithHomeDir(sys *types.SystemContext, key, homeDir string) (t
 		}
 	}
 	if multiErr != nil {
-		return types.DockerAuthConfig{}, multiErr
+		return types.DockerAuthConfig{}, multierr.Format("errors looking up credentials:\n\t* ", "\nt* ", "\n", multiErr)
 	}
 
 	logrus.Debugf("No credentials for %s found", key)
@@ -313,7 +313,7 @@ func SetCredentials(sys *types.SystemContext, key, username, password string) (s
 	}
 
 	// Make sure to collect all errors.
-	var multiErr error
+	var multiErr []error
 	for _, helper := range helpers {
 		var desc string
 		var err error
@@ -345,14 +345,14 @@ func SetCredentials(sys *types.SystemContext, key, username, password string) (s
 			}
 		}
 		if err != nil {
-			multiErr = multierror.Append(multiErr, err)
+			multiErr = append(multiErr, err)
 			logrus.Debugf("Error storing credentials for %s in credential helper %s: %v", key, helper, err)
 			continue
 		}
 		logrus.Debugf("Stored credentials for %s in credential helper %s", key, helper)
 		return desc, nil
 	}
-	return "", multiErr
+	return "", multierr.Format("Errors storing credentials\n\t* ", "\n\t* ", "\n", multiErr)
 }
 
 func unsupportedNamespaceErr(helper string) error {
@@ -376,53 +376,56 @@ func RemoveAuthentication(sys *types.SystemContext, key string) error {
 		return err
 	}
 
-	var multiErr error
 	isLoggedIn := false
 
-	removeFromCredHelper := func(helper string) {
+	removeFromCredHelper := func(helper string) error {
 		if isNamespaced {
 			logrus.Debugf("Not removing credentials because namespaced keys are not supported for the credential helper: %s", helper)
-			return
+			return nil
 		}
 		err := deleteCredsFromCredHelper(helper, key)
 		if err == nil {
 			logrus.Debugf("Credentials for %q were deleted from credential helper %s", key, helper)
 			isLoggedIn = true
-			return
+			return nil
 		}
 		if credentials.IsErrCredentialsNotFoundMessage(err.Error()) {
 			logrus.Debugf("Not logged in to %s with credential helper %s", key, helper)
-			return
+			return nil
 		}
-		multiErr = multierror.Append(multiErr, fmt.Errorf("removing credentials for %s from credential helper %s: %w", key, helper, err))
+		return fmt.Errorf("removing credentials for %s from credential helper %s: %w", key, helper, err)
 	}
 
+	var multiErr []error
 	for _, helper := range helpers {
 		var err error
 		switch helper {
 		// Special-case the built-in helper for auth files.
 		case sysregistriesv2.AuthenticationFileHelper:
 			_, err = jsonEditor(sys, func(fileContents *dockerConfigFile) (bool, string, error) {
+				var helperErr error
 				if innerHelper, exists := fileContents.CredHelpers[key]; exists {
-					removeFromCredHelper(innerHelper)
+					helperErr = removeFromCredHelper(innerHelper)
 				}
 				if _, ok := fileContents.AuthConfigs[key]; ok {
 					isLoggedIn = true
 					delete(fileContents.AuthConfigs, key)
 				}
-				return true, "", multiErr
+				return true, "", helperErr
 			})
 			if err != nil {
-				multiErr = multierror.Append(multiErr, err)
+				multiErr = append(multiErr, err)
 			}
 		// External helpers.
 		default:
-			removeFromCredHelper(helper)
+			if err := removeFromCredHelper(helper); err != nil {
+				multiErr = append(multiErr, err)
+			}
 		}
 	}
 
 	if multiErr != nil {
-		return multiErr
+		return multierr.Format("errors removing credentials\n\t* ", "\n\t*", "\n", multiErr)
 	}
 	if !isLoggedIn {
 		return ErrNotLoggedIn
@@ -439,7 +442,7 @@ func RemoveAllAuthentication(sys *types.SystemContext) error {
 		return err
 	}
 
-	var multiErr error
+	var multiErr []error
 	for _, helper := range helpers {
 		var err error
 		switch helper {
@@ -479,13 +482,16 @@ func RemoveAllAuthentication(sys *types.SystemContext) error {
 		}
 		if err != nil {
 			logrus.Debugf("Error removing credentials from credential helper %s: %v", helper, err)
-			multiErr = multierror.Append(multiErr, err)
+			multiErr = append(multiErr, err)
 			continue
 		}
 		logrus.Debugf("All credentials removed from credential helper %s", helper)
 	}
 
-	return multiErr
+	if multiErr != nil {
+		return multierr.Format("errors removing all credentials:\n\t* ", "\n\t* ", "\n", multiErr)
+	}
+	return nil
 }
 
 // prepareForEdit processes sys and key (if keyRelevant) to return:

pkg/shortnames/shortnames.go

@@ -7,6 +7,7 @@ import (
 	"strings"
 
 	"github.com/containers/image/v5/docker/reference"
+	"github.com/containers/image/v5/internal/multierr"
 	"github.com/containers/image/v5/pkg/sysregistriesv2"
 	"github.com/containers/image/v5/types"
 	"github.com/manifoldco/promptui"
@@ -169,26 +170,17 @@ func (r *Resolved) Description() string {
 // Note that nil is returned if len(pullErrors) == 0.  Otherwise, the amount of
 // pull errors must equal the amount of pull candidates.
 func (r *Resolved) FormatPullErrors(pullErrors []error) error {
-	if len(pullErrors) > 0 && len(pullErrors) != len(r.PullCandidates) {
+	if len(pullErrors) == 0 {
+		return nil
+	}
+
+	if len(pullErrors) != len(r.PullCandidates) {
 		pullErrors = append(slices.Clone(pullErrors),
 			fmt.Errorf("internal error: expected %d instead of %d errors for %d pull candidates",
 				len(r.PullCandidates), len(pullErrors), len(r.PullCandidates)))
 	}
 
-	switch len(pullErrors) {
-	case 0:
-		return nil
-	case 1:
-		return pullErrors[0]
-	default:
-		var sb strings.Builder
-		sb.WriteString(fmt.Sprintf("%d errors occurred while pulling:", len(pullErrors)))
-		for _, e := range pullErrors {
-			sb.WriteString("\n * ")
-			sb.WriteString(e.Error())
-		}
-		return errors.New(sb.String())
-	}
+	return multierr.Format(fmt.Sprintf("%d errors occurred while pulling:\n * ", len(pullErrors)), "\n * ", "", pullErrors)
 }
 
 // PullCandidate is a resolved name.  Once the Value has been used

pkg/sysregistriesv2/shortnames.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/BurntSushi/toml"
 	"github.com/containers/image/v5/docker/reference"
+	"github.com/containers/image/v5/internal/multierr"
 	"github.com/containers/image/v5/internal/rootless"
 	"github.com/containers/image/v5/types"
 	"github.com/containers/storage/pkg/homedir"
@@ -297,11 +298,7 @@ func newShortNameAliasCache(path string, conf *shortNameAliasConf) (*shortNameAl
 		}
 	}
 	if len(errs) > 0 {
-		err := errs[0]
-		for i := 1; i < len(errs); i++ {
-			err = fmt.Errorf("%v\n: %w", errs[i], err)
-		}
-		return nil, err
+		return nil, multierr.Format("", "\n", "", errs)
 	}
 	return &res, nil
 }