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.

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

Use of links between containers should be deprecated #465

Open
3 of 4 tasks
rnorth opened this issue Sep 27, 2017 · 15 comments
Open
3 of 4 tasks

Use of links between containers should be deprecated #465

rnorth opened this issue Sep 27, 2017 · 15 comments

Comments

@rnorth
Copy link
Member

rnorth commented Sep 27, 2017

As discussed in #463, docker networks seem mature enough to use, and links are officially not recommended any more.

We'd like to deprecate use of links; off the top of my head I believe we ought to do the following things:

  • Obviously, mark the container addLink method as deprecated with a clear comment
  • Links aren't currently documented very well, but update the docs with a clear enough description of the networking support, so that ad-hoc linking can be done easily by users
  • Modify Webdriver VNC recorder sidecar container to remove use of links
  • Modify Docker Compose support; can we remove use of the ambassador container now?
@kiview
Copy link
Member

kiview commented Sep 27, 2017

I'm really looking forward to removing the ambassador container, I believe this will make things a lot easier in more complex CI environments.

rnorth added a commit that referenced this issue Nov 12, 2017
* Mark link-related APIs as deprecated pending future removal
See #465

* Update changelog re deprecated links features
rnorth added a commit that referenced this issue Nov 19, 2017
* Mark link-related APIs as deprecated pending future removal
See #465

* Update changelog re deprecated links features
@kiview
Copy link
Member

kiview commented Feb 26, 2018

AFAIK the only thing missing now is the documentation update?

@felixbarny
Copy link

I'd very much appreciate some docs around networks :)

@kiview
Copy link
Member

kiview commented Apr 11, 2018

@felixbarny Yep you are right, PR appreciated 😸

@bsideup
Copy link
Member

bsideup commented Apr 11, 2018

@felixbarny yes, the docs are missing and I feel sorry about it (since I was the one who implemented the networks support)
However, it is as simple as .withNetwork(networkInstance) and until it is implemented you an check existing usages in tests, i.e. Kafka:

public void testExternalZookeeperWithExternalNetwork() throws Exception {
try (
Network network = Network.newNetwork();
KafkaContainer kafka = new KafkaContainer()
.withNetwork(network)
.withExternalZookeeper("zookeeper:2181");
GenericContainer zookeeper = new GenericContainer("confluentinc/cp-zookeeper:4.0.0")
.withNetwork(network)
.withNetworkAliases("zookeeper")
.withEnv("ZOOKEEPER_CLIENT_PORT", "2181");
) {
Stream.of(kafka, zookeeper).parallel().forEach(GenericContainer::start);
testKafkaFunctionality(kafka.getBootstrapServers());
}
}

@stale
Copy link

stale bot commented Oct 28, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

@stale stale bot added the stale label Oct 28, 2018
@stale
Copy link

stale bot commented Nov 11, 2018

This issue has been automatically closed due to inactivity. We apologise if this is still an active problem for you, and would ask you to re-open the issue if this is the case.

@stale stale bot closed this as completed Nov 11, 2018
@rnorth
Copy link
Member Author

rnorth commented Nov 11, 2018

Reopening - docs still needed

@rnorth rnorth reopened this Nov 11, 2018
@stale stale bot removed the stale label Nov 11, 2018
@stale
Copy link

stale bot commented Feb 9, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

@stale stale bot added the stale label Feb 9, 2019
@stale stale bot removed the stale label Feb 9, 2019
@bsideup
Copy link
Member

bsideup commented Nov 8, 2019

@rnorth given https://www.testcontainers.org/features/networking/#advanced-networking, should we close the issue now? :)

@sunmeplz
Copy link

Hi there,
Sometimes it is not possible to add network to container as it managed with third party libs, so add-link is the single solution

@eddumelendez
Copy link
Member

if it is built with GenericContainer similar to the Testcontainers modules then you should have access to withNetwork. Can you elaborate @sunmeplz , please?

@sunmeplz
Copy link

Hi @eddumelendez !
My case: I'm using quarkus framework which provide dev services based on test containers(https://github.com/quarkusio/quarkus/blob/main/extensions/kafka-client/deployment/src/main/java/io/quarkus/kafka/client/deployment/DevServicesKafkaProcessor.java). When you run application in dev or test mode, quarkus run the container. My goal is to create additional service which will use existing container(kafka and db in my case). So I can't access testcontainer instance, just can find already running container using plain docker api

@jfrantzius
Copy link

Hi,
the task "Modify Docker Compose support; can we remove use of the ambassador container now?" that is listed as completed in the description is unfortunately blocking #5351 , please see comment #5351 (comment) there for details.

Are there any plans to replace the deprecated call to SocatContainer.addLink() in ComposeDelegate.withExposedService(String, int, WaitStrategy) with network API calls, as Testcontainer's own Javadoc suggests? https://javadoc.io/doc/org.testcontainers/testcontainers/latest/index.html :

@deprecated
public void addLink(LinkableContainer otherContainer, java.lang.String alias)
Deprecated. Links are deprecated (see #465). Please use Network features instead.
Description copied from interface: Container
Add a link to another container.

@kiview
Copy link
Member

kiview commented Dec 6, 2023

Thanks for collecting the references @jfrantzius. I am not sure why the compose task in the original post in this issue is marked as done, since it is not yet done. I think it makes much sense to re-visist this, ideally with a non-breaking change that provides better compatibility within the ecosystem.

That being said, there aren't concrete plans to tackle this yet (although AFAIK @eddumelendez did a spike on this in the past).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants