-
Notifications
You must be signed in to change notification settings - Fork 313
Remove parity features #2933
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
Remove parity features #2933
Conversation
Marking this as a draft since I want to add documentation to our README and/or TSG; however, first I want to create a couple crates external to the workspace to try scenarios such as enabling |
I've created https://github.com/heaths/azsdk-rust-2750 to test out various scenarios. This is a work in progress. |
I can test this tomorrow in my internal PR to see if it prevents openssl from coming in as well. |
Locally with
and your branch checked out, I'm still seeing
If I remove the azure libs it's not present, the only |
Thanks. After adding more use case examples, I can see where the problem is. Need to do a bit more feature refactoring, but does this seem like a reasonable path forward? What I may need to do is have a So if you wanted to pass your own |
@skeet70 I made a few changes here and uploaded a few more scenarios in https://github.com/heaths/azsdk-rust-2750. In I'd love to hear what you think. As for gzip (and deflate, though it appears none of our services support that while few support gzip), we're still debating whether to make that optional. It's easy enough to disable if you provide your own |
Resolves Azure#2750 and replaces Azure#2811 by removing the `reqwest_rustls` feature which was there only to enable `reqwest/rustls-tls-native-roots-no-provider`. But using resolver v2 and newer, dependents can already do that by just taking a dependency on `reqwest` themselves to add that feature. In some cases, they may need to import `azure_core` with `default-features=false`, but passing a different `reqwest::Client` via our `TransportOptions` is easy. Not only will this scale better, but should help avoid opinionated feature selection that adversely impacts customers as with Azure#2750.
93669cd
to
ad4cd8a
Compare
Removing all of the reqwest pass-through features from the user-focused crates is a significantly positive step forward. It always felt clunky to me. I appreciate the mechanism to provide alternate features into reqwest. This gives users a path forward to use rustls-symcrypt without requiring changes across all of the SDK. Specifying native-tls as a default paved-path, while enabling symcrypt, enables 1st party teams to that are working with the symcrypt team with minimal changes. |
@demoray I'm still contemplating - and have a separate tracking issue open for a decision - keeping |
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.
Pull Request Overview
This PR removes the reqwest_rustls
feature that was originally added to enable reqwest/rustls-tls-native-roots-no-provider
. The change leverages Cargo resolver v2 capabilities, allowing dependents to configure their own reqwest features directly rather than through Azure SDK feature flags. This reduces opinionated feature selection and allows for better scaling as customers can customize their TLS configuration independently.
Key Changes
- Removed
reqwest_rustls
feature from all crates - Added
reqwest_native_tls
as a new default feature to maintain existing TLS behavior - Updated feature dependencies to properly chain reqwest-related features
Reviewed Changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
sdk/typespec/typespec_client_core/src/http/clients/mod.rs |
Simplified conditional compilation by removing reqwest_rustls feature checks |
sdk/typespec/typespec_client_core/README.md |
Updated feature documentation to remove reqwest_rustls and add reqwest_native-tls |
sdk/typespec/typespec_client_core/Cargo.toml |
Restructured features to remove reqwest_rustls and add proper feature chaining |
sdk/core/azure_core/Cargo.toml |
Updated feature definitions and dependencies to use reqwest_native_tls |
Multiple SDK crates | Added explicit default features to inherit from azure_core/default |
Cargo.toml |
Set workspace azure_core dependency to default-features = false |
Resolves #2750 and replaces #2811 by removing the
reqwest_rustls
feature which was there only to enablereqwest/rustls-tls-native-roots-no-provider
. But using resolver v2 and newer, dependents can already do that by just taking a dependency onreqwest
themselves to add that feature. In some cases, they may need to importazure_core
withdefault-features=false
, but passing a differentreqwest::Client
via ourTransportOptions
is easy.Not only will this scale better, but should help avoid opinionated feature selection that adversely impacts customers as with #2750.