simplifying the error handling on manifest delete/remove command

Signed-off-by: Juan Bustamante <jbustamante@vmware.com>

internal/commands/commands.go

@@ -36,8 +36,8 @@ type PackClient interface {
 	CreateManifest(ctx context.Context, opts client.CreateManifestOptions) error
 	AnnotateManifest(ctx context.Context, opts client.ManifestAnnotateOptions) error
 	AddManifest(ctx context.Context, opts client.ManifestAddOptions) error
-	DeleteManifest(ctx context.Context, name []string) []error
-	RemoveManifest(ctx context.Context, name string, images []string) []error
+	DeleteManifest(name []string) error
+	RemoveManifest(name string, images []string) error
 	PushManifest(client.PushManifestOptions) error
 	InspectManifest(string) error
 }

internal/commands/manifest_remove.go

@@ -1,7 +1,6 @@
 package commands
 
 import (
-	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 
 	"github.com/buildpacks/pack/pkg/logging"
@@ -15,33 +14,13 @@ func ManifestDelete(logger logging.Logger, pack PackClient) *cobra.Command {
 		Short:   "Remove one or more manifest lists from local storage",
 		Example: `pack manifest remove my-image-index`,
 		RunE: logError(logger, func(cmd *cobra.Command, args []string) error {
-			return NewErrors(pack.DeleteManifest(cmd.Context(), args)).Error()
+			if err := pack.DeleteManifest(args); err != nil {
+				return err
+			}
+			return nil
 		}),
 	}
 
 	AddHelpFlag(cmd, "remove")
 	return cmd
 }
-
-type Errors struct {
-	errs []error
-}
-
-func NewErrors(errs []error) Errors {
-	return Errors{
-		errs: errs,
-	}
-}
-
-func (e Errors) Error() error {
-	var errMsg string
-	if len(e.errs) == 0 {
-		return nil
-	}
-
-	for _, err := range e.errs {
-		errMsg += err.Error()
-	}
-
-	return errors.New(errMsg)
-}

internal/commands/manifest_remove_test.go

@@ -50,7 +50,6 @@ func testManifestDeleteCommand(t *testing.T, when spec.G, it spec.S) {
 			when("no extra flags are provided", func() {
 				it.Before(func() {
 					mockClient.EXPECT().DeleteManifest(
-						gomock.Any(),
 						gomock.Eq([]string{indexRepoName}),
 					).Return(nil)
 				})
@@ -72,9 +71,8 @@ func testManifestDeleteCommand(t *testing.T, when spec.G, it spec.S) {
 		when("index does not exist", func() {
 			it.Before(func() {
 				mockClient.EXPECT().DeleteManifest(
-					gomock.Any(),
 					gomock.Eq([]string{"some-none-existent-index"}),
-				).Return([]error{errors.New("image index doesn't exists")})
+				).Return(errors.New("image index doesn't exists"))
 			})
 
 			it("should return an error", func() {

internal/commands/manifest_rm.go

@@ -17,7 +17,10 @@ func ManifestRemove(logger logging.Logger, pack PackClient) *cobra.Command {
 Users must pass the digest of the image in order to delete it from the index.
 To discard __all__ the images in an index and the index itself, use 'manifest delete'.`,
 		RunE: logError(logger, func(cmd *cobra.Command, args []string) error {
-			return NewErrors(pack.RemoveManifest(cmd.Context(), args[0], args[1:])).Error()
+			if err := pack.RemoveManifest(args[0], args[1:]); err != nil {
+				return err
+			}
+			return nil
 		}),
 	}
 

internal/commands/manifest_rm_test.go

@@ -49,7 +49,6 @@ func testManifestRemoveCommand(t *testing.T, when spec.G, it spec.S) {
 			when("no extra flags are provided", func() {
 				it.Before(func() {
 					mockClient.EXPECT().RemoveManifest(
-						gomock.Any(),
 						gomock.Eq(indexRepoName),
 						gomock.Eq([]string{"some-image"}),
 					).Return(nil)
@@ -72,10 +71,9 @@ func testManifestRemoveCommand(t *testing.T, when spec.G, it spec.S) {
 		when("index does not exist", func() {
 			it.Before(func() {
 				mockClient.EXPECT().RemoveManifest(
-					gomock.Any(),
 					gomock.Eq(indexRepoName),
 					gomock.Eq([]string{"some-image"}),
-				).Return([]error{errors.New("image index doesn't exists")})
+				).Return(errors.New("image index doesn't exists"))
 			})
 			it("should return an error", func() {
 				command.SetArgs([]string{indexRepoName, "some-image"})

pkg/client/manifest_remove.go

@@ -1,25 +1,24 @@
 package client
 
-import (
-	"context"
-)
+import "errors"
 
 // DeleteManifest implements commands.PackClient.
-func (c *Client) DeleteManifest(ctx context.Context, names []string) (errs []error) {
+func (c *Client) DeleteManifest(names []string) error {
+	var allErrors error
 	for _, name := range names {
 		imgIndex, err := c.indexFactory.LoadIndex(name)
 		if err != nil {
-			errs = append(errs, err)
+			allErrors = errors.Join(allErrors, err)
 			continue
 		}
 
 		if err := imgIndex.DeleteDir(); err != nil {
-			errs = append(errs, err)
+			allErrors = errors.Join(allErrors, err)
 		}
 	}
 
-	if len(errs) == 0 {
+	if allErrors == nil {
 		c.logger.Info("Successfully deleted manifest list(s) from local storage")
 	}
-	return errs
+	return allErrors
 }

pkg/client/manifest_remove_test.go

@@ -2,7 +2,6 @@ package client
 
 import (
 	"bytes"
-	"context"
 	"os"
 	"path/filepath"
 	"testing"
@@ -71,8 +70,8 @@ func testDeleteManifest(t *testing.T, when spec.G, it spec.S) {
 				mockIndexFactory.EXPECT().LoadIndex(gomock.Any(), gomock.Any()).Return(nil, errors.New("index not found locally"))
 			})
 			it("should return an error when index is already deleted", func() {
-				errs := subject.DeleteManifest(context.TODO(), []string{"pack/none-existent-index"})
-				h.AssertNotEq(t, len(errs), 0)
+				err = subject.DeleteManifest([]string{"pack/none-existent-index"})
+				h.AssertNotNil(t, err)
 			})
 		})
 
@@ -90,8 +89,8 @@ func testDeleteManifest(t *testing.T, when spec.G, it spec.S) {
 			})
 
 			it("should delete local index", func() {
-				errs := subject.DeleteManifest(context.TODO(), []string{indexRepoName})
-				h.AssertEq(t, len(errs), 0)
+				err = subject.DeleteManifest([]string{indexRepoName})
+				h.AssertNil(t, err)
 				h.AssertContains(t, out.String(), "Successfully deleted manifest list(s) from local storage")
 				h.AssertPathDoesNotExists(t, indexPath)
 			})

pkg/client/manifest_rm.go

@@ -1,37 +1,39 @@
 package client
 
 import (
-	"context"
+	"errors"
 	"fmt"
 
 	gccrName "github.com/google/go-containerregistry/pkg/name"
 )
 
 // RemoveManifest implements commands.PackClient.
-func (c *Client) RemoveManifest(ctx context.Context, name string, images []string) (errs []error) {
+func (c *Client) RemoveManifest(name string, images []string) error {
+	var allErrors error
+
 	imgIndex, err := c.indexFactory.LoadIndex(name)
 	if err != nil {
-		return append(errs, err)
+		return err
 	}
 
 	for _, image := range images {
 		ref, err := gccrName.NewDigest(image, gccrName.WeakValidation, gccrName.Insecure)
 		if err != nil {
-			errs = append(errs, fmt.Errorf("invalid instance '%s': %w", image, err))
+			allErrors = errors.Join(allErrors, fmt.Errorf("invalid instance '%s': %w", image, err))
 		}
 
 		if err = imgIndex.RemoveManifest(ref); err != nil {
-			errs = append(errs, err)
+			allErrors = errors.Join(allErrors, err)
 		}
 
 		if err = imgIndex.SaveDir(); err != nil {
-			errs = append(errs, err)
+			allErrors = errors.Join(allErrors, err)
 		}
 	}
 
-	if len(errs) == 0 {
+	if allErrors == nil {
 		c.logger.Infof("Successfully removed image(s) from index: '%s'", name)
 	}
 
-	return errs
+	return allErrors
 }

pkg/client/manifest_rm_test.go

@@ -2,7 +2,6 @@ package client
 
 import (
 	"bytes"
-	"context"
 	"os"
 	"path/filepath"
 	"testing"
@@ -80,8 +79,8 @@ func testRemoveManifest(t *testing.T, when spec.G, it spec.S) {
 			})
 
 			it("should remove local index", func() {
-				errs := subject.RemoveManifest(context.TODO(), indexRepoName, []string{digest.Name()})
-				h.AssertEq(t, len(errs), 0)
+				err = subject.RemoveManifest(indexRepoName, []string{digest.Name()})
+				h.AssertNil(t, err)
 
 				// We expect one manifest after removing one of them
 				index := h.ReadIndexManifest(t, indexPath)