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

cli/command/container: remove reportError, and put StatusError to use #5236

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

thaJeztah
Copy link
Member

The reportError utility was present because cli.StatusError would print the error decorated with Status: <error-message>, Code: <exit-code>. That was not desirable in many cases as it would mess-up the output. To prevent this, the CLI had code to check for an empty Status (error message) in which case the error would be "ignored" (and only used for the exit-status), and the reportError utility would be used to manually print a custom error message before returning the error.

Now that bca2090 fixed the output format of cli.StatusError, and 3dd6fc3 and 350a0b6 no longer discard these error, we can get rid of this utility, and just set the error-message for the status-error.

This patch:

  • Introduces a withHelp which takes care of decorating errors with a "Run --help" hint for the user.
  • Introduces a toStatusError utility that detects certain errors in the container to assign a corresponding exit-code (these error-codes can be used to distinguish "client" errors from "container" errors).
  • Removes the reportError utility, and removes code that manually printed errors before returning.

Behavior is mostly unmodified, with the exception of some slight reformatting of the errors:

  • withHelp adds a docker: prefix to the error, to indicate the error is produced by the docker command. This prefix was already present in most cases.
  • The "--help" hint is slightly updated ("Run 'docker run --help' for more information" instead of "See 'docker run --help'"), to make it more clear that it's a "call to action".
  • An empty is added before the "--help" hint to separate it better from the error-message.

Before this patch:

$ docker run --pull=invalid-option alpine
docker: invalid pull option: 'invalid-option': must be one of "always", "missing" or "never".
See 'docker run --help'.
$ echo $?
125

$ docker run --rm alpine nosuchcommand
docker: Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "nosuchcommand": executable file not found in $PATH: unknown.
$ echo $?
127

With this patch:

$ docker run --pull=invalid-option alpine
docker: invalid pull option: 'invalid-option': must be one of "always", "missing" or "never"

Run 'docker run --help' for more information
$ echo $?
125

$ docker run --rm alpine nosuchcommand
docker: Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "nosuchcommand": executable file not found in $PATH: unknown.

Run 'docker run --help' for more information
$ echo $?
127

@thaJeztah thaJeztah added this to the 28.0.0 milestone Jul 5, 2024
@thaJeztah thaJeztah self-assigned this Jul 5, 2024
Comment on lines -210 to +216
reportError(stderr, "run", err.Error(), false)
if copts.autoRemove {
// wait container to be removed
<-statusChan
}
return runStartContainerErr(err)
return toStatusError(err)
Copy link
Member Author

Choose a reason for hiding this comment

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

Slight change here; previously we would print the error before the container was removed; this would be for the "old API, --rm running on the client-side" case. I don't think this would make much of a difference in practice, and it would only be for old API versions (API 1.24 I think?)

@thaJeztah thaJeztah force-pushed the cleanup_run_errors branch from f8e5f7b to 882d070 Compare July 5, 2024 11:07
@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2024

Codecov Report

Attention: Patch coverage is 40.00000% with 21 lines in your changes missing coverage. Please review.

Project coverage is 61.42%. Comparing base (ce4469a) to head (6c14bbc).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5236      +/-   ##
==========================================
- Coverage   61.44%   61.42%   -0.03%     
==========================================
  Files         298      298              
  Lines       20824    20827       +3     
==========================================
- Hits        12796    12793       -3     
- Misses       7113     7120       +7     
+ Partials      915      914       -1     

@thaJeztah thaJeztah marked this pull request as ready for review July 5, 2024 11:33
@thaJeztah thaJeztah requested a review from laurazard July 5, 2024 11:34
@thaJeztah
Copy link
Member Author

@laurazard first round of cleanup after the other one(s) 😄

@thaJeztah thaJeztah force-pushed the cleanup_run_errors branch from 882d070 to 6c14bbc Compare July 16, 2024 23:27
The `reportError` utility was present because cli.StatusError would print
the error decorated with `Status: <error-message>, Code: <exit-code>`.
That was not desirable in many cases as it would mess-up the output. To
prevent this, the CLI had code to check for an empty `Status` (error message)
in which case the error would be "ignored" (and only used for the exit-status),
and the `reportError` utility would be used to manually print a custom error
message before returning the error.

Now that bca2090 fixed the output format
of `cli.StatusError`, and 3dd6fc3 and
350a0b6 no longer discard these error,
we can get rid of this utility, and just set the error-message for
the status-error.

This patch:

- Introduces a `withHelp` which takes care of decorating errors with
  a "Run --help" hint for the user.
- Introduces a `toStatusError` utility that detects certain errors in
  the container to assign a corresponding exit-code (these error-codes
  can be used to distinguish "client" errors from "container" errors).
- Removes the `reportError` utility, and removes code that manually
  printed errors before returning.

Behavior is mostly unmodified, with the exception of some slight reformatting
of the errors:

- `withHelp` adds a `docker:` prefix to the error, to indicate the error
  is produced by the `docker` command. This prefix was already present
  in most cases.
- The "--help" hint is slightly updated ("Run 'docker run --help' for
  more information" instead of "See 'docker run --help'"), to make it
  more clear that it's a "call to action".
- An empty is added before the "--help" hint to separate it better from
  the error-message.

Before this patch:

    $ docker run --pull=invalid-option alpine
    docker: invalid pull option: 'invalid-option': must be one of "always", "missing" or "never".
    See 'docker run --help'.
    $ echo $?
    125

    $ docker run --rm alpine nosuchcommand
    docker: Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "nosuchcommand": executable file not found in $PATH: unknown.
    $ echo $?
    127

With this patch:

    $ docker run --pull=invalid-option alpine
    docker: invalid pull option: 'invalid-option': must be one of "always", "missing" or "never"

    Run 'docker run --help' for more information
    $ echo $?
    125

    $ docker run --rm alpine nosuchcommand
    docker: Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "nosuchcommand": executable file not found in $PATH: unknown.

    Run 'docker run --help' for more information
    $ echo $?
    127

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the cleanup_run_errors branch from 6c14bbc to 90058df Compare July 17, 2024 13:59
Copy link
Member

@laurazard laurazard left a comment

Choose a reason for hiding this comment

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

Looks great overall, thanks!

cli/command/container/create_test.go Show resolved Hide resolved
cli/command/container/run_test.go Show resolved Hide resolved
@thaJeztah
Copy link
Member Author

Let me bring this one in 👍

@thaJeztah thaJeztah merged commit d5f90ed into docker:master Jul 22, 2024
88 checks passed
@thaJeztah thaJeztah deleted the cleanup_run_errors branch July 22, 2024 15:56
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.

3 participants