Skip to content

Conversation

elsirion
Copy link
Contributor

@elsirion elsirion commented Mar 25, 2023

I left the default async-https feature in to make this a non-breaking change. Could be phased out though imo, just make one of the new features a default feature instead.

Fixes #43

Copy link
Contributor

@vladimirfomene vladimirfomene left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks for the contribution! Will discuss with team whether it makes sense to phase out the async-https feature.

@coveralls
Copy link

coveralls commented Mar 28, 2023

Pull Request Test Coverage Report for Build 4518650682

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 15 unchanged lines in 1 file lost coverage.
  • Overall coverage remained the same at 80.077%

Files with Coverage Reduction New Missed Lines %
src/lib.rs 15 96.33%
Totals Coverage Status
Change from base Build 4419789814: 0.0%
Covered Lines: 832
Relevant Lines: 1039

💛 - Coveralls

@notmandatory notmandatory added the enhancement New feature or request label Mar 28, 2023
@notmandatory notmandatory added this to the Release 0.5.0 milestone Mar 28, 2023
Cargo.toml Outdated
async = ["reqwest", "reqwest/socks"]
async-https = ["async", "reqwest/default-tls"]
async-https-native = ["async", "reqwest/native-tls"]
async-https-rustls = ["async", "reqwest/rustls"]
Copy link
Member

@notmandatory notmandatory Mar 28, 2023

Choose a reason for hiding this comment

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

I think we're supposed to use the feature request/rustls-tls which defaults to using webpki-roots, or do you want manually set the root certs? In that case we'd need request/rustls-tls-manual-roots.

Copy link
Member

Choose a reason for hiding this comment

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

Or can add both options:

Suggested change
async-https-rustls = ["async", "reqwest/rustls"]
async-https-rustls = ["async", "reqwest/rustls-tls"]
async-https-rustls-manual-roots = ["async", "reqwest/rustls-tls-manual-roots"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx, fixed!

default = ["blocking", "async", "async-https"]
blocking = ["ureq", "ureq/socks-proxy"]
async = ["reqwest", "reqwest/socks"]
async-https = ["async", "reqwest/default-tls"]
Copy link
Member

Choose a reason for hiding this comment

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

I think it's OK to leave the async-https feature in for now as an option for folks who decided not to decide which TLS to use and go with the default request chooses.

@elsirion elsirion force-pushed the 2023-03-expose-tls-backend-choice branch from 2d0a683 to 44f7d29 Compare April 3, 2023 12:58
@elsirion
Copy link
Contributor Author

What's the status on this? Do I need to change anything?

@vladimirfomene
Copy link
Contributor

Sorry for the delay on this. This is good. I'm merging it.

@vladimirfomene vladimirfomene merged commit 54de811 into bitcoindevkit:master Apr 22, 2023
@elsirion elsirion deleted the 2023-03-expose-tls-backend-choice branch April 22, 2023 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expose TLS implementation choice via features

4 participants