Bitreq client builder (rustls + native-tls)#516
Bitreq client builder (rustls + native-tls)#516APonce911 wants to merge 65 commits intorust-bitcoin:masterfrom
Conversation
…low ClientBuilder
…nce per client instead of per connection.
APonce911
requested review from
TheBlueMatt,
oleonardolima and
tnull
as code owners
…llible to maintain interface consitency
|
Hi @APonce911, little comment on dev process. Its better if you rebase on master instead of merging if you want review. With 65 patches reviewers are not going to know where to look. Many of us review by pulling down the PR and reading each patch in our terminals. Different folk treat things differently but to me if a PR is 'open' it is a request to merge so it should be in a mergable state. We use the process where each commit should be a single logical change. There shouldn't be WIP commits and 'fix something I did three commits ago' commits. The whole set of commits (aka the 'patch set') should be clean and reviewable as stand alone changes. Each described in the commit log. |
|
@tcharding thanks for the feedback. That's something I'll keep in mind! As I've told you, I'm not used to open source so every bit of insight of how you do things is helpful. The process is a bit different than what I'm used to but I'm more than happy to adapt. About those patches and then small subsequent fixes, I've had a hard time trying to replicate the CI suite locally, it didn't catch some corner cases. I could have reset some commits, but didn't think about it by that time. |
|
In a perfect world every commit builds and passes CI cleanly. We do not however check each commit in CI. But locally during dev you should really, in my opinion, be trying to make sure that at least [0] I have shell alias' for these |
|
If you want to get past the ci-doesn't-run-for-first-time-contributors just throw up a quick docs fix PR and I'll merge it. |
|
@tcharding will definitely do that! However I will be off for a few days and won't be able to proceed with the rebase adjustments and doc pr until next week. Thanks for you time again! |
| @@ -0,0 +1,39 @@ | |||
| //! This example demonstrates the client builder with custom DER certificate. | |||
There was a problem hiding this comment.
What's the value of this? Should it not either be a test or a doctest so that its visible to devs?
| all(feature = "native-tls", feature = "tokio-native-tls"), | ||
| all(feature = "rustls", feature = "tokio-rustls") | ||
| )))] | ||
| mod disabled { |
There was a problem hiding this comment.
Please just have one instance of ClientConfig. You can cfg the field in it but there's no reason to have two fully written out instances.
| /// # Arguments | ||
| /// | ||
| /// * `capacity` - Maximum number of cached connections | ||
| /// | ||
| /// # Example | ||
| /// | ||
| /// ```no_run | ||
| /// # use bitreq::Client; | ||
| /// let client = Client::builder() | ||
| /// .with_capacity(10) | ||
| /// .build() | ||
| /// .unwrap(); | ||
| /// ``` |
There was a problem hiding this comment.
Please reduce the verbosity of the docs here and elsewhere. LLMs lovvveeeee to generate output, but more often than not half of it is totally useless and doesn't provide any meaningful information.
| /// that can be converted into a `Vec<u8>`, such as `Vec<u8>`, `&[u8]`, or arrays. | ||
| /// This is useful when connecting to servers using self-signed certificates | ||
| /// or custom Certificate Authorities. | ||
| /// |
There was a problem hiding this comment.
Docs should clarify whether this overrides default trust store or not (probably should).
|
|
||
| #[cfg(feature = "rustls")] | ||
| pub(crate) fn append_certificate(mut self, cert_der: Vec<u8>) -> Result<Self, Error> { | ||
| let certificates = Arc::make_mut(&mut self.inner); |
There was a problem hiding this comment.
Arc::make_mut screams "I shouldn't be an Arc".
| } | ||
|
|
||
| #[cfg(feature = "rustls")] | ||
| pub(crate) fn append_certificate(mut self, cert_der: Vec<u8>) -> Result<Self, Error> { |
There was a problem hiding this comment.
Seems like we should avoid taking by ownership so the caller isn't constantly cloneing the cert store as we go.
3 participants
Summary
This PR addresses issue #473 by creating a Client Builder able to receive custom root certificates at runtime.
The ClientBuilder requires async-https-rustls or async-https-native-tls features.
Changes
This PR substitutes the 502