refactor(core): Standardize configuration and readiness steps in container lifecycle #527

santi · 2024-04-04T08:08:16Z

Aims for this PR:

Include _configure() and _wait_until_ready() hooks in base DockerContainer image to be used by all container implementations in need of a configuration and readiness step.
Standardize method naming for getting a connection string from a container, where a popular client library expects a standardized URL format for connecting to the service provided by the container (E.g. a database URL for DB containers)
Remove DbContainer class, as the two points above will render it redundant.

santi · 2024-04-04T08:37:46Z

Referencing #459 for context

alexanderankin · 2024-04-06T11:36:51Z

would this have the potential to break existing code that depends on conventions from v3 and v4?

…ault wait timeout for `wait_for_logs` (#525) Removes usage of `sqlalchemy`, as part of the work described in #526. - Adds default timeout to the `wait_for_logs` waiting strategy, the same timeout used by default in the `wait_container_is_ready` strategy. - Changes wait strategy for `mysql` container to wait for logs indicating that the DB engine is ready to accept connections (MySQL performs a restart as part of its startup procedure, so the logs will always appear twice. - Add More tests for different `mysql` and `mariadb` versions to ensure consistency in wait strategy. - Remove x86 emulation for ARM devices for MariaDB, as it MariaDB images support ARM architectures already. - Change wait strategy for `oracle-free`, as the images produce a consistent `DATABASE IS READY TO USE!` log message on startup. Next steps will be to remove `sqlalchemy` as a bundled dependency entirely, but I have not included it in this PR as I consider it a bigger change than just changing wait strategies as an internal implementation detail. I plan to do this as part of a bigger rework where i remove the `DbContainer` class and standardize configuration hooks and wait strategies across containers (not just DB containers, all containers in need of a configuration and readiness step). See #527 for WIP. --------- Co-authored-by: David Ankin <daveankin@gmail.com>

jankatins · 2024-04-26T08:25:44Z

core/testcontainers/core/utils.py

+    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


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.

I would actually split this into a generic "create_connection_url" (scheme instead of dialect and driver, path instead of dbname) and one call (create_db_api_connection_url?) which takes db api inputs (like the current one) and calls create_connection_url with the corresponding arguments.

jankatins · 2024-04-26T08:37:35Z

core/testcontainers/core/utils.py

@@ -53,6 +54,43 @@ def inside_container() -> bool:
    return os.path.exists("/.dockerenv")


+def create_connection_string(


create_connection_url -> e.g. you couldn't create a kafka one with this, couldn't you?

jankatins · 2024-04-26T08:40:06Z

core/testcontainers/core/utils.py

@@ -53,6 +54,43 @@ def inside_container() -> bool:
    return os.path.exists("/.dockerenv")


+def create_connection_string(
+    dialect: str,


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.

jankatins · 2024-04-26T08:44:46Z

modules/azurite/testcontainers/azurite/__init__.py

            ...   client = BlobServiceClient.from_connection_string(
-            ...        connection_string,
+            ...        azurite_container.get_connection_string(),


For ArangoDbContainer is get_conenction_url`, here it's '*_string'. Should we unify that?

jankatins · 2024-04-26T08:46:48Z

modules/azurite/testcontainers/azurite/__init__.py

    @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))))))


Might it make sense to also pull this into utils as a wait_for_port? At least I had the need in my code to basically write something similar:

@wait_container_is_ready(NoSuchPortExposed) def _wait_for_port() -> None: zks.get_service_port("schemaregistry", 8085) _wait_for_port()

jankatins · 2024-04-26T09:03:52Z

modules/influxdb/testcontainers/influxdb.py

-
-        return response.json()
-
-    def get_influxdb_version(self) -> str:


Should this function be kept? It's seems it has value outside the normal startup procedure?

jankatins · 2024-04-26T09:05:20Z

modules/influxdb/tests/test_influxdb.py

    ],
 )
 def test_influxdbcontainer_get_influxdb_version(
    image: str, influxdb_container_class: Type[InfluxDbContainer], expected_version: str
 ):
    with influxdb_container_class(image) as influxdb_container:
-        assert influxdb_container.get_influxdb_version().startswith(expected_version)
+        version = get(f"{influxdb_container.get_url()}/health").json()["version"]
+        assert version == expected_version


This test implies that influxdb_container.get_influxdb_version() is API?

jankatins · 2024-04-26T09:11:27Z

modules/mssql/testcontainers/mssql/__init__.py

@@ -43,13 +54,29 @@ def __init__(
        self.dbname = dbname
        self.dialect = dialect

+    @wait_container_is_ready(*ADDITIONAL_TRANSIENT_ERRORS)
+    def _wait_until_ready(self):
+        import sqlalchemy


This will import sqla and fail the tests if it is not available. Which is something we just removed form PG and kafka. Is there no way to check for a port with nc or so?

microsoft/mssql-docker#133 has some interesting suggestion for a healthcheck

See fefb9d0 where it is already removed?

jankatins · 2024-04-26T09:24:50Z

core/testcontainers/core/container.py

+
+    def _wait_until_ready(self) -> None:
+        pass
+
    def start(self):


Suggested change

def start(self):

def start(self) -> Self:

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.

alexanderankin · 2024-05-14T07:43:13Z

if i were to try to merge this, what should i keep in mind @santi ? Anything not already reflected here? I did a first pass and seems like i will have to revisit this several times in order to make sense of this fully...

also, like i asked above, is this breaking at all?

santi · 2024-05-20T20:15:54Z

@alexanderankin Hey, sorry for the delay on this one. This is just a WIP draft that is not ready for a merge quite yet, and has probably drifted a bit from the main branch. I intend on getting this one up-to-date this week, and do a nice write-up on the implications and document the intention behind the changes / API standardisation it introduces.

santi added 20 commits April 2, 2024 14:43

Include configuration and waiting hooks in base DockerContainer class

fc7c466

Inherit directly from DockerContainer in ArangoDB module

2578a97

Use new _wait_until_ready hook in Azureite module

fa0d34a

Use new _wait_until_ready hook in Cassandra module

482df60

Add http waiting strategy for basic HTTP endpoint status codes

9437d89

Use new _wait_until_ready hook in ChromaDB module

d4a0612

Create utility for generating db client compatible connection strings

b7e52ba

Use new _wait_until_ready hook in clickhouse module

6964d3e

Use new _wait_until_ready hook in elasticsearch module

9e43875

Use new _wait_until_ready hook in google module

1b3aebc

Fix case for connection_string without username or password

abbbf7d

Use new _wait_until_ready hook in influxdb module

09c10ba

Use new _wait_until_ready hook in k3s module

bea1372

Use new _wait_until_ready hook in kafka module

d19f6c9

Use new _wait_until_ready hook in keycloak module

124727c

Use new _wait_until_ready hook in localstack module

1831366

Use new _wait_until_ready hook in minio module

0688858

Use new _wait_until_ready hook in mongodb module

e5afe88

Reorder imports for linting

20aead1

Use new _wait_until_ready hook in mssql module

00b2e7f

santi mentioned this pull request Apr 4, 2024

fix(dependencies): remove usage of sqlalchemy in DB extras. Add default wait timeout for wait_for_logs #525

Merged

alexanderankin mentioned this pull request Apr 8, 2024

fix(core): make config editable to avoid monkeypatching.1 #532

Merged

jankatins reviewed Apr 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): Standardize configuration and readiness steps in container lifecycle #527

refactor(core): Standardize configuration and readiness steps in container lifecycle #527

santi commented Apr 4, 2024

santi commented Apr 4, 2024

alexanderankin commented Apr 6, 2024

jankatins Apr 26, 2024 •

edited

jankatins Apr 26, 2024

This comment was marked as resolved.

This comment was marked as resolved.

jankatins Apr 26, 2024

jankatins Apr 26, 2024

jankatins Apr 26, 2024

jankatins Apr 26, 2024

jankatins Apr 26, 2024

jankatins Apr 26, 2024

jankatins Apr 26, 2024

jankatins Apr 26, 2024

jankatins Apr 26, 2024

alexanderankin commented May 14, 2024

santi commented May 20, 2024

		@@ -53,6 +54,43 @@ def inside_container() -> bool:
		return os.path.exists("/.dockerenv")


		def create_connection_string(


		return response.json()

		def get_influxdb_version(self) -> str:

refactor(core): Standardize configuration and readiness steps in container lifecycle #527

Are you sure you want to change the base?

refactor(core): Standardize configuration and readiness steps in container lifecycle #527

Conversation

santi commented Apr 4, 2024

santi commented Apr 4, 2024

alexanderankin commented Apr 6, 2024

jankatins Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderankin commented May 14, 2024

santi commented May 20, 2024

jankatins Apr 26, 2024 •

edited