feat(network): Add network context manager by mloesch · Pull Request #367 · testcontainers/testcontainers-python

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

+
+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

+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! 🚀

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

 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

 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

+ 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

+ 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

+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 <carli.freudenberg@energymeteo.de>

🤖 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 9c3ecc8Compare July 25, 2023 06:50

SergeyPirogov approved these changes Jul 27, 2023
View reviewed changes

github-actionsBot 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

mloeschand 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 managerMar 20, 2024

santi reviewed Apr 4, 2024
View reviewed changes

mloeschand 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 torufffromblackfor 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-actionsBotand 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 9426254Compare 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 managerApr 15, 2024

santiand others added 2 commits April 15, 2024 19:03

Merge branch 'main' into network
66e2584

chore: reformat
2e50cb3

santi merged commit 11964de into testcontainers:mainApr 16, 2024

github-actionsBot mentioned this pull request Apr 16, 2024
chore(main): release testcontainers 4.4.0 #539
Merged

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mloesch commented Jul 5, 2023•
edited
Loading