Conversation
0712aae to
06f7f37
Compare
|
Overall I really like the custom error and think it might be usable in one or the other place 👍 |
| const ( | ||
| ExitOK = 0 // successful execution | ||
| ExitError = 1 // generic error | ||
| ExitUsageError = 80 // invalid input, validation errors | ||
| ExitForbidden = 83 // permission denied | ||
| ExitInternalError = 100 // bug in nctl | ||
| ExitUnavailable = 101 // API/service unreachable | ||
| ) |
There was a problem hiding this comment.
I think that the biggest long-term risk is misclassification of errors.
People will start wrapping everything as a usage error because it looks nice.
Maybe we can provide here some rule of thumb, like:
usage error: user typo, missing flag, invalid value ...
forbidden: permission/auth ...
etc...
| slices.Sort(available) | ||
|
|
||
| return cli.ErrorWithContext(fmt.Errorf("could not find context %q in kubeconfig", contextName)). | ||
| WithExitCode(cli.ExitUsageError). |
There was a problem hiding this comment.
some helpers here (under api/...) start returning internal/cli.Error directly. That's totally fine as our api is internal (for now). But it would be good to clarify this intention somewhere (and be consistent).
Or maybe we can have api return more semantic errors that get wrapped later?
| } | ||
| } | ||
|
|
||
| func TestErrorOutput(t *testing.T) { |
There was a problem hiding this comment.
With this MR, nctl error output becomes structured.
People will probably start depend on it at some point, e.g. shell scripts grepping output or tests asserting error messages. I don't mean we should lock it totally, but maybe we can check more stuff here, like: iteration order, emoji placement, spacing before/after, indentation of multi-line messages, etc...
Depends on: #348
This MR introduces a custom error that can be used to add context and suggestions. It also uses standardized exit codes: