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

Ability to use local Docker compose binary instead of container. #200

Merged

Conversation

outofcoffee
Copy link
Contributor

Purpose

Adds the ability to use local Docker compose binary instead of container.

Rationale

In scenarios where tests are running within a container (such as using Jenkins Docker Plugin), it is difficult for the docker-compose.yml file to be bind mounted into the internal Docker compose container used by the current DockerComposeContainer implementation.

Implementation

Key methods from DockerCompose have been extracted to an interface, and a second implementation, LocalDockerCompose has been added that uses the docker-compose on the PATH.

Users call the withLocalCompose() method on DockerComposeContainer to activate this behaviour.


// wait for the compose process to stop, which should only happen after it has spawned all the service containers
p.waitFor(120, TimeUnit.SECONDS);
Copy link
Member

Choose a reason for hiding this comment

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

Should this timeout be longer, especially to allow for pulling/building of containers..?
I'm not sure there's actually any easy timeout length, so maybe just let it be infinite?

@qoomon
Copy link
Contributor

qoomon commented Aug 15, 2016

I think it shouldn't be done with an explicit call. I would prefer to do it with an environment variable. We than can run the same java source within different environments without adopting the test to specific environment.

@rnorth
Copy link
Member

rnorth commented Aug 15, 2016

@qoomon good point. How about we make withLocalCompose() require a boolean value to be passed to turn this behaviour on/off?

That way users would be free to define an environment variable (rather than us dictating one) and use that. Alternatively they could just pass in true if there's something about their usage that means this behaviour is always desired. What do you think @outofcoffee ?

@qoomon
Copy link
Contributor

qoomon commented Aug 15, 2016

I dont like the idea to check out a project and then are not be able to run all test because of environment dependencies. Therefore i would like to keep dependencies at a bare minimum.
Maybe another solution could be to fall back to compose container if withLocalComose() is set.

@qoomon
Copy link
Contributor

qoomon commented Aug 15, 2016

And i think the problem described is already solved by #189

@rnorth
Copy link
Member

rnorth commented Aug 16, 2016

What do you think of #189 @outofcoffee? Does this PR address a different
issue?
On Mon, 15 Aug 2016 at 23:17 Bengt Brodersen notifications@github.com
wrote:

And i think the problem described is already solved by #189
#189


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#200 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIET9Nrss9l6RNup_0tHn0flp-HxcPnks5qgOV5gaJpZM4Jhprs
.

Richard

@outofcoffee
Copy link
Contributor Author

@rnorth #189 solves something different. In this scenario, PWD resolves to a path within the current container; namely the one running the JUnit test. That path is not going to be accessible to the Compose container, so Compose can't see the YAML file.

Mounting a volume is also impractical as a work around due to the issue of the buildup of garbage with containers having different lifecycles (compose and the test host container).

In short, no, it doesn't work for this scenario.

@outofcoffee
Copy link
Contributor Author

outofcoffee commented Sep 24, 2016

@rnorth rebased on master and:

  • switched to zt-exec
  • added check for binary and fails fast if not present
  • cleaned up exit value check
  • set timeout to infinite, as suggested
  • tidied up a bit
  • made withLocalCompose accept a boolean

@rnorth
Copy link
Member

rnorth commented Oct 16, 2016

@outofcoffee sorry for the numerous delays in looking at this. I will merge. However, please could you rebase onto master and solve the current merge conflicts?

@pcornish
Copy link

@rnorth - passed on Travis, but CircleCI failed with:

org.testcontainers.containers.ContainerFetchException: Retry limit reached while trying to pull image: mysql:5.5

:-/

@rnorth
Copy link
Member

rnorth commented Oct 17, 2016

@pcornish thanks. Docker pull issue = :z
Merging, as this failure is an intermittent one.

@rnorth rnorth merged commit 0307150 into testcontainers:master Oct 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants