- Notifications
You must be signed in to change notification settings - Fork928
chore: handle errors in wsproxy server for cli using buildinfo#11584
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| // traceError is a helper function that aides developers debugging failed cli | ||
| // commands. When we pretty print errors, we lose the context in which they came. | ||
| // This function adds the context back. Unfortunately there is no easy way to get | ||
| // the prefix to: "error string: %w", so we do a bit of string manipulation. | ||
| // | ||
| //nolint:errorlint | ||
| func traceError(err error) string { | ||
| if uw, ok := err.(interface{ Unwrap() error }); ok { | ||
| a, b := err.Error(), uw.Unwrap().Error() | ||
| c := strings.TrimSuffix(a, b) | ||
| return c | ||
| } | ||
| return err.Error() | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is the biggest thing I added, and it only prints inverbose mode.
Essentially when we wrap some errors, we lose the context because the pretty printer doesn't know what to do with them.
An example is:
return fmt.Errorf("fetch buildinfo: %w", err)The underlyingerr is an SDK error, so the contextfetch buildinfo: is lost.
This code aims to put that back in somewhere that is reasonable, so if you run-v in your command, that info returns.
I am debating just always printing this, because I can't image it gets too unwieldly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I decided to always print this. Without it, it is really hard to debug where some api calls fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I kinda followed it, having more detailed errors is always a win. Is it not possible to have some sort of golden file or templated string that we can compare against so that someone doesn't unintentionally mess up the formatting?
| @@ -65,14 +70,28 @@ func (RootCmd) errorExample() *clibase.Cmd { | |||
| // A multi-error | |||
| { | |||
| Use: "multi-error", | |||
| Handler: func(inv *clibase.Invocation) error { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should we hide this command? There's no reason a user would ever need to use this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is hidden up the stack:
Line 12 in9656c7a
| Hidden:true, |
In this stack:
This stack of pull requests is managed by Graphite.Learn more about stacking. |
We can. We can make it a followup PR. I don't have the bandwidth atm to add that |
@Emyrk could you open an issue so that we can track and not forget? |
Uh oh!
There was an error while loading.Please reload this page.
See for yourself. I don't think encoding this into a static unit test would be very helpful. It would just cost more time to fix when you change things. Maybe a golden-file type test would be appropriate here at some point.
Before
After
We have to show the html because we have no clue what the response body is. This error is not perfect, but it is a lot better.