Skip to content

docker image rm -f should exit with 1 if the image was not removed #418

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
adshmh wants to merge 1 commit into from
Closed

docker image rm -f should exit with 1 if the image was not removed #418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion cli/command/image/remove.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
Expand Down
29 changes: 26 additions & 3 deletions cli/command/image/remove_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand All @@ -31,6 +43,15 @@ func TestNewRemoveCommandErrors(t *testing.T) {
name: "wrong args",
expectedError: "requires at least 1 argument(s).",
},
{
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"},
Expand Down Expand Up @@ -68,14 +89,16 @@ func TestNewRemoveCommandSuccess(t *testing.T) {
},
},
{
name: "Image Deleted with force option",
Copy link
Contributor

@dnephin dnephin Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should still have a case here "Image not found with force option"

Copy link
Contributor Author

@adshmh adshmh Aug 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review. I added a test case.
Would it be a good idea for the docker/docker/client package to export the notFound* errors to make testing easier? I think in addition to the new test, stack/deploy_composefile_test.go also implements a type of notFound error for testing.

Copy link
Contributor

@dnephin dnephin Aug 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly. The problem with exporting them is that other packages could start to use them to report errors, which is not really the intent.

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",
expectedErrMsg: "Error: No such image: image1",
},

{
name: "Image Untagged",
args: []string{"image1"},
Expand Down
Empty file removed cli/command/image/testdata/remove-command-success.Image Deleted with force option.golden
View file
Open in desktop
Empty file.
1 change: 1 addition & 0 deletions cli/command/image/testdata/remove-command-success.Image not found with force option.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@

4 changes: 4 additions & 0 deletions vendor/github.com/docker/docker/client/image_remove.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.