X Tutup
Skip to content

Container: let rm -f exit with success#821

Closed
akimd wants to merge 1 commit intodocker:masterfrom
akimd:rm-f-success
Closed

Container: let rm -f exit with success#821
akimd wants to merge 1 commit intodocker:masterfrom
akimd:rm-f-success

Conversation

@akimd
Copy link
Contributor

@akimd akimd commented Jan 19, 2018

rm -f non-existing exits with success, and so does docker rmi -f non-existing. But not docker rm -f.

Arguably we should actually check whether these errors are really doesNotExist (rm -f exits with failure for lack of permissions for instance), but at least we should be consistent.

FWIW, in the code, we have Rm in cli/command/container/rm.go and Remove in cli/command/image/remove.go. The latter has a test suite, not the former.

- A picture of a cute animal (not mandatory but encouraged)

lamasticot

`rm -f non-existing` exits with success, and so does
`docker rmi -f non-existing`.  But not `docker rm -f`.

Signed-off-by: Akim Demaille <akim.demaille@docker.com>
@codecov-io
Copy link

codecov-io commented Jan 19, 2018

Codecov Report

Merging #821 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #821      +/-   ##
==========================================
- Coverage   51.67%   51.66%   -0.02%     
==========================================
  Files         244      244              
  Lines       15823    15826       +3     
==========================================
- Hits         8177     8176       -1     
- Misses       7101     7105       +4     
  Partials      545      545

@dnephin
Copy link
Contributor

dnephin commented Jan 19, 2018

This is the same change that was made to rmi initially and had to be reverted. This is not correct, it needs to check for a "Not Found" error and only ignore those.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

@akimd can you update the PR, using a similar approach as #654 / #418 ? It's possible changes in the API are needed for this (see the linked PR's)

@akimd
Copy link
Contributor Author

akimd commented Feb 16, 2018

Thanks for the pointers! I'll have a look, but I can't tell when (yet).

@thaJeztah
Copy link
Member

Looks like this was addressed in #2678. The error is still printed, but the exit-code is 0 ("success");

docker rm nosuchcontainer
Error: No such container: nosuchcontainer

echo $?
1

docker rm -f nosuchcontainer
Error: No such container: nosuchcontainer

echo $?
0

@thaJeztah thaJeztah closed this Apr 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

X Tutup