Skip to content

Container: let rm -f exit with success#821

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

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

Conversation

@akimd

@akimd akimd commented Jan 19, 2018

Copy link
Copy Markdown
Contributor

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 <[email protected]>
@codecov-io

codecov-io commented Jan 19, 2018

Copy link
Copy Markdown

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

dnephin commented Jan 19, 2018

Copy link
Copy Markdown
Contributor

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.

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

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

akimd commented Feb 16, 2018

Copy link
Copy Markdown
Contributor Author

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

@thaJeztah

Copy link
Copy Markdown
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