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

Initialize cluster via connection URL #24

Closed
selevit opened this issue Jun 28, 2024 · 5 comments
Closed

Initialize cluster via connection URL #24

selevit opened this issue Jun 28, 2024 · 5 comments

Comments

@selevit
Copy link

selevit commented Jun 28, 2024

Hi, and thanks for the great library!

Wanted to mention that it would be nice to be able to initialize the ClickhouseCluster class via a URL like clickhouse//:user:password@host.... The clickhouse_driver library that you use actually supports it via a Client.from_url static method.

Unfortunately, clickhouse_driver does not yet expose the "parse url" API, and I need to parse the URL manually to pass it to ClickhouseCluster.

@selevit selevit changed the title Initialize cluster via connection url Initialize cluster via connection URL Jun 28, 2024
zifter added a commit that referenced this issue Jul 1, 2024
)

* #24 Allow connection string for initialization of ClickhouseCluster and add ability to pass db url via command line

---------

Co-authored-by: Aleh Strakachuk <[email protected]>
@zifter
Copy link
Owner

zifter commented Jul 1, 2024

Hey,

Thank you for the idea.

I made the requested changes, #25

You can find the necessary changes in the latest release 0.7.0

@zifter zifter closed this as completed Jul 1, 2024
@selevit
Copy link
Author

selevit commented Jul 1, 2024

Hello, thanks a lot for implementing this!

There is a minor issue that I wanted to report. Now it's possible to pass a default database for migration into ClickhouseCluster (via a URL), but ClickhouseCluster.migrate requires db_name to be passed. Would be nice to make this parameter optional.

Because clickhouse_driver does not yet expose their URL parsing API (I also created a ticket in their issues).

It means that to supply db_name to ClickhouseCluster I need to parse the URL myself, i.e. reimplement this logic of Client.from_url. It doesn't make a lot of sense, since clickhouse-migrations supports the db URL now.

zifter added a commit that referenced this issue Jul 1, 2024
* Allow default db name (from db URL)

---------

Co-authored-by: Aleh Strakachuk <[email protected]>
@zifter
Copy link
Owner

zifter commented Jul 1, 2024

#26 is merged
@selevit can you check the latest release 0.7.1?

@selevit
Copy link
Author

selevit commented Jul 2, 2024

Big thanks for the quick fix!

It worked for me. Just a minor – IMO it looks a bit weird that it's required to pass the db name explicitly as None, like this.

cluster.migrate(
    db_name=None,  # use default db
    migration_path=MIGRATIONS_DIR,
    ...
)

@zifter
Copy link
Owner

zifter commented Jul 2, 2024

@selevit yes, looks weird, but I don't want to make breaking changes in API

zifter added a commit that referenced this issue Aug 15, 2024
* Allow default db name (from db URL)

---------

Co-authored-by: Aleh Strakachuk <[email protected]>
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

No branches or pull requests

2 participants