-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
wrap doRequest calls for error handling #11158
Conversation
Signed-off-by: FFMMM <FFMMM@users.noreply.github.com>
Signed-off-by: FFMMM <FFMMM@users.noreply.github.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.
Thanks for following up here! I left some questions below
Signed-off-by: FFMMM <FFMMM@users.noreply.github.com>
119efd3
to
7ad5982
Compare
@FFMMM Given some of the complexity that arose from this PR I think there's some refactoring that may need to be done for Both functions take Currently This would mean we'd have to make the handling a bit more verbose on all of the call sites, for example: _, resp, err := c.doRequest(r)
if err != nil {
return nil, fmt.Errorf("error making request: %w", err) // distinguish errors from doRequest
}
if err := requireOK(resp); err != nil {
return nil, err // this came from the response, meaning doRequest succeeded
}
defer closeResponseBody(resp) Does this make sense? cc @dnephin |
I haven't had a chance to catch up on this discussion, but I think that suggested refactor makes sense at a high level. Assuming the common case is |
hey @kisunji and @dnephin thank y'all for chiming in! let me agree with y'all below with some context and proposed path:
Yes that's actually something that @markan and I were chatting about when looking at the I think I'd love to have a chance to address this refactor in a subsequent PR.
There's actually >130 usages of I'm hoping that we can merge this PR if it makes sense and I can take on the work to have a stab at refactoring the code in a subsequent PR. What do y'all think? 👍🏼 or 👎🏼 ? |
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.
Sounds reasonable to defer the refactor to another PR!
low-prio todos from my perspective:
- revisit error-handling of
requireOK
to be more clear/explicit - use
api.StatusError
in allfmt.Errorf
's where we embed the status code in the error message
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/461161. |
Overview
This PR builds on #11054 to wrap all remaining
doRequest()
calls around arequire*()
call.Issue Related
#10865
Notes:
PR Checklist
go fmt
go mod