cli/command/container: remove reportError, and put StatusError to use#5236
Merged
thaJeztah merged 1 commit intodocker:masterfrom Jul 22, 2024
Merged
cli/command/container: remove reportError, and put StatusError to use#5236thaJeztah merged 1 commit intodocker:masterfrom
thaJeztah merged 1 commit intodocker:masterfrom
Conversation
thaJeztah
commented
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) |
Member
Author
There was a problem hiding this comment.
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?)
f8e5f7b to
882d070
Compare
Codecov ReportAttention: Patch coverage is
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 |
Member
Author
|
@laurazard first round of cleanup after the other one(s) 😄 |
882d070 to
6c14bbc
Compare
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]>
6c14bbc to
90058df
Compare
laurazard
approved these changes
Jul 22, 2024
Member
Author
|
Let me bring this one in 👍 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
reportErrorutility was present because cli.StatusError would print the error decorated withStatus: <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 emptyStatus(error message) in which case the error would be "ignored" (and only used for the exit-status), and thereportErrorutility 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:
withHelpwhich takes care of decorating errors with a "Run --help" hint for the user.toStatusErrorutility 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).reportErrorutility, and removes code that manually printed errors before returning.Behavior is mostly unmodified, with the exception of some slight reformatting of the errors:
withHelpadds adocker:prefix to the error, to indicate the error is produced by thedockercommand. This prefix was already present in most cases.Before this patch:
With this patch: