From 12cbadf58b17f7605d4520ba7e64cbe93091aaa4 Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Fri, 3 May 2024 15:08:17 -0500 Subject: [PATCH] simplifying the error handling on manifest delete/remove command Signed-off-by: Juan Bustamante --- internal/commands/commands.go | 4 +-- internal/commands/manifest_remove.go | 29 +++---------------- internal/commands/manifest_remove_test.go | 4 +-- internal/commands/manifest_rm.go | 5 +++- internal/commands/manifest_rm_test.go | 4 +-- .../commands/testmocks/mock_pack_client.go | 20 ++++++------- pkg/client/manifest_remove.go | 15 +++++----- pkg/client/manifest_remove_test.go | 9 +++--- pkg/client/manifest_rm.go | 18 +++++++----- pkg/client/manifest_rm_test.go | 5 ++-- 10 files changed, 45 insertions(+), 68 deletions(-) diff --git a/internal/commands/commands.go b/internal/commands/commands.go index fcc3bfaf6..734344dba 100644 --- a/internal/commands/commands.go +++ b/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 } diff --git a/internal/commands/manifest_remove.go b/internal/commands/manifest_remove.go index 34d0cf4cf..590bb84e3 100644 --- a/internal/commands/manifest_remove.go +++ b/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) -} diff --git a/internal/commands/manifest_remove_test.go b/internal/commands/manifest_remove_test.go index 0a33b7c7b..4b0bd61b5 100644 --- a/internal/commands/manifest_remove_test.go +++ b/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() { diff --git a/internal/commands/manifest_rm.go b/internal/commands/manifest_rm.go index 383f7660e..115d123a7 100644 --- a/internal/commands/manifest_rm.go +++ b/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 }), } diff --git a/internal/commands/manifest_rm_test.go b/internal/commands/manifest_rm_test.go index 9e4927c65..615f3daed 100644 --- a/internal/commands/manifest_rm_test.go +++ b/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"}) diff --git a/internal/commands/testmocks/mock_pack_client.go b/internal/commands/testmocks/mock_pack_client.go index 58d769935..981704c09 100644 --- a/internal/commands/testmocks/mock_pack_client.go +++ b/internal/commands/testmocks/mock_pack_client.go @@ -107,17 +107,17 @@ func (mr *MockPackClientMockRecorder) CreateManifest(arg0, arg1 interface{}) *go } // DeleteManifest mocks base method. -func (m *MockPackClient) DeleteManifest(arg0 context.Context, arg1 []string) []error { +func (m *MockPackClient) DeleteManifest(arg0 []string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteManifest", arg0, arg1) - ret0, _ := ret[0].([]error) + ret := m.ctrl.Call(m, "DeleteManifest", arg0) + ret0, _ := ret[0].(error) return ret0 } // DeleteManifest indicates an expected call of DeleteManifest. -func (mr *MockPackClientMockRecorder) DeleteManifest(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockPackClientMockRecorder) DeleteManifest(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteManifest", reflect.TypeOf((*MockPackClient)(nil).DeleteManifest), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteManifest", reflect.TypeOf((*MockPackClient)(nil).DeleteManifest), arg0) } // DownloadSBOM mocks base method. @@ -312,17 +312,17 @@ func (mr *MockPackClientMockRecorder) RegisterBuildpack(arg0, arg1 interface{}) } // RemoveManifest mocks base method. -func (m *MockPackClient) RemoveManifest(arg0 context.Context, arg1 string, arg2 []string) []error { +func (m *MockPackClient) RemoveManifest(arg0 string, arg1 []string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "RemoveManifest", arg0, arg1, arg2) - ret0, _ := ret[0].([]error) + ret := m.ctrl.Call(m, "RemoveManifest", arg0, arg1) + ret0, _ := ret[0].(error) return ret0 } // RemoveManifest indicates an expected call of RemoveManifest. -func (mr *MockPackClientMockRecorder) RemoveManifest(arg0, arg1, arg2 interface{}) *gomock.Call { +func (mr *MockPackClientMockRecorder) RemoveManifest(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveManifest", reflect.TypeOf((*MockPackClient)(nil).RemoveManifest), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveManifest", reflect.TypeOf((*MockPackClient)(nil).RemoveManifest), arg0, arg1) } // YankBuildpack mocks base method. diff --git a/pkg/client/manifest_remove.go b/pkg/client/manifest_remove.go index 6f1bae67e..41a79865d 100644 --- a/pkg/client/manifest_remove.go +++ b/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 } diff --git a/pkg/client/manifest_remove_test.go b/pkg/client/manifest_remove_test.go index 739ca1750..5f31f2dfa 100644 --- a/pkg/client/manifest_remove_test.go +++ b/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) }) diff --git a/pkg/client/manifest_rm.go b/pkg/client/manifest_rm.go index b5717245f..98ac8a092 100644 --- a/pkg/client/manifest_rm.go +++ b/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 } diff --git a/pkg/client/manifest_rm_test.go b/pkg/client/manifest_rm_test.go index b3fe58e46..bc683c206 100644 --- a/pkg/client/manifest_rm_test.go +++ b/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)