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

Added HTTPS support (bis) #54

Merged
merged 1 commit into from
Feb 19, 2023
Merged

Added HTTPS support (bis) #54

merged 1 commit into from
Feb 19, 2023

Conversation

bezze
Copy link
Contributor

@bezze bezze commented Dec 20, 2022

I took most of the code from #36 and wrapped it as suggested. Any reviews are welcome.

  • Added 'tls' as default feature
  • Added 'hyper-tls' as dependency for tls feature
  • Now when setting up the client, if compiled with 'tls' feature, the Client::with_url method checks for urls that start with 'https:' and creates an HttpsConnector for the hyper client

* Added 'tls' as default feature
* Added 'hyper-tls' as dependency for tls feature
* Now when setting up the client, if compiled with 'tls' feature, the
  'with_url' method checks for urls that start with 'https:' and creates
  an HttpsConnector for the hyper client
@loyd loyd mentioned this pull request Jan 3, 2023
Copy link
Collaborator

@loyd loyd left a comment

Choose a reason for hiding this comment

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

Sorry for the late response, there were health problems.

Also, check lints, please.

@@ -101,6 +104,16 @@ impl Client {
/// ```
pub fn with_url(mut self, url: impl Into<String>) -> Self {
self.url = url.into();

#[cfg(feature = "tls")]
if self.url.starts_with("https:") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

From hyper-tls docs:

By default this connector will use plain HTTP if the URL provided uses the HTTP scheme (eg: http://example.com/).

Thus, it seems possible to just use HttpsConnector instead HttpConnector inside impl Default for Client if the feature is enabled.

@gd87429
Copy link

gd87429 commented Jan 27, 2023

@bezze @loyd does this mean the driver in its current form supports only http?

@loyd
Copy link
Collaborator

loyd commented Jan 30, 2023

@gd87429, no, you can specify a custom HTTPS connector, see #27 for details.

This PR is about automatic detection without a boilerplate.

@loyd loyd merged commit 37b7fea into ClickHouse:master Feb 19, 2023
@loyd
Copy link
Collaborator

loyd commented Feb 19, 2023

I decided to merge it and fix comments on my own.

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