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.

Sign up for GitHub

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

Jump to bottom

Enable using clickhouse HTTP interface #319

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Enable using clickhouse HTTP interface #319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
4d2b49e
Add clickhouse-connect dependency
pofl Mar 12, 2023
c9a2dd0
Enable using HTTP port
pofl Mar 12, 2023
9a3c217
Adjust doctest
pofl Mar 12, 2023
de8502a
Repair import
pofl Mar 12, 2023
564ce66
Handle all connect errors
pofl Mar 12, 2023
cc4e563
Reformat doctest
pofl Mar 12, 2023
8682063
Revert some accidental changes
pofl Mar 12, 2023
04e999c
Add type
pofl Mar 12, 2023
4554473
Satisfy linter
pofl Mar 24, 2023
9008da5
Improve docstring
pofl Mar 24, 2023
b50984d
Update clickhouse/testcontainers/clickhouse/__init__.py
pofl Mar 24, 2023
21a7f00
Repair doctest
pofl Mar 24, 2023
c1761e2
Trim whitespace
pofl Mar 24, 2023
ac47799
Merge remote-tracking branch 'upstream/master' into patch-1
pofl Apr 12, 2023
66ccd27
Fix merge
pofl Apr 12, 2023
4932238
Add test for HTTP
pofl Apr 12, 2023
b851c6e
Merge branch 'master' into patch-1
pofl Apr 13, 2023
8e097c6
Merge remote-tracking branch 'upstream/main' into patch-1
pofl May 8, 2023
e06bbdd
Update requirements
pofl May 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions clickhouse/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
url="https://github.com/testcontainers/testcontainers-python",
install_requires=[
"testcontainers-core",
"clickhouse-connect",
Copy link
Collaborator

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?

Copy link
Author

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?

Copy link
Collaborator

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.

Copy link
Author

@pofl pofl May 9, 2023 •
edited

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: []

Copy link
Collaborator

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.

"clickhouse-driver",
],
python_requires=">=3.7",
Expand Down
56 changes: 41 additions & 15 deletions clickhouse/testcontainers/clickhouse/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,10 @@
import os
from typing import Optional

import clickhouse_connect
from clickhouse_connect.driver.exceptions import Error as ClickhouseConnectError
import clickhouse_driver
from clickhouse_driver.errors import Error
from clickhouse_driver.errors import Error as ClickhouseDriverError

from testcontainers.core.generic import DbContainer
from testcontainers.core.utils import raise_for_deprecated_parameter
Expand All @@ -23,38 +25,62 @@

class ClickHouseContainer(DbContainer):
"""
ClickHouse database container.
ClickHouse database container. This testcontainer defaults to exposing the TCP port of
ClickHouse. If you want to use the HTTP interface, specify port 8123 to be exposed.

Example:

The example spins up a ClickHouse database and connects to it using the
:code:`clickhouse-driver`.
This example shows how to spin up ClickHouse.
It demonstrates how to connect to the *TCP* interface using :code:`clickhouse-driver`
and how to connect to the *HTTP* interface using :code:`clickhouse-connect`, the
official client library.

.. doctest::

>>> import clickhouse_driver
>>> from testcontainers.clickhouse import ClickHouseContainer

>>> with ClickHouseContainer("clickhouse/clickhouse-server:21.8") as clickhouse:
>>> # clickhouse_driver is a client lib that uses the TCP interface
>>> import clickhouse_driver
>>> # ClickHouseContainer exports the TCP port by default
>>> with ClickHouseContainer(image="clickhouse/clickhouse-server:21.8") as clickhouse:
... client = clickhouse_driver.Client.from_url(clickhouse.get_connection_url())
... client.execute("select 'working'")
[('working',)]

>>> # clickhouse_connect is the official client lib, based on the HTTP interface
>>> import clickhouse_connect
>>> # If you want to use the HTTP interface, port 8123 needs to be exposed
>>> with ClickHouseContainer(port=8123) as clickhouse:
... client = clickhouse_connect.get_client(dsn=clickhouse.get_connection_url())
... client.query("select 'working'").result_rows
[('working',)]
pofl marked this conversation as resolved.
Show resolved Hide resolved
"""
def __init__(self, image: str = "clickhouse/clickhouse-server:latest", port: int = 9000,
username: Optional[str] = None, password: Optional[str] = None,
dbname: Optional[str] = None, **kwargs) -> None:

def __init__(
self,
image: str = "clickhouse/clickhouse-server:latest",
port: int = 9000,
username: Optional[str] = None,
password: Optional[str] = None,
dbname: Optional[str] = None,
**kwargs
) -> None:
raise_for_deprecated_parameter(kwargs, "user", "username")
super().__init__(image=image, **kwargs)
self.username = username or os.environ.get("CLICKHOUSE_USER", "test")
self.password = password or os.environ.get("CLICKHOUSE_PASSWORD", "test")
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
Copy link
Collaborator

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?

Copy link
Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could use something like http_port and tcp_port as for the azurite container (see here). You can use raise_for_deprecated_parameter (see here for an example). We can introduce a breaking change as the next release will be a major release.

self.with_exposed_ports(self.port)

@wait_container_is_ready(Error, EOFError)
@wait_container_is_ready(ClickhouseDriverError, ClickhouseConnectError, EOFError)
def _connect(self) -> None:
with clickhouse_driver.Client.from_url(self.get_connection_url()) as client:
client.execute("SELECT version()")
if self.port == 8123:
with clickhouse_connect.get_client(dsn=self.get_connection_url()) as client:
client.command("SELECT version()")
else:
with clickhouse_driver.Client.from_url(self.get_connection_url()) as client:
client.execute("SELECT version()")

def _configure(self) -> None:
self.with_env("CLICKHOUSE_USER", self.username)
Expand Down
14 changes: 10 additions & 4 deletions clickhouse/tests/test_clickhouse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
import clickhouse_connect
import clickhouse_driver
from testcontainers.clickhouse import ClickHouseContainer


def test_docker_run_clickhouse():
clickhouse_container = ClickHouseContainer()
with clickhouse_container as clickhouse:
def test_clickhouse_tcp_interface():
with ClickHouseContainer() as clickhouse:
client = clickhouse_driver.Client.from_url(clickhouse.get_connection_url())
result = client.execute("select 'working'")
assert result == [("working",)]

assert result == [('working',)]

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",)]
Comment on lines +13 to +17
Copy link
Author

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.

Copy link
Collaborator

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.

27 changes: 18 additions & 9 deletions requirements/macos-latest-3.10.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,16 +96,17 @@ bcrypt==4.0.1
# via paramiko
bleach==6.0.0
# via readme-renderer
boto3==1.26.129
boto3==1.26.130
# via testcontainers-localstack
botocore==1.29.129
botocore==1.29.130
# via
# boto3
# s3transfer
cachetools==5.3.0
# via google-auth
certifi==2022.12.7
certifi==2023.5.7
# via
# clickhouse-connect
# minio
# opensearch-py
# requests
Expand All @@ -116,6 +117,8 @@ cffi==1.15.1
# pynacl
charset-normalizer==3.1.0
# via requests
clickhouse-connect==0.5.23
# via testcontainers-clickhouse
clickhouse-driver==0.2.6
# via testcontainers-clickhouse
coverage[toml]==7.2.5
Expand All @@ -133,7 +136,7 @@ distro==1.8.0
# via docker-compose
dnspython==2.3.0
# via pymongo
docker[ssh]==6.1.0
docker[ssh]==6.1.1
# via
# docker-compose
# testcontainers-core
Expand Down Expand Up @@ -214,6 +217,8 @@ kafka-python==2.0.2
# via testcontainers-kafka
keyring==23.13.1
# via twine
lz4==4.3.2
# via clickhouse-connect
markdown-it-py==2.2.0
# via rich
markupsafe==2.1.2
Expand Down Expand Up @@ -250,7 +255,7 @@ pluggy==1.0.0
# via pytest
proto-plus==1.22.2
# via google-cloud-pubsub
protobuf==4.22.4
protobuf==4.23.0
# via
# google-api-core
# google-cloud-pubsub
Expand Down Expand Up @@ -309,10 +314,11 @@ python-dotenv==0.21.1
# via docker-compose
python-jose==3.3.0
# via python-keycloak
python-keycloak==2.16.1
python-keycloak==2.16.2
# via testcontainers-keycloak
pytz==2023.3
# via
# clickhouse-connect
# clickhouse-driver
# neo4j
pytz-deprecation-shim==0.1.0.post0
Expand All @@ -321,7 +327,7 @@ pyyaml==5.4.1
# via docker-compose
readme-renderer==37.3
# via twine
redis==4.5.4
redis==4.5.5
# via testcontainers-redis
requests==2.30.0
# via
Expand All @@ -335,7 +341,7 @@ requests==2.30.0
# requests-toolbelt
# sphinx
# twine
requests-toolbelt==0.10.1
requests-toolbelt==1.0.0
# via
# python-arango
# python-keycloak
Expand All @@ -352,7 +358,7 @@ s3transfer==0.6.1
# via boto3
scramp==1.4.4
# via pg8000
selenium==4.9.0
selenium==4.9.1
# via testcontainers-selenium
six==1.16.0
# via
Expand Down Expand Up @@ -418,6 +424,7 @@ tzlocal==4.3
urllib3[socks]==1.26.15
# via
# botocore
# clickhouse-connect
# docker
# minio
# opensearch-py
Expand All @@ -440,6 +447,8 @@ wsproto==1.2.0
# via trio-websocket
zipp==3.15.0
# via importlib-metadata
zstandard==0.21.0
# via clickhouse-connect

# The following packages are considered to be unsafe in a requirements file:
# setuptools
27 changes: 18 additions & 9 deletions requirements/ubuntu-latest-3.10.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,16 +96,17 @@ bcrypt==4.0.1
# via paramiko
bleach==6.0.0
# via readme-renderer
boto3==1.26.129
boto3==1.26.130
# via testcontainers-localstack
botocore==1.29.129
botocore==1.29.130
# via
# boto3
# s3transfer
cachetools==5.3.0
# via google-auth
certifi==2022.12.7
certifi==2023.5.7
# via
# clickhouse-connect
# minio
# opensearch-py
# requests
Expand All @@ -116,6 +117,8 @@ cffi==1.15.1
# pynacl
charset-normalizer==3.1.0
# via requests
clickhouse-connect==0.5.23
# via testcontainers-clickhouse
clickhouse-driver==0.2.6
# via testcontainers-clickhouse
coverage[toml]==7.2.5
Expand All @@ -134,7 +137,7 @@ distro==1.8.0
# via docker-compose
dnspython==2.3.0
# via pymongo
docker[ssh]==6.1.0
docker[ssh]==6.1.1
# via
# docker-compose
# testcontainers-core
Expand Down Expand Up @@ -219,6 +222,8 @@ kafka-python==2.0.2
# via testcontainers-kafka
keyring==23.13.1
# via twine
lz4==4.3.2
# via clickhouse-connect
markdown-it-py==2.2.0
# via rich
markupsafe==2.1.2
Expand Down Expand Up @@ -255,7 +260,7 @@ pluggy==1.0.0
# via pytest
proto-plus==1.22.2
# via google-cloud-pubsub
protobuf==4.22.4
protobuf==4.23.0
# via
# google-api-core
# google-cloud-pubsub
Expand Down Expand Up @@ -314,10 +319,11 @@ python-dotenv==0.21.1
# via docker-compose
python-jose==3.3.0
# via python-keycloak
python-keycloak==2.16.1
python-keycloak==2.16.2
# via testcontainers-keycloak
pytz==2023.3
# via
# clickhouse-connect
# clickhouse-driver
# neo4j
pytz-deprecation-shim==0.1.0.post0
Expand All @@ -326,7 +332,7 @@ pyyaml==5.4.1
# via docker-compose
readme-renderer==37.3
# via twine
redis==4.5.4
redis==4.5.5
# via testcontainers-redis
requests==2.30.0
# via
Expand All @@ -340,7 +346,7 @@ requests==2.30.0
# requests-toolbelt
# sphinx
# twine
requests-toolbelt==0.10.1
requests-toolbelt==1.0.0
# via
# python-arango
# python-keycloak
Expand All @@ -359,7 +365,7 @@ scramp==1.4.4
# via pg8000
secretstorage==3.3.3
# via keyring
selenium==4.9.0
selenium==4.9.1
# via testcontainers-selenium
six==1.16.0
# via
Expand Down Expand Up @@ -425,6 +431,7 @@ tzlocal==4.3
urllib3[socks]==1.26.15
# via
# botocore
# clickhouse-connect
# docker
# minio
# opensearch-py
Expand All @@ -447,6 +454,8 @@ wsproto==1.2.0
# via trio-websocket
zipp==3.15.0
# via importlib-metadata
zstandard==0.21.0
# via clickhouse-connect

# The following packages are considered to be unsafe in a requirements file:
# setuptools