Merge pull request #654 from dnephin/pr-fix-image-rm

Fix behaviour of `rmi -f` with unexpected errors

cli/command/image/remove.go

@@ -9,6 +9,7 @@ import (
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
 	"github.com/docker/docker/api/types"
+	apiclient "github.com/docker/docker/client"
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 )
@@ -56,9 +57,13 @@ func runRemove(dockerCli command.Cli, opts removeOptions, images []string) error
 	}
 
 	var errs []string
+	var fatalErr = false
 	for _, img := range images {
 		dels, err := client.ImageRemove(ctx, img, options)
 		if err != nil {
+			if !apiclient.IsErrNotFound(err) {
+				fatalErr = true
+			}
 			errs = append(errs, err.Error())
 		} else {
 			for _, del := range dels {
@@ -73,7 +78,7 @@ func runRemove(dockerCli command.Cli, opts removeOptions, images []string) error
 
 	if len(errs) > 0 {
 		msg := strings.Join(errs, "\n")
-		if !opts.force {
+		if !opts.force || fatalErr {
 			return errors.New(msg)
 		}
 		fmt.Fprintf(dockerCli.Err(), msg)

cli/command/image/remove_test.go

@@ -13,6 +13,18 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+type notFound struct {
+	imageID string
+}
+
+func (n notFound) Error() string {
+	return fmt.Sprintf("Error: No such image: %s", n.imageID)
+}
+
+func (n notFound) NotFound() bool {
+	return true
+}
+
 func TestNewRemoveCommandAlias(t *testing.T) {
 	cmd := newRemoveCommand(test.NewFakeCli(&fakeClient{}))
 	assert.True(t, cmd.HasAlias("rmi"))
@@ -31,6 +43,15 @@ func TestNewRemoveCommandErrors(t *testing.T) {
 			name:          "wrong args",
 			expectedError: "requires at least 1 argument.",
 		},
+		{
+			name:          "ImageRemove fail with force option",
+			args:          []string{"-f", "image1"},
+			expectedError: "error removing image",
+			imageRemoveFunc: func(image string, options types.ImageRemoveOptions) ([]types.ImageDeleteResponseItem, error) {
+				assert.Equal(t, "image1", image)
+				return []types.ImageDeleteResponseItem{}, errors.Errorf("error removing image")
+			},
+		},
 		{
 			name:          "ImageRemove fail",
 			args:          []string{"arg1"},
@@ -43,12 +64,14 @@ func TestNewRemoveCommandErrors(t *testing.T) {
 		},
 	}
 	for _, tc := range testCases {
-		cmd := NewRemoveCommand(test.NewFakeCli(&fakeClient{
+		t.Run(tc.name, func(t *testing.T) {
-			imageRemoveFunc: tc.imageRemoveFunc,
+			cmd := NewRemoveCommand(test.NewFakeCli(&fakeClient{
-		}))
+				imageRemoveFunc: tc.imageRemoveFunc,
-		cmd.SetOutput(ioutil.Discard)
+			}))
-		cmd.SetArgs(tc.args)
+			cmd.SetOutput(ioutil.Discard)
-		testutil.ErrorContains(t, cmd.Execute(), tc.expectedError)
+			cmd.SetArgs(tc.args)
+			testutil.ErrorContains(t, cmd.Execute(), tc.expectedError)
+		})
 	}
 }
 
@@ -57,7 +80,7 @@ func TestNewRemoveCommandSuccess(t *testing.T) {
 		name            string
 		args            []string
 		imageRemoveFunc func(image string, options types.ImageRemoveOptions) ([]types.ImageDeleteResponseItem, error)
-		expectedErrMsg  string
+		expectedStderr  string
 	}{
 		{
 			name: "Image Deleted",
@@ -68,14 +91,16 @@ func TestNewRemoveCommandSuccess(t *testing.T) {
 			},
 		},
 		{
-			name: "Image Deleted with force option",
+			name: "Image not found with force option",
 			args: []string{"-f", "image1"},
 			imageRemoveFunc: func(image string, options types.ImageRemoveOptions) ([]types.ImageDeleteResponseItem, error) {
 				assert.Equal(t, "image1", image)
-				return []types.ImageDeleteResponseItem{}, errors.Errorf("error removing image")
+				assert.Equal(t, true, options.Force)
+				return []types.ImageDeleteResponseItem{}, notFound{"image1"}
 			},
-			expectedErrMsg: "error removing image",
+			expectedStderr: "Error: No such image: image1",
 		},
+
 		{
 			name: "Image Untagged",
 			args: []string{"image1"},
@@ -96,14 +121,14 @@ func TestNewRemoveCommandSuccess(t *testing.T) {
 		},
 	}
 	for _, tc := range testCases {
-		cli := test.NewFakeCli(&fakeClient{imageRemoveFunc: tc.imageRemoveFunc})
+		t.Run(tc.name, func(t *testing.T) {
-		cmd := NewRemoveCommand(cli)
+			cli := test.NewFakeCli(&fakeClient{imageRemoveFunc: tc.imageRemoveFunc})
-		cmd.SetOutput(ioutil.Discard)
+			cmd := NewRemoveCommand(cli)
-		cmd.SetArgs(tc.args)
+			cmd.SetOutput(ioutil.Discard)
-		assert.NoError(t, cmd.Execute())
+			cmd.SetArgs(tc.args)
-		if tc.expectedErrMsg != "" {
+			assert.NoError(t, cmd.Execute())
-			assert.Equal(t, tc.expectedErrMsg, cli.ErrBuffer().String())
+			assert.Equal(t, tc.expectedStderr, cli.ErrBuffer().String())
-		}
+			golden.Assert(t, cli.OutBuffer().String(), fmt.Sprintf("remove-command-success.%s.golden", tc.name))
-		golden.Assert(t, cli.OutBuffer().String(), fmt.Sprintf("remove-command-success.%s.golden", tc.name))
+		})
 	}
 }