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: make cli.StatusError slightly prettier #5231

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jul 4, 2024

This error didn't do a great job at formatting. If a StatusError was produced without a Status message, it would print a very non-informative error, with information missing.

Let's update the output:

  • If a status-message is provided; print just that (after all the status code is something that can be found from the shell, e.g. through echo $? in Bash).
  • If no status-message is provided: print a message more similar to Go's exec.ExecError, which uses os.rocessState.String() (see 1).

Before this patch, an error without custom status would print:

Status: , Code: 2

After this patch:

exit status 2

In situations where a custom error-message is provided, the error-message
is print as-is, whereas before this patch, the message got combined with
the Status: and Code:, which resulted in some odd output.

Before this patch:

docker volume --no-such-flag
Status: unknown flag: --no-such-flag
See 'docker volume --help'.

Usage:  docker volume COMMAND

Manage volumes

Commands:
  create      Create a volume
  inspect     Display detailed information on one or more volumes
  ls          List volumes
  prune       Remove unused local volumes
  rm          Remove one or more volumes
  update      Update a volume (cluster volumes only)

Run 'docker volume COMMAND --help' for more information on a command.
, Code: 125

With this patch, the error is shown as-is;

docker volume --no-such-flag
unknown flag: --no-such-flag
See 'docker volume --help'.

Usage:  docker volume COMMAND

Manage volumes

Commands:
  create      Create a volume
  inspect     Display detailed information on one or more volumes
  ls          List volumes
  prune       Remove unused local volumes
  rm          Remove one or more volumes
  update      Update a volume (cluster volumes only)

Run 'docker volume COMMAND --help' for more information on a command.

While the exit-code is no longer printed, it's still properly handled;

echo $?
125

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

@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 61.47%. Comparing base (5aae44b) to head (bca2090).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5231      +/-   ##
==========================================
- Coverage   61.48%   61.47%   -0.02%     
==========================================
  Files         298      298              
  Lines       20813    20815       +2     
==========================================
- Hits        12797    12795       -2     
- Misses       7104     7107       +3     
- Partials      912      913       +1     

@thaJeztah
Copy link
Member Author

I have some changes staged to look at actually providing an error-message for some of these, but I need to clean it up a bit 😅 - also have #5229 (comment) as a follow-up.

@thaJeztah thaJeztah requested review from Benehiko and laurazard July 4, 2024 17:59
This error didn't do a great job at formatting. If a StatusError was
produced without a Status message, it would print a very non-informative
error, with information missing.

Let's update the output:

- If a status-message is provided; print just that (after all the
  status code is something that can be found from the shell, e.g.
  through `echo $?` in Bash).
- If no status-message is provided: print a message more similar to
  Go's `exec.ExecError`, which uses `os.rocessState.String()` (see [1]).

Before this patch, an error without custom status would print:

    Status: , Code: 2

After this patch:

    exit status 2

In situations where a custom error-message is provided, the error-message
is print as-is, whereas before this patch, the message got combined with
the `Status:` and `Code:`, which resulted in some odd output.

Before this patch:

    docker volume --no-such-flag
    Status: unknown flag: --no-such-flag
    See 'docker volume --help'.

    Usage:  docker volume COMMAND

    Manage volumes

    Commands:
      create      Create a volume
      inspect     Display detailed information on one or more volumes
      ls          List volumes
      prune       Remove unused local volumes
      rm          Remove one or more volumes
      update      Update a volume (cluster volumes only)

    Run 'docker volume COMMAND --help' for more information on a command.
    , Code: 125

With this patch, the error is shown as-is;

    docker volume --no-such-flag
    unknown flag: --no-such-flag
    See 'docker volume --help'.

    Usage:  docker volume COMMAND

    Manage volumes

    Commands:
      create      Create a volume
      inspect     Display detailed information on one or more volumes
      ls          List volumes
      prune       Remove unused local volumes
      rm          Remove one or more volumes
      update      Update a volume (cluster volumes only)

    Run 'docker volume COMMAND --help' for more information on a command.

While the exit-code is no longer printed, it's still properly handled;

    echo $?
    125

[1]: https://github.com/golang/go/blob/82c14346d89ec0eeca114f9ca0e88516b2cda454/src/os/exec_posix.go#L107-L135

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah merged commit cad08ff into docker:master Jul 5, 2024
94 checks passed
@thaJeztah thaJeztah deleted the prettier_exit_status branch July 5, 2024 08:50
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