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

1.1 #167

Merged
merged 40 commits into from
Jul 5, 2016
Merged

1.1 #167

merged 40 commits into from
Jul 5, 2016

Conversation

rnorth
Copy link
Member

@rnorth rnorth commented Jul 3, 2016

Main changes:

rnorth added 27 commits July 3, 2016 15:35
Use a TCP-unix socket proxy instead of relying on netty to support unix sockets. More tests are disabled due to OS X / docker machine coupling assumptions.
Needs testing for reliability and performance, but seems to function reasonably well so far.

Add a configuration strategy for proxied connections to Docker for Mac daemon on OS X

Include temporary workaround for docker-java/docker-java#588
… polling of Docker API (using rate limiter for testing new JDBC container connections)
…mpatibility issues

Refactor Docker compose support, to allow scaling, better output logs, and eventually docker-compose v2 format. Refs #146, #147

Start support for docker-compose v2 environments, putting ambassador containers onto the right network to be able to access the compose-launched containers.

Fix Netty conflicts by using Jedis for testing (simpler dependencies)
…that don't have a slash prepended, i.e. non docker compose containers

Use partial name matching for creating links between containers (in line with linked container network identification)

Remove automatic setting of network mode to 'bridge' when a linked container is not on any network, as this causes an error in the current Docker API and is unnecessary.
Tidy up Docker client configuration to avoid repetitive creation of temporary clients
Add recommended logback.xml entry for testcontainers logs to be at INFO level
…onger necessary, so removal simplifies reasoning about control flow.

<dependency>
<groupId>org.rnorth</groupId>
<artifactId>tcp-unix-socket-proxy</artifactId>
Copy link
Member Author

Choose a reason for hiding this comment

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

Until netty and/or docker-java have native support for unix domain sockets, use a slightly hacky but effective workaround of proxying over TCP. For Testcontainers' purposes, there's no perceptible performance impact.

<transformer
implementation="org.apache.maven.plugins.shade.resource.ManifestResourceTransformer"/>
<transformer
implementation="org.apache.maven.plugins.shade.resource.ServicesResourceTransformer"/>
</transformers>
<relocations>
Copy link
Member Author

Choose a reason for hiding this comment

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

Still need to add netty relocation

@@ -191,31 +190,10 @@ private void tryStart(Profiler profiler) {
containerIsStarting(containerInfo);

// Wait until the container is running (may not be fully started)
profiler.start("Wait until container state=running, or there's evidence it failed to start.");
Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed this startup attempt loop - it might have been useful back when it was added (e.g. for startup of ambassador containers), but I don't think that is the case any more. Stability seems to be unaffected by removal.

Simplifying this should make the code much more readable and easier to reason about.

class DockerCompose extends GenericContainer<DockerCompose> {
public DockerCompose(File composeFile, String identifier) {

super("dduportal/docker-compose:1.6.0");
Copy link
Contributor

Choose a reason for hiding this comment

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

We have been using docker-compose 1.7.1 successfully instead of 1.6.0. Is there a reason to not upgrade this?

Copy link
Member Author

Choose a reason for hiding this comment

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

@valdisrigdon Good point, thanks. I've updated and other than a very minor tweak it seems to work fine.

@rnorth
Copy link
Member Author

rnorth commented Jul 5, 2016

Will merge as I'm pretty comfortable that no major things need to be done. I'd expect to follow up with a point release reasonably soon to address further improvements or any necessary bugfixes.

@rnorth rnorth merged commit e1e501c into master Jul 5, 2016
@rnorth rnorth deleted the 1.1 branch November 13, 2016 10:34
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

2 participants