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.

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

Fix behaviour of `rmi -f` with unexpected errors #654

Merged
merged 2 commits into from Oct 31, 2017

Conversation

Projects
None yet
7 participants
@dnephin
Collaborator

dnephin commented Oct 30, 2017

Carry #418
Closes #418
Closes #567

docker rm -f exits with 1 if the image was not removed
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
Use t.Run() for tests
Signed-off-by: Daniel Nephin <dnephin@docker.com>
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 30, 2017

Codecov Report

Merging #654 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #654      +/-   ##
==========================================
+ Coverage   50.02%   50.03%   +0.01%     
==========================================
  Files         216      216              
  Lines       17676    17680       +4     
==========================================
+ Hits         8842     8846       +4     
  Misses       8391     8391              
  Partials      443      443

codecov-io commented Oct 30, 2017

Codecov Report

Merging #654 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #654      +/-   ##
==========================================
+ Coverage   50.02%   50.03%   +0.01%     
==========================================
  Files         216      216              
  Lines       17676    17680       +4     
==========================================
+ Hits         8842     8846       +4     
  Misses       8391     8391              
  Partials      443      443

@dnephin dnephin requested review from vdemeester, vieux and thaJeztah Oct 30, 2017

@dnephin

First commit from #418 LGTM

@thaJeztah

LGTM, thanks for carrying!

@vdemeester

LGTM 🐮

@vdemeester vdemeester merged commit 6e04da3 into docker:master Oct 31, 2017

9 checks passed

ci/circleci: cross Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 50%)
Details
codecov/project 50.03% (+0.01%) compared to 0c4fa69
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
dco-signed All commits are signed

@GordonTheTurtle GordonTheTurtle added this to the 17.11.0 milestone Oct 31, 2017

@thaJeztah thaJeztah modified the milestones: 17.11.0, 17.12.0 Oct 31, 2017

@dnephin dnephin deleted the dnephin:pr-fix-image-rm branch Oct 31, 2017

@asottile

This comment has been minimized.

Show comment
Hide comment
@asottile

asottile Dec 6, 2017

Noticed this PR while trying to debug the following oddity (missing trailing newline):

$ docker --version
Docker version 17.09.0-ce, build afdb6d4
$ docker rmi doesnt_exist -f
Error: No such image: doesnt_exist$ 

This patch (or something related to it) fixes that but changes the exit code:

root@91136c2cdac6:/go/src/github.com/docker/docker# docker rmi -f doesnt_exist; echo $?
Error response from daemon: No such image: doesnt_exist:latest
1

From this it seems intentional, but perhaps unfortunate:
this is now inconsistent behaviour from what I would expect with (coreutils) rm ... vs rm -f ... (the latter exiting 0 even if the thing doesn't exist).

asottile commented Dec 6, 2017

Noticed this PR while trying to debug the following oddity (missing trailing newline):

$ docker --version
Docker version 17.09.0-ce, build afdb6d4
$ docker rmi doesnt_exist -f
Error: No such image: doesnt_exist$ 

This patch (or something related to it) fixes that but changes the exit code:

root@91136c2cdac6:/go/src/github.com/docker/docker# docker rmi -f doesnt_exist; echo $?
Error response from daemon: No such image: doesnt_exist:latest
1

From this it seems intentional, but perhaps unfortunate:
this is now inconsistent behaviour from what I would expect with (coreutils) rm ... vs rm -f ... (the latter exiting 0 even if the thing doesn't exist).

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Dec 6, 2017

Collaborator

This PR should actually fix the problem you see in 17.09. It'll be in 17.12

Collaborator

dnephin commented Dec 6, 2017

This PR should actually fix the problem you see in 17.09. It'll be in 17.12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment