fix: use retryable http client #5315
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sgalsaleh
reviewed
May 8, 2025
| if err != nil { | ||
| return "", errors.Wrap(err, "failed to create new request") | ||
| } | ||
|
|
||
| reportingInfo := reporting.GetReportingInfo(appID) | ||
| reporting.InjectReportingInfoHeaders(req, reportingInfo) | ||
| reporting.InjectReportingInfoHeaders(req.Header, reportingInfo) |
There was a problem hiding this comment.
does this set the reporting headers correctly on the request because Header is a map and maps are passed by reference? don't wanna suddenly lose reporting information
There was a problem hiding this comment.
I tested it in a go playground
sgalsaleh
reviewed
May 8, 2025
pkg/util/http.go
Outdated
| if bodyStr != "" { | ||
| return resp, fmt.Errorf("%s %s giving up after %d attempt(s): %s", | ||
| req.Method, redactURL(req.URL), attempt, bodyStr) | ||
| } else { |
There was a problem hiding this comment.
could get rid of this else keyword for clarity and return directly
sgalsaleh
reviewed
May 8, 2025
pkg/util/http.go
Outdated
| if bodyStr != "" { | ||
| return resp, fmt.Errorf("%s %s giving up after %d attempt(s) with error %w: %s", | ||
| req.Method, redactURL(req.URL), attempt, err, bodyStr) | ||
| } else { |
sgalsaleh
reviewed
May 8, 2025
pkg/util/http.go
Outdated
| header.Add("User-Agent", buildversion.GetUserAgent()) | ||
| } | ||
|
|
||
| // errorHandler includes the body in the response when there is an error |
There was a problem hiding this comment.
would this cause OOM if the body was a non-complete binary download? should probably have a cap on the body size that can be read
There was a problem hiding this comment.
good call. i made this much more simple
sgalsaleh
previously approved these changes
May 8, 2025
sgalsaleh
approved these changes
May 8, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
APIs can be flakey. Use retryable http client for resilliency.
https://github.com/replicatedhq/embedded-cluster/actions/runs/14914714913/job/41898012325
Which issue(s) this PR fixes:
Does this PR require a test?
Does this PR require a release note?
Does this PR require documentation?