-
Notifications
You must be signed in to change notification settings - Fork 6
Add delegate for HttpClient override #64
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
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
HttpClientinstead ofHttpClientHandler. This gives them more control over managing the lifecycle of theHttpClientif they choose, which is notoriously difficult in .Net. It also aligns with the recommended factory pattern of creating HttpClients recommended hereIf you set the
timeoutwhen configuringVaultClientthat will take precedent over any timeout configured in your providedHttpClientResolves # VAULT-9887
How has this been tested?
Tested locally with a generic
HttpClientand preconfigured with some random settings.