Skip to content
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

Return zero exit-code when force-removing non-existing containers #2678

Merged
merged 1 commit into from
Aug 27, 2020

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Aug 14, 2020

When using docker rm / docker container rm with the -f / --force option, attempts to remove non-existing containers should print a warning, but should return a zero exit code ("successful").

Currently, a non-zero exit code is returned, marking the removal as "failed";

$ docker rm -fv 798c9471b695
Error: No such container: 798c9471b695
$ echo $?
1

The command should match the behavior of rm / rm -f, with the exception that
a warning is printed (instead of silently ignored):

Running rm with -f silences output and returns a zero exit code:

touch some-file && rm -f no-such-file some-file; echo exit code: $?; ls -la
# exit code: 0
# total 0
# drwxr-xr-x    2 sebastiaan  staff    64 Aug 14 12:17 .
# drwxr-xr-x  199 sebastiaan  staff  6368 Aug 14 12:13 ..

mkdir some-directory && rm -rf no-such-directory some-directory; echo exit code: $?; ls -la
# exit code: 0
# total 0
# drwxr-xr-x    2 sebastiaan  staff    64 Aug 14 12:17 .
# drwxr-xr-x  199 sebastiaan  staff  6368 Aug 14 12:13 ..

Note that other reasons for a delete to fail should still result in a non-zero
exit code, matching the behavior of rm. For instance, in the example below,
the rm failed because directories can only be removed if the -r option is used;

touch some-file && mkdir some-directory && rm -f some-directory no-such-file some-file; echo exit code: $?; ls -la
# rm: some-directory: is a directory
# exit code: 1
# total 0
# drwxr-xr-x    3 sebastiaan  staff    96 Aug 14 14:15 .
# drwxr-xr-x  199 sebastiaan  staff  6368 Aug 14 12:13 ..
# drwxr-xr-x    2 sebastiaan  staff    64 Aug 14 14:15 some-directory

This patch updates the docker rm / docker container rm command to not produce
an error when attempting to remove a missing containers, and instead only print
the error, but return a zero (0) exit code.

With this patch applied:

docker create --name mycontainer busybox \
&& docker rm nosuchcontainer mycontainer; \
echo exit code: $?; \
docker ps -a --filter name=mycontainer
# df23cc8573f00e97d6e948b48d9ea7d75ce3b4faaab4fe1d3458d3bfa451f39d
# mycontainer
# Error: No such container: nosuchcontainer
# exit code: 0
# CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES

Signed-off-by: Sebastiaan van Stijn [email protected]

- What I did

- How I did it

- How to verify it

- Description for the changelog

Fix `docker rm --force` returning a non-zero exit code if one or more containers did not exist

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

When using `docker rm` / `docker container rm` with the `-f` / `--force` option, attempts to remove non-existing containers should print a warning, but should return a zero exit code ("successful").

Currently, a non-zero exit code is returned, marking the removal as "failed";

	$ docker rm -fv 798c9471b695
	Error: No such container: 798c9471b695
	$ echo $?
	1

The command should match the behavior of `rm` / `rm -f`, with the exception that
a warning is printed (instead of silently ignored):

Running `rm` with `-f` silences output and returns a zero exit code:

    touch some-file && rm -f no-such-file some-file; echo exit code: $?; ls -la
    # exit code: 0
    # total 0
    # drwxr-xr-x    2 sebastiaan  staff    64 Aug 14 12:17 .
    # drwxr-xr-x  199 sebastiaan  staff  6368 Aug 14 12:13 ..

    mkdir some-directory && rm -rf no-such-directory some-directory; echo exit code: $?; ls -la
    # exit code: 0
    # total 0
    # drwxr-xr-x    2 sebastiaan  staff    64 Aug 14 12:17 .
    # drwxr-xr-x  199 sebastiaan  staff  6368 Aug 14 12:13 ..

Note that other reasons for a delete to fail should still result in a non-zero
exit code, matching the behavior of `rm`. For instance, in the example below,
the `rm` failed because directories can only be removed if the `-r` option is used;

    touch some-file && mkdir some-directory && rm -f some-directory no-such-file some-file; echo exit code: $?; ls -la
    # rm: some-directory: is a directory
    # exit code: 1
    # total 0
    # drwxr-xr-x    3 sebastiaan  staff    96 Aug 14 14:15 .
    # drwxr-xr-x  199 sebastiaan  staff  6368 Aug 14 12:13 ..
    # drwxr-xr-x    2 sebastiaan  staff    64 Aug 14 14:15 some-directory

This patch updates the `docker rm` / `docker container rm` command to not produce
an error when attempting to remove a missing containers, and instead only print
the error, but return a zero (0) exit code.

With this patch applied:

    docker create --name mycontainer busybox \
    && docker rm nosuchcontainer mycontainer; \
    echo exit code: $?; \
    docker ps -a --filter name=mycontainer
    # df23cc8573f00e97d6e948b48d9ea7d75ce3b4faaab4fe1d3458d3bfa451f39d
    # mycontainer
    # Error: No such container: nosuchcontainer
    # exit code: 0
    # CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the fix_rm_force_exit_status branch from adf6cd5 to 9a071a9 Compare August 14, 2020 14:17
@codecov-commenter
Copy link

Codecov Report

Merging #2678 into master will increase coverage by 0.25%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2678      +/-   ##
==========================================
+ Coverage   58.16%   58.42%   +0.25%     
==========================================
  Files         295      295              
  Lines       21207    21210       +3     
==========================================
+ Hits        12336    12391      +55     
+ Misses       7963     7909      -54     
- Partials      908      910       +2     

@thaJeztah
Copy link
Member Author

@silvin-lubecki @cpuguy83 PTAL

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

Probably we should do this in the API, though.

@cpuguy83 cpuguy83 merged commit 612567c into docker:master Aug 27, 2020
@thaJeztah thaJeztah deleted the fix_rm_force_exit_status branch August 27, 2020 22:41
@thaJeztah
Copy link
Member Author

Probably we should do this in the API, though

hm, possibly; also feels a bit odd to have the API ignore a non-existing resource

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants