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

Put all docker compose files into environment before starting #755

Merged
merged 3 commits into from
Jun 16, 2018

Conversation

zbstof
Copy link
Contributor

@zbstof zbstof commented Jun 15, 2018

Currently, only first docker-compose entry is put into environment:

new DockerComposeContainer(
                new File(Initializer.class.getResource("/docker/docker-compose.yml").toURI()),
                new File(Initializer.class.getResource("/docker/docker-compose-app.yml").toURI()))
              ...
                .withLocalCompose(true)
                .withPull(false);

will fail, when started, to the effect that properties from second docker-compose weren't applied

Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

Ah - good catch, and sorry if this caused you some problems. This looks good to me.

I think I'd like to have a test class to demonstrate that the problem is resolved. Is that something you could perhaps add?

@zbstof
Copy link
Contributor Author

zbstof commented Jun 15, 2018 via email

@rnorth
Copy link
Member

rnorth commented Jun 15, 2018

Thanks - if you run ./gradlew check (Linux/MacOS) or gradlew.bat check (if on Windows) that should work!

The tests take quite a while to run, so I'd recommend just running your own test class in your IDE if you can while developing it.

Thanks again.

@zbstof
Copy link
Contributor Author

zbstof commented Jun 15, 2018

I think this test will require docker-compose to be present on build service?
Is this the case for Circle-CI?

Also, Intellij fails compilation, and from command-line I cannot run single test for some reason:

./gradlew test --tests org.testcontainers.junit.DockerComposeLocalOverrideTest
> Task :couchbase:test FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':couchbase:test'.
> No tests found for given includes: [org.testcontainers.junit.DockerComposeLocalOverrideTest](--tests filter)

DockerComposeLocalOverrideTest - this is my test

@zbstof zbstof closed this Jun 15, 2018
@zbstof zbstof reopened this Jun 15, 2018
@zbstof
Copy link
Contributor Author

zbstof commented Jun 15, 2018

Ok, with new test, but without the fix:

Gradle Test Executor 6 > org.testcontainers.junit.DockerComposeLocalOverrideTest > testEnvVar FAILED
    org.rnorth.ducttape.TimeoutException: Timeout waiting for result with exception
        at org.rnorth.ducttape.unreliables.Unreliables.retryUntilSuccess(Unreliables.java:51)
        at org.rnorth.ducttape.unreliables.Unreliables.retryUntilTrue(Unreliables.java:95)
        at org.testcontainers.junit.DockerComposeLocalOverrideTest.testEnvVar(DockerComposeLocalOverrideTest.java:54)

        Caused by:
        java.lang.RuntimeException: Not ready yet

With fix test passes

@zbstof
Copy link
Contributor Author

zbstof commented Jun 15, 2018

BTW, could you please release this as a patch release, if this is not too much trouble?
We need this feature in my company. For some reason dockerized compose didn't work for us.

@rnorth
Copy link
Member

rnorth commented Jun 15, 2018

Thanks @zbstof, I'll have a look now!

@rnorth
Copy link
Member

rnorth commented Jun 15, 2018

I'm just going to push a slight change if that's OK; I've combined three similar test classes into one class that has parameterized tests.

This was something that needed to be done before your change, it's just a bit more necessary now.

@rnorth rnorth merged commit 6a83e85 into testcontainers:master Jun 16, 2018
@rnorth
Copy link
Member

rnorth commented Jun 16, 2018

Thanks for this @zbstof - I've merged into master

@rnorth
Copy link
Member

rnorth commented Jul 12, 2018

Released in 1.8.1! 🎉

Thanks for your contribution @zbstof!

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