-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add delegate for HttpClient override #64
base: main
Are you sure you want to change the base?
Conversation
public {{packageName}}Configuration(string basePath, | ||
HttpClientHandler httpClientHandler = null, | ||
public VaultConfiguration(string basePath, | ||
Func<HttpClientHandler, HttpClient> httpClientProvider = null, |
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.
what is Func
here? shouldn't this be HttpClient
?
(sorry if dumb question, looks weird from a non-c# perspective)
I saw IHttpClientFactory
above, shouldn't it take that as the param type?
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.
So, this is kind of weird. But because the VaultClient
needs to configure the HttpClientHandler
(for TLS among other things), which the HttpClient
is dependent on we have to allow users to pass a delegate (i.e. Func
) to new up their own HttpClient
with our pre-configured client handler.
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 was debating whether to allow just to pass their own HttpClient
object, like you're suggesting, but if they wanted to utilize the TLS, then they'd have to preconfigure it, which may be annoying.
It could be that we offer both options.
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.
In the Go version, we allow you to provide HTTPClient directly but then override the TLS options (if configured). Not sure if this is something doable here.
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.
Does this make sense in the context of IHttpClientFactory, as the factory manages the HttpClientHandler instances?
We also allow you to bring your own `HttpClient` through a delegate. This can work | ||
with any current factory pattern that you're using to create an `HttpClient` | ||
object, which is one of the recommended patterns for managing your `HttpClient` | ||
lifecycle. More on the `IHttpClientFactory` can be found [here][[http-client-factory]] |
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.
bad link?
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.
oh whooops
Description
This allows users to bring their own
HttpClient
instead ofHttpClientHandler
. This gives them more control over managing the lifecycle of theHttpClient
if they choose, which is notoriously difficult in .Net. It also aligns with the recommended factory pattern of creating HttpClients recommended hereIf you set the
timeout
when configuringVaultClient
that will take precedent over any timeout configured in your providedHttpClient
Resolves # VAULT-9887
How has this been tested?
Tested locally with a generic
HttpClient
and preconfigured with some random settings.