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

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)

should use ContainerState as a super interface here

please use getters, not fields

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

Seems to be there since Docker 1.6.0 in 2015 (docker-compose 1.3.0).
docker/compose#1066
https://github.com/docker/compose/pull/1356/files#diff-84582fcbdd42b004bb763c973ef1e9b5

why?

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.

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

I would use ContainerState here

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?

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.

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)

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

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.

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

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?

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?

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

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

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.

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?

Yes, sorry, obviously keySet() returns a Set! :-)

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

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

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

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());
    }

just override (implement) both inComposeServiceWaitStrategyTarget , getExposedPortNumbers and getExposedPorts

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?

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?

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!

you can remove this method and use:

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

it will also make it thread safe

why this change is needed?

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.

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)

already set in computeIfAbsent

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.

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.

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.

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(...))

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())

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?

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?

sounds good

can't be null

return self()

a leftover? Logger should not be exposed

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?

Yes, I think we should remove it and create another logger in ExecInContainerPattern

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