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

small suggestion - port int coercion in get_client #395

Closed
vicentesurraco opened this issue Sep 10, 2024 · 1 comment · Fixed by #400
Closed

small suggestion - port int coercion in get_client #395

vicentesurraco opened this issue Sep 10, 2024 · 1 comment · Fixed by #400
Labels
enhancement New feature or request

Comments

@vicentesurraco
Copy link

vicentesurraco commented Sep 10, 2024

I was struggling to figure out why I could connect to clickhouse everywhere except clickhouse-client. It turned out the port in my env was being read as a string and it must be passed in as an int to get_client if the port is 443 or 8443. This is because use_tls returns False when comparing "8443" to 8443. Then the interface is set to http instead of https and get_client responds with ConnectionResetError: [Errno 54] Connection reset by peer. It would have been nice if it coerced the port to an int (or at least thrown a clearer error).

The following change would fix this issue (wrapping port with int())

use_tls = str(secure).lower() == 'true' or interface == 'https' or (not interface and int(port) in (443, 8443))

Figured I might as well suggest this in case it helps someone in the future.

@genzgd genzgd added the enhancement New feature or request label Sep 10, 2024
@genzgd
Copy link
Collaborator

genzgd commented Sep 10, 2024

I agree with the suggestion but you could also have set the interface to https explicitly.

@genzgd genzgd mentioned this issue Sep 26, 2024
2 tasks
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 a pull request may close this issue.

2 participants