-
Notifications
You must be signed in to change notification settings - Fork 281
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
Enable using clickhouse HTTP interface #319
base: main
Are you sure you want to change the base?
Conversation
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 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see there have been some changes to requirements management. Is there anything I should do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve error message for missing requirements (cf. #319).
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.