feat(network): Add network context manager #367

mloesch · 2023-07-05T12:58:51Z

This PR adds a Network helper class that allows to create networks and connect containers programmatically.

The networks are context-managed resources like containers created via DockerContainer.

Please also see tests for a usage example :)

VerdantForge · 2023-11-10T09:36:52Z

I'm interested in this feature. But I see progress has been dropped. Is there a reason?

mloesch · 2023-11-10T09:43:57Z

I'm interested in this feature. But I see progress has been dropped. Is there a reason?

The build is failing due to cython/cython#4568

I created #376 to fix it, but it hasn't been approved yet.

kiview · 2023-11-24T19:21:32Z

Hey @mloesch, thanks for providing this PR for the feature. I see you decided to allow specifying a name for the Network. In Testcontainers for Java, we made the design decision to only generate networks with random names, to avoid the potential for naming conflicts with existing networks. This is a general design approach of Testcontainers and it should be fine for the Testcontainers use cases.

Would you like to adapt the implementation accordingly?

See https://github.com/testcontainers/testcontainers-java/blob/1a3b23385f9a595576246a4c7f5dc54d244dd69d/core/src/main/java/org/testcontainers/containers/Network.java for the Java implementation.

mloesch · 2023-11-27T10:51:50Z

Hey @kiview, thanks for the review, I changed the implementation to use random names in c10223f

kiview

Thanks @mloesch, that change for the random id looks good. I did another pass and compared it with the implementation in tc-java (which I consider our reference implementation) and had some more suggestions.

I am still getting up to speed with the internals of tc-python, so we can discuss whether my suggestions make sense in this context.

kiview · 2023-11-28T12:43:57Z

core/tests/test_network.py

+
+def test_containers_can_communicate_over_network():
+    with Network() as network:
+        with DockerContainer("nginx:alpine-slim").with_name(


Instead of using with_name (which can lead to naming conflicts when parallelizing tests or accidentally reusing names), can we add an API with_network_aliases(string[]), similar to tc-java, that uses Docker's --network-alias mechanism?

@kiview thanks for the pointer! Added this in c757142

kiview · 2023-11-28T12:45:24Z

core/tests/test_network.py

+def test_containers_can_communicate_over_network():
+    with Network() as network:
+        with DockerContainer("nginx:alpine-slim").with_name(
+                "alpine1").with_kwargs(network=network.name) as alpine1:


Does it make sense to use with_kwargs here and not have an explicit API with_network(Network)?

👍 added in e4d9c19

mloesch · 2024-02-06T09:18:04Z

@kiview is there anything else required for this PR to be merged? :)

santi

I went through your PR now and have some final remarks that should be addressed before we are ready to merge. Very good work so far, this is feature is awesome! 🚀

core/testcontainers/core/container.py

santi · 2024-04-04T10:37:17Z

core/testcontainers/core/container.py

@@ -42,6 +43,8 @@ def __init__(
        self._container = None
        self._command = None
        self._name = None
+        self._network: Optional[Network] = None
+        self._network_aliases: Optional[list] = None


Could you further specify the list to list[str]?

fixed in cac19d3

santi · 2024-04-04T10:38:21Z

core/testcontainers/core/container.py

@@ -57,6 +60,14 @@ def with_exposed_ports(self, *ports: int) -> "DockerContainer":
            self.ports[port] = None
        return self

+    def with_network(self, network: Network) -> "DockerContainer":


Latest version of this file has return type Self from typing_extensions for these methods. Could you sync it with main and update the signature to use Self?

fixed in 1c8ebdc

santi · 2024-04-04T10:40:19Z

core/testcontainers/core/network.py

+    Network context manager for programmatically connecting containers.
+    """
+
+    def __init__(self, docker_client_kw: Optional[dict] = None, **kwargs) -> None:


From an API users' point of view, it is not really clear what the kwargs arguments are meant for. Could you update the signature to be def __init__(self, docker_client_kw: Optional[dict] = None, docker_network_kw: Optional[dict] = None) -> None:?

fixed in b392ca6

santi · 2024-04-04T12:24:08Z

core/testcontainers/core/network.py

+    def __exit__(self, exc_type, exc_val, exc_tb) -> None:
+        self.remove()
+
+    def __del__(self) -> None:


We've had some bad experiences with external resource cleanup through the __del__ method, so we have decided to move away from this. Having the context manager and the remove method provides sufficient means to cleanup the resource. __del__ hook can be removed 👍

fixed in 8c8b6e2

santi · 2024-04-04T12:24:49Z

core/tests/test_network.py

+from testcontainers.core.docker_client import DockerClient
+from testcontainers.core.network import Network
+
+NGINX_ALPINE_SLIM_IMAGE = "nginx:alpine-slim"


Can you pin this dpenendency to a specific, versioned tag? We are currently working on making our tests more reproducible, and specific version tags for images used in tests are a part of this.

fixed in 9db1080

…estcontainers#529) Changed pre-commit config. Some files became re-formatted.

…#366) Add support for a Vault container.

…ners#532) see testcontainers#531: I am using testcontainers within a library that provides some pytest-fixtures. In order for this to work I have change some settings. As I can not guarantee that that my lib is imported before testcontainers I need to monkeypatch the settings. This is much easier if I only need to monkeypatch the config file and not all modules that use configurations. I would argue that for a potential library as this, this is a better design. Also one can easier see that the given UPERCASE variable is not a constant but rather a setting. Co-authored-by: Carli* Freudenberg <[email protected]>

🤖 I have created a release *beep* *boop* --- ## [4.3.2](testcontainers/testcontainers-python@testcontainers-v4.3.1...testcontainers-v4.3.2) (2024-04-08) ### Bug Fixes * **core:** Improve typing for common container usage scenarios ([testcontainers#523](testcontainers#523)) ([d5b8553](testcontainers@d5b8553)) * **core:** make config editable to avoid monkeypatching.1 ([testcontainers#532](testcontainers#532)) ([3be6da3](testcontainers@3be6da3)) * **vault:** add support for HashiCorp Vault container ([testcontainers#366](testcontainers#366)) ([1326278](testcontainers@1326278)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

santi · 2024-04-16T10:14:34Z

Thank you for this @mloesch! 🙌 Really appreciate your contribution, this will benefit a lot of users 💯

🤖 I have created a release *beep* *boop* --- ## [4.4.0](testcontainers-v4.3.3...testcontainers-v4.4.0) (2024-04-17) ### Features * **labels:** Add common testcontainers labels ([#519](#519)) ([e04b7ac](e04b7ac)) * **network:** Add network context manager ([#367](#367)) ([11964de](11964de)) ### Bug Fixes * **core:** [#486](#486) for colima delay for port avail for connect ([#543](#543)) ([90bb780](90bb780)) * **core:** add TESTCONTAINERS_HOST_OVERRIDE as alternative to TC_HOST ([#384](#384)) ([8073874](8073874)) * **dependencies:** remove usage of `sqlalchemy` in DB extras. Add default wait timeout for `wait_for_logs` ([#525](#525)) ([fefb9d0](fefb9d0)) * tests for Kafka container running on ARM64 CPU ([#536](#536)) ([29b5179](29b5179)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Add network context manager

9c3ecc8

mloesch force-pushed the network branch from 1ba3a11 to 9c3ecc8 Compare July 25, 2023 06:50

SergeyPirogov approved these changes Jul 27, 2023

View reviewed changes

github-actions bot added the 👀 requires attention label Nov 10, 2023

Merge branch 'main' into network

97ad2b8

Use random names for networks

c10223f

Merge branch 'main' into network

7542743

kiview reviewed Nov 28, 2023

View reviewed changes

mloesch and others added 4 commits November 28, 2023 18:03

Add explicit API DockerContainer.with_network(Network)

e4d9c19

Add support for network aliases

c757142

Merge remote-tracking branch 'origin/main' into network

b076309

Merge branch 'main' into network

8c2f8b9

alexanderankin added feat and removed 👀 requires attention labels Mar 9, 2024

Merge remote-tracking branch 'origin/main' into network

b523301

alexanderankin changed the title ~~Add network context manager~~ feat: Add network context manager Mar 20, 2024

santi reviewed Apr 4, 2024

View reviewed changes

mloesch and others added 8 commits April 4, 2024 17:38

Merge remote-tracking branch 'origin/main' into network

9093c6b

chore: change return type to Self

1c8ebdc

chore: further specify type

cac19d3

chore: clarify Network init signature

b392ca6

chore: remove __del__hook

8c8b6e2

chore(codestyle): switch to ruff from black for code formatting (t…

a64625c

…estcontainers#529) Changed pre-commit config. Some files became re-formatted.

fix(vault): add support for HashiCorp Vault container (testcontainers…

a00d8dd

…#366) Add support for a Vault container.

github-actions bot and others added 3 commits April 8, 2024 11:39

chore: pin nginx image

9db1080

fix class style

9426254

mloesch force-pushed the network branch from ba8f8ca to 9426254 Compare April 8, 2024 09:46

Merge remote-tracking branch 'origin/main' into network

3fc6bcf

santi changed the title ~~feat: Add network context manager~~ feat(network): Add network context manager Apr 15, 2024

santi and others added 2 commits April 15, 2024 19:03

Merge branch 'main' into network

66e2584

chore: reformat

2e50cb3

santi merged commit 11964de into testcontainers:main Apr 16, 2024
7 checks passed

github-actions bot mentioned this pull request Apr 16, 2024

chore(main): release testcontainers 4.4.0 #539

Merged

feat(network): Add network context manager #367

feat(network): Add network context manager #367

Uh oh!

Conversation

mloesch commented Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VerdantForge commented Nov 10, 2023

Uh oh!

mloesch commented Nov 10, 2023

Uh oh!

kiview commented Nov 24, 2023

Uh oh!

mloesch commented Nov 27, 2023

Uh oh!

kiview left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mloesch commented Feb 6, 2024

Uh oh!

santi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

santi commented Apr 16, 2024

Uh oh!

Uh oh!

Uh oh!

mloesch commented Jul 5, 2023 •

edited

Loading