feat: improve CLI error messages #6778
Conversation
There was a problem hiding this comment.
I agree that the original error for login is pretty bad, but I don't think reformatting it improves it, it's just a bad message to begin with (which is a side-effect of how it's implemented). It should simply have said Unable to reach "https://dev.coder.con", is the URL correct and is Coder accessible from your browser? (i.e. mainly omit the first user part and the detailed error part).
We could have a --debug flag that prints more detailed error messages.
Errors from the API still seem a bit messy:
❯ /tmp/coder ssh list
ERROR
GET http://127.0.0.1:3000/api/v2/workspaces?q=owner%3Ame: unexpected status code 401: You are signed out or your session has expired. Please sign in again to continue.: Try logging in using 'coder login <url>'.
Error: API key expired at "2023-03-21 15:14:13.352559 +0000 UTC".
They were better before:
❯ coder list
You are signed out or your session has expired. Please sign in again to continue. Try logging in using 'coder login <url>'.
Run 'coder list --help' for usage.
Unless debugging, the expiry date isn't very useful, and the status code is pretty much useless since the human message already says it. I think keeping the error message mostly to a single line is good for CLI users too.
Personally I think the formatting doesn't really help here either:
❯ /tmp/coder ssh test -G
ERROR
upload GPG public keys and ownertrust to workspace
─────
export local ownertrust from GPG
─────
`gpg --export-ownertrust` failed: stderr: gpg: Fatal: can't open '/home/mafredri/.gnupg/trustdb.gpg': No such file or directory
stdout:
exit status 2
─────
exit status 2
─────
But the original wasn't good either:
❯ coder ssh test -G
upload GPG public keys and ownertrust to workspace: export local ownertrust from GPG: `gpg --export-ownertrust` failed: stderr: gpg: Fatal: can't open '/home/mafredri/.gnupg/trustdb.gpg': No such file or directory
stdout:
exit status 2: exit status 2
Run 'coder ssh --help' for usage.
Original feels less busy though, but obviously stdout should be discarded since it's empty (and likely always), as should the double exit status.
|
kk.. I got rid of decoration:
and simplified in cases where we have a |
|
I want to avoid changing the errors themselves in this PR and just get us to a state where we feel they're as good as they were before clibase. |
|
Per slack poll, it's now on one line |
|
Looks better 👍🏻, thanks. I do seem to have a knack for finding these problems though. Might be good to verify what the behavior is for a few more commands: I.e. duplicated message and And weird long-indent for second exit code 🤔 |
|
Good catch. We'll need golden files for errors soon... Fixed both by removing the unnecessary recursion in the error formatter.. c9e31bb |
| // only show the SDK error on the top of the stack. | ||
| msg = sdkError.Message | ||
| if sdkError.Helper != "" { | ||
| msg = msg + "\n" + sdkError.Helper |
There was a problem hiding this comment.
The newline here makes sense and makes this look nice. 👍🏻
Before clibase:
clibase:
now:
