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

fix: config support parse tls #446

Merged
merged 6 commits into from
Jul 8, 2024
Merged

Conversation

TCeason
Copy link
Collaborator

@TCeason TCeason commented Jul 3, 2024

Summary

  • On local: if add --tls will return err:

➜ ~/database/source-code/bendsql👆 git:(main) ⚡ ./target/debug/bendsql --tls=true
path is "/home/taichong/.config/bendsql/config.toml"
not set dsn : databend://root@localhost:8000?tls=false
Welcome to BendSQL 0.18.3-fbb976e(2024-07-03T13:28:17.492814078Z).
Connecting to localhost:8000 as user root.
Error: APIError: RequestError: error sending request for url (https://localhost:8000/v1/query)

  • If add it in config.toml, now it also return err:

And I think the doc in README.md is wrong. That is right.

➜  ~/database/source-code/bendsql👆 git:(main) ⚡ cat  /home/taichong/.config/bendsql/config.toml
[connection]
host = "127.0.0.1"

[connection.args]
tls = "true"
connect_timeout = "30"


[settings]
display_pretty_sql = true
progress_color = "green"
no_auto_complete = true
prompt = ":) "

➜ ~/database/source-code/bendsql👆 git:(main) ⚡ ./target/debug/bendsql
path is "/home/taichong/.config/bendsql/config.toml"
not set dsn : databend://root@localhost:8000?tls=true
Welcome to BendSQL 0.18.3-fbb976e(2024-07-03T13:28:17.492814078Z).
Connecting to localhost:8000 as user root.
Error: APIError: RequestError: error sending request for url (https://localhost:8000/v1/query)

Fix: #440

@TCeason
Copy link
Collaborator Author

TCeason commented Jul 3, 2024

cc @sundy-li

@TCeason TCeason force-pushed the fix_440 branch 3 times, most recently from c2945de to fcfe0f6 Compare July 3, 2024 16:12
README.md Outdated Show resolved Hide resolved
@TCeason TCeason requested a review from everpcpc July 4, 2024 01:53
Copy link
Member

@everpcpc everpcpc left a comment

Choose a reason for hiding this comment

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

connection.args is designed to be equivalent to arguments in DSN, so it is a map<string, string>, tls is not included.
Maybe it should be

[connection]
tls = true

[connection.args]
...

or

[connection]
...

[connection.args]
sslmode = "disable"

@TCeason
Copy link
Collaborator Author

TCeason commented Jul 4, 2024

[connection]
tls = true

[connection.args]
...

Good idea. Fixed. Please review the newest commit.

[connection]
tls = true

[connection.args]
...

@everpcpc everpcpc merged commit 0abc4df into databendlabs:main Jul 8, 2024
7 checks passed
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.

Add connection.tls config option
3 participants