Allow users to specify their own client implementation used by the library#73
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves HTTP client configurability by introducing a configurable request timeout and an option to supply a custom retryablehttp.Client, enabling users to bring their own transport settings as a base.
Changes:
- Added
timeouttohttpClientOptionwith a default of 5 minutes and a newWithTimeoutoption. - Added support for injecting a custom
retryablehttp.Clientvia a new HTTP option. - Made proxy assignment conditional on
proxyFuncbeing set.
Comments suppressed due to low confidence (1)
common_client.go:129
- When a custom
retryablehttp.Clientis provided,retryClient.HTTPClient.Transportmay benil(meaninghttp.DefaultTransport) or a non-*http.Transportimplementation. The current type assertion will fail and return an error, preventing use of custom clients. Consider handlingnilby defaulting to a clonedhttp.Transport(e.g., fromhttp.DefaultTransport) and either supporting non-*http.Transportby skipping transport mutations or documenting/validating this constraint up front.
transport, ok := retryClient.HTTPClient.Transport.(*http.Transport)
if !ok {
// this should always be true, because retryablehttp.NewClient() uses
// cleanhttp.DefaultPooledTransport()
return nil, fmt.Errorf("failed to get http transport from retryablehttp client")
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func WithRetryableHTTPClint(client *retryablehttp.Client) HTTPOption { | ||
| return func(c *httpClientOption) { | ||
| c.retryableHTTPClient = client | ||
| } | ||
| } |
There was a problem hiding this comment.
New behavior is introduced via WithRetryableHTTPClint (custom retryable client) and WithTimeout, but common_client_test.go doesn’t currently cover these option paths. Adding unit tests would help ensure timeouts are applied as intended and custom-client handling (including transport/TLS/proxy interactions) doesn’t regress.
| // HTTPOption defines a functional option for configuring the Client. | ||
| type HTTPOption func(*httpClientOption) | ||
|
|
||
| func WithRetryableHTTPClint(client *retryablehttp.Client) HTTPOption { |
There was a problem hiding this comment.
WithRetryableHTTPClint appears to be a misspelling of WithRetryableHTTPClient. Since this is a new exported API, it’s worth correcting now (and updating the struct comment on line 81) to avoid permanently shipping a typo in the public surface.
| func WithRetryableHTTPClint(client *retryablehttp.Client) HTTPOption { | |
| // WithRetryableHTTPClient sets a custom retryable HTTP client for the Client. | |
| func WithRetryableHTTPClient(client *retryablehttp.Client) HTTPOption { |
| } else { | ||
| retryClient = retryablehttp.NewClient() | ||
| retryClient.RetryMax = o.retryMax | ||
| retryClient.RetryWaitMax = o.retryWaitMax |
There was a problem hiding this comment.
WithTimeout currently only affects the built-in client path; when o.retryableHTTPClient is provided, o.timeout is ignored and the provided client's HTTPClient.Timeout is left unchanged. Given the PR description (“new options for setting request timeouts”), consider applying o.timeout to the provided client as well (or clearly documenting that WithTimeout is ignored when a custom retryable client is supplied).
| retryClient.RetryWaitMax = o.retryWaitMax | |
| retryClient.RetryWaitMax = o.retryWaitMax | |
| } | |
| // Apply timeout to the underlying HTTP client regardless of whether a | |
| // custom retryable client was supplied or a new one was created. | |
| if retryClient.HTTPClient != nil { |
| @@ -120,7 +139,9 @@ func (o *httpClientOption) newRetryableHTTPClient() (*retryablehttp.Client, erro | |||
| transport.TLSClientConfig.InsecureSkipVerify = true | |||
| } | |||
|
|
|||
| transport.Proxy = o.proxyFunc | |||
| if o.proxyFunc != nil { | |||
| transport.Proxy = o.proxyFunc | |||
| } | |||
|
|
|||
| retryClient.HTTPClient.Transport = transport | |||
There was a problem hiding this comment.
This function mutates the transport (and potentially TLS settings) on the retryablehttp.Client instance passed in via WithRetryableHTTPClint. If callers reuse the same client/transport elsewhere, this can cause unexpected cross-component side effects and can race with concurrent use. Consider cloning the http.Transport (and possibly the http.Client) before applying library-specific settings, or explicitly documenting that the passed client will be modified and must not be shared.
This pull request enhances the configurability of the HTTP client by introducing new options for setting request timeouts and allowing the use of a custom retryable HTTP client.
Using custom client allows users to specify transport options that will be used as a base.
Fixes #64
Fixes #60