Skip to content

Add the native-tls-vendored feature to the sdk#526

Merged
labbott merged 1 commit intomainfrom
add_native_tls
Jan 22, 2024
Merged

Add the native-tls-vendored feature to the sdk#526
labbott merged 1 commit intomainfrom
add_native_tls

Conversation

@labbott
Copy link
Copy Markdown
Contributor

@labbott labbott commented Jan 18, 2024

Needed for reqwest

@labbott labbott requested a review from ahl January 18, 2024 19:55
@ahl
Copy link
Copy Markdown
Collaborator

ahl commented Jan 19, 2024

Why would we put this in the SDK rather than letting consumers of the SDK choose?

@labbott
Copy link
Copy Markdown
Contributor Author

labbott commented Jan 19, 2024

Why would we put this in the SDK rather than letting consumers of the SDK choose?

We use danger_accept_invalid_hostnames and danger_accept_invalid_certs as part of our client_builder in the sdk which are behind this feature (https://docs.rs/reqwest/latest/reqwest/struct.ClientBuilder.html#method.danger_accept_invalid_certs), without this feature the sdk fails to compile when used in the SDK consumer. It seems better to just have the SDK select it.

Copy link
Copy Markdown
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

seems good. we might want to rethink the public interface of the sdk; I think this exposes some of the rough edges between the sdk and cli

@labbott
Copy link
Copy Markdown
Contributor Author

labbott commented Jan 22, 2024

seems good. we might want to rethink the public interface of the sdk; I think this exposes some of the rough edges between the sdk and cli

Agreed 100%

@labbott labbott merged commit 334a03b into main Jan 22, 2024
@labbott labbott deleted the add_native_tls branch January 22, 2024 14:51
@inickles inickles mentioned this pull request Apr 23, 2026
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.

2 participants