-
Notifications
You must be signed in to change notification settings - Fork 297
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
refactor(core): Standardize configuration and readiness steps in container lifecycle #527
base: main
Are you sure you want to change the base?
Changes from all commits
fc7c466
2578a97
fa0d34a
482df60
9437d89
d4a0612
b7e52ba
6964d3e
9e43875
1b3aebc
abbbf7d
09c10ba
bea1372
d19f6c9
124727c
1831366
0688858
e5afe88
20aead1
00b2e7f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
import platform | ||
import subprocess | ||
import sys | ||
from typing import Optional, Union | ||
|
||
LINUX = "linux" | ||
MAC = "mac" | ||
|
@@ -53,6 +54,43 @@ def inside_container() -> bool: | |
return os.path.exists("/.dockerenv") | ||
|
||
|
||
def create_connection_string( | ||
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. create_connection_url -> e.g. you couldn't create a kafka one with this, couldn't you? |
||
dialect: str, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this would be DB API only, I would say "dialect" is fine, but if this should also be used to generate http URLs, I would call this base_scheme. |
||
host: str, | ||
port: Union[str, int, None] = None, | ||
username: Optional[str] = None, | ||
password: Optional[str] = None, | ||
driver: Optional[str] = None, | ||
dbname: Optional[str] = None, | ||
) -> str: | ||
""" | ||
Returns a connection URL following the RFC-1738 format. | ||
Compatible with database clients such as SQLAlchemy and other popular database client libraries. | ||
|
||
Example: postgres+psycopg2://myuser:mypassword@localhost:5432/mytestdb | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to document that setting driver to None or "" will remove the driver from the URL (psycopg v3 does need a url without dialect and any regular http URL also needs that :-)) Or at least add an example without driver. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would actually split this into a generic "create_connection_url" ( |
||
""" | ||
dialect_driver = dialect | ||
if driver: | ||
dialect_driver += f"+{driver}" | ||
|
||
username_password = username if username else "" | ||
if password: | ||
username_password += f":{password}" | ||
|
||
if username_password: | ||
username_password += "@" | ||
|
||
host_port = host | ||
if port: | ||
host_port += f":{port}" | ||
|
||
connection_string = f"{dialect_driver}://{username_password}{host_port}" | ||
if dbname: | ||
connection_string += f"/{dbname}" | ||
|
||
return connection_string | ||
|
||
|
||
def default_gateway_ip() -> str: | ||
""" | ||
Returns gateway IP address of the host that testcontainer process is | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,10 +33,9 @@ class AzuriteContainer(DockerContainer): | |
>>> from testcontainers.azurite import AzuriteContainer | ||
>>> from azure.storage.blob import BlobServiceClient | ||
|
||
>>> with AzuriteContainer() as azurite_container: | ||
... connection_string = azurite_container.get_connection_string() | ||
>>> with AzuriteContainer("mcr.microsoft.com/azure-storage/azurite:3.29.0") as azurite_container: | ||
... client = BlobServiceClient.from_connection_string( | ||
... connection_string, | ||
... azurite_container.get_connection_string(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For |
||
... api_version="2019-12-12" | ||
... ) | ||
""" | ||
|
@@ -102,12 +101,7 @@ def get_connection_string(self) -> str: | |
|
||
return connection_string | ||
|
||
def start(self) -> "AzuriteContainer": | ||
super().start() | ||
self._connect() | ||
return self | ||
|
||
@wait_container_is_ready(OSError) | ||
def _connect(self) -> None: | ||
def _wait_until_ready(self) -> None: | ||
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: | ||
s.connect((self.get_container_host_ip(), int(self.get_exposed_port(next(iter(self.ports)))))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might it make sense to also pull this into utils as a @wait_container_is_ready(NoSuchPortExposed)
def _wait_for_port() -> None:
zks.get_service_port("schemaregistry", 8085)
_wait_for_port() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,17 @@ | ||
import pytest | ||
|
||
from cassandra.cluster import Cluster, DCAwareRoundRobinPolicy | ||
|
||
from testcontainers.cassandra import CassandraContainer | ||
|
||
|
||
def test_docker_run_cassandra(): | ||
with CassandraContainer("cassandra:4.1.4") as cassandra: | ||
@pytest.mark.parametrize("version", ["4.1.4", "3.11.16"]) | ||
def test_docker_run_cassandra(version: str): | ||
with CassandraContainer(f"cassandra:{version}") as cassandra: | ||
cluster = Cluster( | ||
cassandra.get_contact_points(), | ||
load_balancing_policy=DCAwareRoundRobinPolicy(cassandra.get_local_datacenter()), | ||
) | ||
session = cluster.connect() | ||
result = session.execute("SELECT release_version FROM system.local;") | ||
assert result.one().release_version == "4.1.4" | ||
assert result.one().release_version == version |
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.
Not sure what version we target (so either whats in python or via an backport), but I saw that this PR removes a lot of overwritten
start()
methods which just do this to get the proper type passed out, so not doing this here will mean that typing will regress.