- Notifications
You must be signed in to change notification settings - Fork927
fix(cli): ensure that the support bundle command does not panic on zero values#14392
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
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.
👍
Do you have an idea why the original code that panicked, didn't post any error/warning?
Uh oh!
There was an error while loading.Please reload this page.
Just a testing gap, to be honest. |
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.
Minor suggestions, LGTM
Uh oh!
There was an error while loading.Please reload this page.
cli/support_test.go Outdated
| assert.NoError(t, json.NewEncoder(w).Encode(resp)) | ||
| default: | ||
| // Simply return a 200 OK for everything else. | ||
| w.WriteHeader(http.StatusOK) |
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.
Would a non-200 response also trigger this problem?
For example:
eg.Go(func()error {hr,err:=healthsdk.New(client).DebugHealth(ctx)iferr!=nil {returnxerrors.Errorf("fetch health report: %w",err)}d.HealthReport=&hrreturnnil})
I think it might be more idiomatic to have all calls fail rather than return empty values, although both are worthwhile testing.
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.
That's a good point, I'll test against a range of status codes. 👍
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.
Updated!
Co-authored-by: Danny Kopping <danny@coder.com>
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.
👍
82e6070 intomainUh oh!
There was an error while loading.Please reload this page.
We try to write a cute little summary at the end of the bundle, but that could panic if some of the fields of the bundle were nil. Adds a test that essentially ensures nil values in a bundle, and ensures that it can be handled without losing our towels.