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

No waitingFor() method in DockerComposeContainer #174

Closed
ldcasillas-progreso opened this issue Jul 15, 2016 · 12 comments
Closed

No waitingFor() method in DockerComposeContainer #174

ldcasillas-progreso opened this issue Jul 15, 2016 · 12 comments

Comments

@ldcasillas-progreso
Copy link

Using version 1.1.0, the DockerComposeContainer class no longer extends from GenericContainer and thus doesn't have anything like the waitingFor method.

Related issues:

@rnorth
Copy link
Member

rnorth commented Jul 16, 2016

Hi Luis

Yes, I'll reinstate. I'm afraid I'd not anticipated that use case.

I imagine you must be relying on a call to getServicePort in your
WaitStrategy. Is that correct? If so I'll need to take a little care to
ensure that also works.

If you could possibly share some demo code for how you're using this it
would help me make sure compatibility is retained!

Thanks

Richard

Richard

@ldcasillas-progreso
Copy link
Author

Because of problems with DockerComposeContainer in 1.0.5 I was just not using it at all—I was manually coordinating multiple GenericContainers. So I don't have a code example that's regressed between 1.0.5 and 1.1.0, or any backwards compatibility constraints.

@rnorth rnorth modified the milestones: 1.1.3, 1.1.4 Jul 19, 2016
@rnorth
Copy link
Member

rnorth commented Aug 16, 2016

Sorry this didn't make it into 1.1.4 :(

@rnorth rnorth removed this from the 1.1.4 milestone Aug 16, 2016
@outofcoffee
Copy link
Contributor

Suggest we move waitingFor to its own interface, implemented by both GenericContainer and DockerComposeContainer. In the latter case, the implementation could make use of the listChildContainers method and apply the WaitStrategy to each of them.

Thoughts @rnorth ?

@rnorth
Copy link
Member

rnorth commented Nov 20, 2016

Potentially @outofcoffee; I can see there being some complication in needing different wait strategies for different child containers. So we'd need a way of telling testcontainers which wait strategy corresponds to which service, wouldn't we?

@outofcoffee
Copy link
Contributor

Agree; I've implemented this by abstracting the port exposure stuff into a higher level 'EndpointBuilder', which works like this:

// Fetch an EndpointBuilder from container/compose container
EndpointBuilder endpointBuilder = container.newEndpointBuilder();

endpointBuilder.add("my-service")
    .withExposedPort(...)
    .waitingFor(...);

// add further endpoints

endpointBuilder.build();

When calling EndpointBuilder.add() an intermediary state holder, Endpoint is returned and registered in the builder. Calling build() iterates over these and handles the logic of orchestrating the calls to the containers to set up ports and wait strategies etc.

To do this, Endpoint, GenericContainer and DockerComposeContainer implement an interface with methods like 'waitingFor', 'withExposedPort' and 'getMappedPort'.

@edgarvonk
Copy link

Hi all, are there any updates on this maybe? This feature would greatly help us because we need to wait until certain events have taken place until we can start our component tests.

@barrycommins
Copy link
Contributor

I've been having a quick look at this, and was wondering would it be a good idea to add an overloaded withExposedService() method with a WaitStrategy parameter on DockerComposeContainer?

Something like:

public SELF withExposedService(String serviceName, int servicePort, @NonNull WaitStrategy waitStrategy)

Example usage:

new DockerComposeContainer(new File("test-compose.yml"))
    .withExposedService("redis_1", 6379, Wait.forListeningPort())
    .withExposedService("elasticsearch_1", 9200, Wait.forHttp("/all").forStatusCode(200));

The HttpWaitStrategy should work fine, but I'm not sure how the others would work with the AmbassadorContainer

@bsideup
Copy link
Member

bsideup commented Feb 22, 2018

Hi @barrycommins,
I really like the idea :) Do you want to contribute it?

@barrycommins
Copy link
Contributor

Hi @bsideup,

I looked at this a like bit at the time. I think things are a little complicated due to the use of Ambassador containers.

I think it'd be easy enough to apply WaitStrategy to the Ambassador containers, but more difficult to apply them to the proxied containers, which is what you'd really need.

This is a feature I'd really like though, so I'll try to get back to it in the next couple of weeks, and see what's possible.

@kiview
Copy link
Member

kiview commented Feb 24, 2018

Hey @barrycommins, just as a heads up, the old ambassador container has been changed to SocatContainer in Testcontainers 1.6.0, which is a simpler design I think and is a very transparent proxy.

bsideup pushed a commit that referenced this issue Mar 19, 2018
…oach (#600)

* Support for WaitStrategy on DockerComposeContainer

As discussed in #174

* CI code quality fixes

* CI code quality fixes

* Another attempt at adding WaitStrategy support for docker-compose

* Another attempt at adding WaitStrategy support for docker-compose

fixed some tests and added some files missed from previous commit

* Changes after code review comments

Added new WaitStrategyTarget interface, plus moved some methods into
new LogFollower and CommandExecutor interfaces

* Changes after code review comments

Removed CommandExecutor and LogFollower interfaces.
Added ExecInContainerPattern utility class.

Changed existing WaitStrategy implementations to inherit from new ones

* CI code quality fixes

* Changes from code review.

Moved getMappedPort to default implementation in ContainerState.
Using proxyContainer in ComposeStrategyWaitServiceTarget to get ip.
Removed unnecessary null checks.
Using lazy getter for containerInfo in ComposeStrategyWaitServiceTarget.

* Changes from code review.

Removed getLogger() method.
Removed getContainerName from Container and ContainerState.
Removed getExposedPortNumbers from ContainerState.
Added waitingFor method to DockerComposeContainer.
Removed StartupTimeout interface.
@bsideup bsideup added this to the next milestone Mar 19, 2018
@bsideup
Copy link
Member

bsideup commented Apr 7, 2018

Released in 1.7.0 (already in Bintray's JCenter, will be synced with Maven Central soon)

@bsideup bsideup closed this as completed Apr 7, 2018
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