Skip to content
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

clients: feature gate tls #545

Merged
merged 6 commits into from
Dec 6, 2021
Merged

clients: feature gate tls #545

merged 6 commits into from
Dec 6, 2021

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Oct 30, 2021

Closing #241

@niklasad1 niklasad1 force-pushed the na-clients-opt-of-from-tls branch from 3b39d60 to 2d9020a Compare December 3, 2021 19:01
#[derive(Debug, Copy, Clone)]
pub enum EitherStream<S, T> {
#[derive(Debug)]
pub enum EitherStream {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this but I realized I can just used placeholder type such as () to keep the generics here not sure that we need the generics anyway.

Unless we plan to support any transport using tokio rustls :)

for _ in 0..self.max_redirections {
tracing::debug!("Connecting to target: {:?}", target);

// The sockaddrs might get reused if the server replies with a relative URI.
let sockaddrs = std::mem::take(&mut target.sockaddrs);
for sockaddr in &sockaddrs {
let tcp_stream = match connect(*sockaddr, self.timeout, &target.host, &tls_connector).await {
#[cfg(feature = "tls")]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to do this to avoid feature gate the entire method.

@niklasad1 niklasad1 changed the title [WIP] clients: introduce tls feature flag clients: feature gate tls Dec 3, 2021
@niklasad1 niklasad1 marked this pull request as ready for review December 3, 2021 19:06
@niklasad1 niklasad1 requested a review from a team as a code owner December 3, 2021 19:06

[features]
default = ["tls"]
tls = ["hyper-rustls/webpki-tokio"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL

Copy link
Contributor

@maciejhirsz maciejhirsz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, do we have CI that tests with different features on and off?

@niklasad1
Copy link
Member Author

Looks good, do we have CI that tests with different features on and off?

yeah, we have this which should compile each crate with default-features = false and test each feature "standalone". However we just run the tests on the default features because it was so slow but I can double check the tests locally to be on the safe-side

jsonrpsee-types = { path = "../types", version = "0.6.0" }
jsonrpsee-utils = { path = "../utils", version = "0.6.0", features = ["client", "http-helpers"] }
serde = { version = "1.0", default-features = false, features = ["derive"] }
serde_json = "1.0"
thiserror = "1.0"
tokio = { version = "1.8", features = ["time"] }
tracing = "0.1"
url = "2.2"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will close #554

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@dvdplm dvdplm merged commit 3f1c7fc into master Dec 6, 2021
@dvdplm dvdplm deleted the na-clients-opt-of-from-tls branch December 6, 2021 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants