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 - alternative approach #600

Merged
merged 10 commits into from
Mar 19, 2018
Merged

Support for WaitStrategy on DockerComposeContainer - alternative approach #600

merged 10 commits into from
Mar 19, 2018

Conversation

barrycommins
Copy link
Contributor

@bsideup, just wanted to continue the conversation from #598

I didn't want to update that pull request for now, as this is a much bigger change.

I've implemented an alternative approach, involving breaking out all of the methods that apply after startup from the Container interface into a new interface ContainerState.

I've also moved the functionality implemented in the various WaitStrategy classes into classes with the same names, but in a sub-package org.testcontainer.wait.strategy and updated the current implementations to delegate to the new ones. I have also deprecated the use of the current classes in favour of the new ones.

For each instance of a service started by docker-compose, I'm instantiating a class called DockerComposeServiceInstance, which implements ContainerState, and applying the new WaitStrategy implementation to them.

For example:

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

would create two instance, and apply a WaitStartegy to each.

This is a considerably larger change than the other pull request, and might not gel with the future plans for interface changes, so I thought I'd request feedback at this point. There's still work required on it, but all tests are passing.

If this is too large a change, or doesn't feel like the right approach, please let me know. If that's the case, maybe we should revisit after the planned changes for splitting configuration and state are done.

@bsideup
Copy link
Member

bsideup commented Mar 8, 2018

@barrycommins while I like the final result a lot, I'm thinking what if we dramatically reduce the number of changes by going a bit to the low level landscape and using com.github.dockerjava.api.model.Info as an argument to the wait strategies (instead of ContainerState)

This way we can unify the strategies between GenericContainer and Docker Compose, always use the actual state (current implementation with GenericContainer will use the state set by user (i.e. envs), but what if Docker daemon will change them? Would be more safe to use Info object instead which represents the actual state received from Docker daemon)

WDYT?

@barrycommins
Copy link
Contributor Author

barrycommins commented Mar 8, 2018

That would certainly be a simpler solution.

I think it would probably work ok for the HostPortWaitStrategy and LogMessageWaitStrategy, but maybe not the HttpWaitStrategy.

For docker-compose, that wait strategy needs to know the port the service is mapped to on the SocatContainer, which wouldn't be available through the Info object of the service container.

I'll make another attempt at it though with that approach and see if I can come up with a solution.

Are you ok with deprecating the current WaitStrategy and creating new implementations in a sub-package, then delegating to the new implementations?

Or can you suggest an alternative approach for that?

Edit: thinking about it, the HostPortWaitStrategy requires commands to be executed within the running container to see if the internal port is listening, so would need access to the DockerClient and not just the Info object.

@bsideup
Copy link
Member

bsideup commented Mar 8, 2018

@barrycommins Docker client is always available as DockerClientFactory.instance().client(), so it's not a problem

Re HttpWaitStrategy - actually, DockerComposeContainer should handle it instead of a strategy itself because socat-based exposing is a detail of an implementation of it and strategies should be agnostic to it.

Maybe need a "proxying" object, something like WaitStrategyTarget, with Info plus everything related to the host, like getMappedPort.

@barrycommins
Copy link
Contributor Author

Ok, good suggestions, thanks.

I'm very much in favour of reducing the changeset, so I'll take try this approach in the next week or so.

@bsideup
Copy link
Member

bsideup commented Mar 8, 2018

@barrycommins let me know if you need a help or something :)
Some thoughts:

  • WaitStrategyTarget is better be an interface with an abstract implementation or some default methods
  • Please keep your WaitStrategy refactoring 👍
  • Later, once we extract ContainerState at a core level, we could add it to the WaitStrategyTarget interface, so that the strategies can also benefit from it
  • Some operations (like getting mapped port, envs, etc) could be shared between GenericContainer and WaitStrategies. While I was writing this I realized that maybe it's a good idea to still introduce ContainerState interface, but with a very limited set of methods, and every method must use Info as a source, and also maybe implemented as a default method of ContainerState. How does it sound to you? :)

P.S. good job, thanks! Will be one of the biggest (and most awaited) features of the upcoming release 👍

@barrycommins
Copy link
Contributor Author

Those are all really helpful suggestions, thanks.

@barrycommins
Copy link
Contributor Author

barrycommins commented Mar 9, 2018

@bsideup

While I was writing this I realized that maybe it's a good idea to still introduce ContainerState interface, but with a very limited set of methods, and every method must use Info as a source,

The Info class seems to refer to Docker itself, rather than an individual container.
Should it be InspectContainerResponse?

@bsideup
Copy link
Member

bsideup commented Mar 9, 2018

@barrycommins oh, yes, InspectContainerResponse! Sorry :)

Added new WaitStrategyTarget interface, plus moved some methods into
new LogFollower and CommandExecutor interfaces
@barrycommins
Copy link
Contributor Author

@bsideup
I've added another commit, which I hope is a bit closer to what you are looking for.

I added two more interfaces CommandExecutor and LogFollower, which could probably be folded into ContainerState, but didn't seem to quite fit there. Maybe that's just a naming problem.

The change set is still quite large, so if you have any more suggestions, or if I have misunderstood your earlier suggestions, please let me know.

Copy link
Member

@bsideup bsideup left a comment

Choose a reason for hiding this comment

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

We're getting much closer to a final result 👍

Added a few comments, but it looks much better in general 👍

/**
* @return the logger for the current container
*/
Logger getLogger();
Copy link
Member

Choose a reason for hiding this comment

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

logger should not be a part of the container's state

/**
* @return the container info
*/
default InspectContainerResponse getContainerInfo() {
Copy link
Member

Choose a reason for hiding this comment

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

the result of this method must be cached, otherwise it's a huge performance penalty. Please either make it abstract or use a caching Map (with weak keys)

@@ -32,6 +30,11 @@ default SELF self() {
return (SELF) this;
}

@Override
default String getContainerName() {
return CommandExecutor.super.getContainerName();
Copy link
Member

Choose a reason for hiding this comment

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

should use ContainerState as a super interface here

}

private String getServiceNameFromContainer(Container container) {
final String containerName = container.labels.get("com.docker.compose.service");
Copy link
Member

Choose a reason for hiding this comment

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

please use getters, not fields

Copy link
Member

Choose a reason for hiding this comment

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

When was this label added to the Docker Compose? I want to make sure we're not raising our compatibility baseline

Copy link
Contributor Author

@barrycommins barrycommins Mar 12, 2018

Choose a reason for hiding this comment

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

@@ -343,13 +377,13 @@ protected Integer getLivenessCheckPort() {

/**
* @return the ports on which to check if the container is ready
* @deprecated use {@link #getLivenessCheckPortNumbers()} instead
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getLivenessCheckPorts() in GenericContainer is protected, not public, so I couldn't move it to an interface without a breaking change. It's overridden in several subclasses of GenericContainer, so I presume it is probably implemented by some other users.

*/
@Deprecated
@NonNull
protected WaitStrategy waitStrategy = Wait.defaultWaitStrategy();
Copy link
Member

Choose a reason for hiding this comment

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

old WaitStrategies should extend from new so simplify the transition and also to remove this duality of strategies here

@@ -13,12 +13,12 @@
*/
@RequiredArgsConstructor
public class ExternalPortListeningCheck implements Callable<Boolean> {
private final Container<?> container;
private final WaitStrategyTarget waitStrategyTarget;
Copy link
Member

Choose a reason for hiding this comment

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

I would use ContainerState here

import java.io.IOException;
import java.nio.charset.Charset;

public interface CommandExecutor extends ContainerState {
Copy link
Member

Choose a reason for hiding this comment

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

While I understand the motivation for this interface (and also LogFollower), I see more value in some utility class with execInContainer(containerId, Charset charset, String...command) method instead, and both GenericContainer and WaitStrategy calling it when needed.

The motivation is simple:

  1. it will be possible to use it with any container (currently one will have to implement this interface to access this functionality)
  2. it will get us closer to the "reusable container patterns" idea we had some time ago
  3. having less interfaces gives us more flexibility to change the API in future

So, something like:

public class ExecInContainerPattern {

    // Note generic ExecResult instead of Container.ExecResult (should be converted to later in GenericContainer)
    public static ExecResult execInContainer(String containerId, Charset charset, String... command) {
        // ...
    }
}

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.

Made this change along with the other suggested changes in the latest commit.
I've added a Logger parameter to the method in this new class, it's probably not ideal since it is only used for debug and trace messages, but they can be useful and I didn't want to change the existing behaviour.

* @param types types that should be followed (one or both of STDOUT, STDERR)
*/
default void followOutput(Consumer<OutputFrame> consumer, OutputFrame.OutputType... types) {
LogUtils.followOutput(DockerClientFactory.instance().client(), getContainerId(), consumer, types);
Copy link
Member

Choose a reason for hiding this comment

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

Since we already have a LogUtils, I would also (the same as CommandExecutor) remove this interface (but maybe add an overload for followOutput with default output types)

@bsideup bsideup requested a review from rnorth March 12, 2018 07:08
@bsideup bsideup self-assigned this Mar 12, 2018
Removed CommandExecutor and LogFollower interfaces.
Added ExecInContainerPattern utility class.

Changed existing WaitStrategy implementations to inherit from new ones
@rnorth
Copy link
Member

rnorth commented Mar 13, 2018

Sorry for my continuing lack of review on this - it looks like a lot of hard, good work has gone into it, so thank you! I'll try to review tomorrow.

Copy link
Member

@bsideup bsideup left a comment

Choose a reason for hiding this comment

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

I'm requesting some changes but we're getting much closer to the final result 👍 Super excited!

this.proxyContainer = proxyContainer;
this.logger = logger;

if (mappedPorts != null) {
Copy link
Member

Choose a reason for hiding this comment

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

please add @NonNull to mappedPorts argument and use this.mappedPorts = new HashMap(mappedPorts)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A ComposeServiceWaitStrategyTarget is created for every service returned by listChildContainers() in DockerComposeContainer, which can return services with no exposed ports.

for example:

redis:
  image: redis
db:
  image: orchardup/mysql

and a test with

new DockerComposeContainer(new File("src/test/resources/compose-test.yml"))
            .withExposedService("redis_1", REDIS_PORT, Wait.forListeningPort());

Only the redis container has exposed/mapped ports, so the ComposeServiceWaitStrategyTarget for the db service would have a null value for mappedPorts.

I'll change the behaviour to only create a ComposeServiceWaitStrategyTarget for services that have been explicitly exposed.

Copy link
Member

Choose a reason for hiding this comment

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

We should never accept null as a valid collection argument's value, that was the motivation :)

Copy link
Member

Choose a reason for hiding this comment

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

I'll change the behaviour to only create a ComposeServiceWaitStrategyTarget for services that have been explicitly exposed.

Now I think about use cases where waiting strategy doesn't use the ports at all but checks the logs for instance.
Current API doesn't allow that without exposing a port, right?

Should we maybe add waitingFor("redis_1", Wait.forLogMessage()) method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add that method, but then we get into the problem of a ComposeServiceWaitStrategyTarget with no exposed ports, so the constructor would have to accept a null or empty value for mappedPorts, which would mean returning a empty list from getExposedPorts.

That goes against this idea:

We should never accept null as a valid collection argument's value, that was the motivation :)

It also means you could use DockerComposeContainer like this:

new DockerComposeContainer(new File("src/test/resources/compose-test.yml"))
            .withExposedService("redis_1", REDIS_PORT, Wait.forListeningPort());

or like this:

new DockerComposeContainer(new File("src/test/resources/compose-test.yml"))
            .withExposedService("redis_1", REDIS_PORT)
            .waitingFor("redis_1", Wait.forListeningPort());

Is that slightly confusing for the user?

Copy link
Member

@bsideup bsideup Mar 17, 2018

Choose a reason for hiding this comment

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

but then we get into the problem of a ComposeServiceWaitStrategyTarget with no exposed ports, so the constructor would have to accept a null or empty value for mappedPorts, which would mean returning a empty list from getExposedPorts

Sounds absolutely fine to me - no exposed ports means an empty set.

Second example IMO doesn't look confusing, and makes even more sense because we're actually waiting for a service and not for the exposed port :)

Just .withExposedService("redis_1", REDIS_PORT, Wait.forListeningPort()) is a good alias/shortcut for it


if (mappedPorts != null) {
this.mappedPorts.putAll(mappedPorts);
this.exposedPorts.addAll(this.mappedPorts.keySet());
Copy link
Member

Choose a reason for hiding this comment

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

why do you store exposedPorts when it's just an alias to this.mappedPorts.keySet()? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the getExposedPorts() method from the ContainerState interface returns Set<Integer>, while this.mappedPorts.keySet() returns List<Integer>.

I didn't want to create a new Set every time the getter is called.

Copy link
Member

Choose a reason for hiding this comment

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

keySet() returns Set as far as I remember :) Maybe you meant that getExposedPorts returns List<Integer> and not Set<Integer>?

Also, maybe we should make getExposedPortNumbers return Set<Integer>, use it where possible, and add getExposedPorts to ComposeServiceWaitStrategyTarget with getExposedPortNumbers().stream.collect(toList()) implementation?

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, sorry, obviously keySet() returns a Set! :-)

getExposedPortNumbers().stream.collect(toList())

Isn't that functionally the same as new ArrayList<>(getExposedPortNumbers());?

Copy link
Member

Choose a reason for hiding this comment

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

it is :) Both are equally fine to me since we're talking about collections with just a few elements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add getExposedPorts to ComposeServiceWaitStrategyTarget with getExposedPortNumbers().stream.collect(toList()) implementation?

Ah, I've just realised why this won't work. getExposedPortNumbers() in ContainerState is a default method that calls getExposedPort()!

default Set<Integer> getExposedPortNumbers() {
        return getExposedPorts().stream()
            .map(this::getMappedPort)
            .collect(Collectors.toSet());
    }

Copy link
Member

Choose a reason for hiding this comment

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

just override (implement) both inComposeServiceWaitStrategyTarget , getExposedPortNumbers and getExposedPorts

Copy link
Member

Choose a reason for hiding this comment

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

oh, I didn't notice that! Yes, sure!

P.S. I checked the implementation and it seems it is only used in WaitStrategyTarget ::getLivenessCheckPortNumbers.

Let's just remove this method and use it's implementation in WaitStrategyTarget.
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.

Despite the similar names, getExposedPorts() and getExposedPortNumbers() do different things.

I think getExposedPortNumbers() is confusingly named, because what it actually does is return the port that the service port has been mapped to.

e.g. .withExposedService("redis_1", 6379).
getExposedPorts() returns 6379 and getExposedPortNumbers() would return e.g. 32679

I took the name from a private method in GenericContainer, but it should maybe be renamed in ContainerState to getMappedPorts(). 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.

Sorry, I deleted the original comment, because I was starting to confuse myself.
Yes, it's only used in WaitStrategyTarget ::getLivenessCheckPortNumbers
I agree, it can definitely go!

}

@Override
public InspectContainerResponse getContainerInfo() {
Copy link
Member

Choose a reason for hiding this comment

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

you can remove this method and use:

@Getter(lazy=true)
private InspectContainerResponse containerInfo = DockerClientFactory.instance().client().inspectContainerCmd(getContainerId()).exec();

Copy link
Member

Choose a reason for hiding this comment

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

it will also make it thread safe

@@ -32,6 +32,11 @@ default SELF self() {
return (SELF) this;
}

@Override
default String getContainerName() {
Copy link
Member

Choose a reason for hiding this comment

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

why this change is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HostPortWaitStrategy uses getContainerName(), which was previously protected visibility in GenericContainer.
It's only used for a debug statement, so it could probably be removed if you think this method shouldn't be added.

Copy link
Member

Choose a reason for hiding this comment

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

since the method was protected, this is a breaking change anyway (making it public)
Let's just not expose this method and use getContainerInfo().getName() in HostPortWaitStrategy, or maybe even remove it since the name is not that helpful anyway (we assign random names)

(WaitAllStrategy) new WaitAllStrategy().withStartupTimeout(startupTimeout));

if(waitStrategy != null) {
waitStrategy.withStartupTimeout(startupTimeout);
Copy link
Member

Choose a reason for hiding this comment

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

already set in computeIfAbsent

Copy link
Contributor Author

@barrycommins barrycommins Mar 16, 2018

Choose a reason for hiding this comment

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

The startupTimeout is set on the WaitAllStrategy that wraps the (potentially) multiple WaitStrategy instances for a particular service, but I thought it would be a good idea to apply the same timeout to all of those strategies.

Otherwise you could have the case where one strategy could time out after 1 minute, but the WaitAllStrategy could be set to 5 minutes.

As an example:

ew DockerComposeContainer(new File("src/test/resources/compose-test.yml"))
            .withStartupTimeout(Duration.ofSeconds(120))
            .withExposedService("redis_1", REDIS_PORT, Wait.forListeningPort())
            .withExposedService("redis_1", SOME_OTHER_PORT, Wait.forListeningPort());

Two instances of HostPortWaitStrategy would be applied to the same service here. The idea was to apply the same timeout to both.

WDYT? Does it make sense to do this?

edit: it's just occurred to me that the order in which the methods are called would be important in this implementation, because if withStartupTimeout() is called after withExposedService(), the timeout would not be applied to the strategies. I'll fix after your feedback on this comment.

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. Hm, what if we rename the method to withDefaultStartupTimeout here?
Not sure tho.

We need to be careful because of existing "startup timeout" in GenericContainer where it applies to the container and not a set of containers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had moved SELF withStartupTimeout(Duration startupTimeout); to a new interface StartupTImeout, which I thought was inherited by Container and implemented by 'GenericContainerandDockerComposeContainer`, but part of that seems to have been lost in one of the refactors.

Currently that interface is only implemented by DockerComposeContainer, while Container has a method with the same signature.
I could remove that interface and just add the method directly to DockerComposeContainer? Or add a method like withDefaultStartupTimeout, as you mentioned?

In general though, should we be applying a single timeout to all WaitStrategy instances on docker-compose?
AbstractWaitStrategy has a default timeout of 60s, so it would be difficult to check if a default should be applied to all strategies, or if it has been overridden for a particular strategy.

Since there's probably multiple services involved, the timeouts are likely to need to be longer than for a single GenericContainer.

Copy link
Member

Choose a reason for hiding this comment

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

I would remove the interface, at least I see no use cases for it

Re "single timeout to all WaitStrategy instances" - good point about "all strategies". Maybe we should consider removing the method (withStartupTimeout) at all?

So that users will explicitly add the timeout per exposed service:

.withExposedService("redis_1", 6379, Wait.forHttp("/").withStartupTimeout(...))

Copy link
Contributor Author

@barrycommins barrycommins Mar 17, 2018

Choose a reason for hiding this comment

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

There's still the case where you may have more than one WaitStrategy per service, e.g., it if exposes two ports

.withExposedService("redis_1", 6379, Wait.forListeningPort())
.withExposedService("redis_1", 6380, Wait.forListeningPort())

I think each service needs to have a WaitAllStrategy to wrap the possible multiple strategies (which is what it's doing in the current PR), and that also needs to have a timout. Or at least, it has one in the current implementation.

There's no other current mechanism to expose multiple ports on the same service. We could have added varargs like withExposedService(String serviceName, int... ports), but that is not possible with these new methods, since varargs have to be the last parameter.

We could add an array/collection as a parameter on another overloaded method, but that's not so great from a user perspective, I think.

Maybe an array would be ok, because you could then use:
withExposedService("redis_1", {6379, 6380}, Wait.forListeningPort())

Copy link
Member

Choose a reason for hiding this comment

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

WaitAllStrategy can have an infinite duration (means the duration will be controlled by inner waiters):

.withExposedService("redis_1", 6379, Wait.forListeningPort().withStartupTimeout(timeout1))
.withExposedService("redis_1", 6380, Wait.forListeningPort()) // Uses default

no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current implementation of WaitAllStrategy has a default timeout of 30 seconds. For this case we can either implement a new InfiniteTimeWaitAllStrategy, or just set the timeout for an improbably long time for the WaitAllStrategy created in DockerComposeContainer per service. Say 30 minutes or so?

Copy link
Member

Choose a reason for hiding this comment

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

sounds good

final WaitAllStrategy waitAllStrategy = waitStrategyMap.computeIfAbsent(serviceInstanceName, __ ->
(WaitAllStrategy) new WaitAllStrategy().withStartupTimeout(startupTimeout));

if(waitStrategy != null) {
Copy link
Member

Choose a reason for hiding this comment

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

can't be null

@Override
public SELF withStartupTimeout(Duration startupTimeout) {
this.startupTimeout = startupTimeout;
return (SELF) this;
Copy link
Member

Choose a reason for hiding this comment

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

return self()

* {@inheritDoc}
*/
@Override
public Logger getLogger() {
Copy link
Member

Choose a reason for hiding this comment

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

a leftover? Logger should not be exposed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The execInContainer() methods, which have now been moved to ExecInContainerPattern still use a logger which must be passed into the method.

It's used for a debug and two trace statements, which are useful, but not exactly crucial.
Should they be removed, or can you see another way around 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, I think we should remove it and create another logger in ExecInContainerPattern

@@ -837,6 +833,7 @@ public Integer getMappedPort(final int originalPort) {
Preconditions.checkState(containerId != null, "Mapped port can only be obtained after the container is started");
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't getMappedPort be a default implementation in ContainerState interface? Ofc implementations like DockerComposeContainer will override it because of their specifics, but the rest should be fine with this implementation

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.
@barrycommins
Copy link
Contributor Author

@bsideup I've made some of the changes you requested in the review, but had a couple of questions on other ones.

Let me know what you think, and I'll make further changes as necessary.

Removed getLogger() method.
Removed getContainerName from Container and ContainerState.
Removed getExposedPortNumbers from ContainerState.
Added waitingFor method to DockerComposeContainer.
Removed StartupTimeout interface.
@barrycommins
Copy link
Contributor Author

The latest commit is my favourite type of commit, because it mostly removes code.

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.

Thank you for this @barrycommins! I'm happy for this to be merged following @bsideup's review - I don't have anything to add.

@barrycommins
Copy link
Contributor Author

Thanks for the review @rnorth, and thanks to @bsideup for helping beat this into shape.
I think it's a lot better now than it was when I first submitted it.
It's a feature I've been looking forward to for a while, so I'm happy I was able to help.

Went through about 10 commits, so I'm sure it will need a squash before merging.

Any other features/bugs you would like someone to look at, just let me know.

@bsideup bsideup merged commit 8558203 into testcontainers:master Mar 19, 2018
@bsideup
Copy link
Member

bsideup commented Mar 19, 2018

@barrycommins merged 🎉 Thanks a lot for this contribution 👍

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.

3 participants