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

Support for WaitStrategy on DockerComposeContainer #598

Closed
wants to merge 3 commits into from
Closed

Support for WaitStrategy on DockerComposeContainer #598

wants to merge 3 commits into from

Conversation

barrycommins
Copy link
Contributor

@barrycommins barrycommins commented Mar 4, 2018

As discussed in #174

@bsideup @kiview
I think this probably requires a bit more work, but opening a PR to get your opinions on the approach, and see if you have any suggestions for improvement.

I've created a class DockerComposeContainerInstance which exposes containers created through Docker Compose and should behave consistently with regular GenericContainers after startup.

This should mean that any WaitStrategy, including customer user implementations. should work with DockerComposeContainers.

One concern I have is that with the current implementation, the DockerComposeContainer constructor returns as soon as the SocatContainer is available.
If users are waiting in their current tests through custom methods, it could now timeout instead of continuing if all exposed containers aren't available after 60 seconds.

The alternative is to keep the current implementation, but it is probably a better developer experience to apply a default WaitStrategy in most cases.

I'm also not sure about the same DockerComposeContainerInstance. Maybe DockerComposeServiceContainer would be better?

*/
@SuppressWarnings("ConstantConditions")
@EqualsAndHashCode(callSuper = true)
public class DockerComposeContainerInstance<SELF extends GenericContainer<SELF>> extends GenericContainer<SELF> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not sure what this class is doing, could you please explain it a bit more?

Copy link
Contributor Author

@barrycommins barrycommins Mar 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, sorry, it's probably neither well named nor well documented.

This class represents a service instance that has been created through docker-compose via DockerComposeContainer.

Take for example:

new DockerComposeContainer(new File("docker-compose.yml")
.withExposedService("redis_1", REDIS_PORT, Wait.forListeningPort())
.withExposedService("elasticsearch_1", ELASTICSEARCH_PORT, Wait.forListeningPort());

This will create two DockerComposeContainerInstances. One for "redis_1" and one for "elasticsearch_1".

It represents the service with the same API as GenericContainer, so things like getContainerId(), getMappedPort(), etc behave the same way as a GenericContainer.

This is why the constructor sets up things by calling setters and with... methods, so that the corresponding getters work the same way as if the service had been created through a GenericContainer.

The DockerComposeContainerInstance is then passes to the WaitStrategy via waitUntilReady(GenericContainer container), and means that any WaitStrategy should work the same way for a service created through DockerComposeContainer as one created through GenericContainer.

Lots of methods like getCommandParts() will probably never be used in any WaitStrategy, but there's no harm in having the API behave consistently for any future use cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now. So, the problem is that WaitStrategy is highly coupled with GenericContainer, right?

We should probably solve that instead of proxying with "fake" GenericContainer because otherwise, we will have to keep it in sync.
Let me check what can we do with it without breaking the compatibility.

Maybe, as a first step, we could add GenericContainer-agnostic wait strategies and adapt GenericContainer before calling them. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, GenericContainer and WaitStrategy are highly coupled and I was conscious of not breaking backwards compatibility.

I thought it might be possible to break out Container into separate interfaces, one with just getters and exec methods that represents a container that has already been started, and one with setters for setting up a container before starting it.

The getter interface could then be passed to the WaitStrategy.

The problem was that I couldn't find a way to do it without breaking compatibility.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we already have this (separation of container's config and state) in our minds and plan to implement really soon :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be pretty straightforward to update existing WaitStrategy classes to use that.

The difficulty comes with users who have implemented their own custom WaitStrategy classes.

I'm not sure how common that is, but I had it in mind when implementing this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, me either, but I was thinking to implement it the way their custom strategies will "just work" with GenericContainer but the compiler will not allow them to use with Docker Compose (new feature), so that they will need to update their implementation

@barrycommins
Copy link
Contributor Author

Closing in favour of #600

@bsideup bsideup removed this from the next milestone Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants