Enable using clickhouse HTTP interface #319
Conversation
pofl
changed the title
No, you aren't annoying. We really appreciate pull-requests from the community but you should understand we try to maintain this repo besides our full-time jobs and private life. So that is why there are some delays. |
|
I think it is fine to keep everything related to Clickhouse inside one Container class. |
Co-authored-by: Pepijn Fijt <pffijt@gmail.com>
|
I'm happy with the PR too. Feel free to merge, as I can't. Thanks for reviewing ❤️ |
|
Thanks for the update. Would you be able to rebase on |
| def test_clickhouse_http_interface(): | ||
| with ClickHouseContainer(port=8123) as clickhouse: | ||
| client = clickhouse_connect.get_client(dsn=clickhouse.get_connection_url()) | ||
| result = client.query("select 'working'").result_rows | ||
| assert result == [("working",)] |
There was a problem hiding this comment.
I have previously not noticed this test file. I have extended it with the new HTTP interface. However, I would actually vote for dropping this test file because it adds no value when the doctests in the other file exist.
There was a problem hiding this comment.
Yeah, good question. We've got some duplicate code across the documentation and in the tests, in part to keep track of test coverage. If we could also run the doctests through pytest with test coverage, that'd be great. It sounds like it's possible, but I haven't looked into it.
|
@tillahoffmann @pffijt From my perspective this PR is done. Feel free to rerun the CI and merge on success (I can do neither of these things) 👍 Btw, while merging in master I stumbled across this issue: #335 |
|
I just merged in master once more. |
| @@ -12,6 +12,7 @@ | |||
| url="https://github.com/testcontainers/testcontainers-python", | |||
| install_requires=[ | |||
| "testcontainers-core", | |||
| "clickhouse-connect", | |||
There was a problem hiding this comment.
Given the updated requirements, let's update the lock files. Could you run make requirements from the root directory and push the changes?
There was a problem hiding this comment.
I see there have been some changes to requirements management. Is there anything I should do?
There was a problem hiding this comment.
You can run python get_requirements.py --pr=[your PR number] to update the requirements. Full details here. You may have to rebase on main and wait for CI to run before the script will work.
There was a problem hiding this comment.
I am getting an error
$ python get_requirements.py --pr=319
we need a GitHub access token to fetch the requirements; please visit https://github.com/settings/tokens/new, create a token with `public_repo` scope, and paste it here: ----------------------------
do you want to cache the token in a `.github-token` file [Ny]? N
fetching most recent commit for PR #319
Traceback (most recent call last):
File "/home/pofl/testcontainers-python/get_requirements.py", line 94, in <module>
__main__()
File "/home/pofl/testcontainers-python/get_requirements.py", line 65, in __main__
raise RuntimeError(f"could not identify unique workflow run: {runs}")
RuntimeError: could not identify unique workflow run: []There was a problem hiding this comment.
Yeah, the CI needs to have completed first, and GitHub doesn't allow us to start the run automatically for first-time contributors. I've started it now. We should probably also improve that error message.
| self.dbname = dbname or os.environ.get("CLICKHOUSE_DB", "test") | ||
| self.username: str = username or os.environ.get("CLICKHOUSE_USER", "test") | ||
| self.password: str = password or os.environ.get("CLICKHOUSE_PASSWORD", "test") | ||
| self.dbname: str = dbname or os.environ.get("CLICKHOUSE_DB", "test") | ||
| self.port = port |
There was a problem hiding this comment.
Would it make sense to just expose both ports? Then the user can connect with clickhouse_driver, clickhouse_connect or both at the same time?
There was a problem hiding this comment.
Yes that would make sense. Can you advise me on code style: How would you express that as a __init__ function signature with a default arg? The only thing I can come up with is changing the port type to list of int but I'm worried about backwards-compatibility.
Improve error message for missing requirements (cf. #319).
alexanderankin
added
the
community-feat
ClickHouse provides both a TCP connection interface and a HTTP API. The existing implementation of the CH testcontainer was solely based on the TCP interface. However, the HTTP interface is the recommended interface to use. The official clickhouse Python client library uses the HTTP interface.
IMO the Clickhouse testcontainer should expose the HTTP interface by default. However, that would be a breaking change, so I implement the change as optional. Let me know what you think.
It's also worth considering adding a new Testcontainer class
class ClickHouseHTTPContainer(DbContainer):Instead of accepting this PR.