Support for WaitStrategy on DockerComposeContainer - alternative approach #600
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| package org.testcontainers.containers; | ||
|
|
||
| import com.github.dockerjava.api.command.InspectContainerResponse; | ||
| import com.github.dockerjava.api.model.Container; | ||
| import lombok.EqualsAndHashCode; | ||
| import lombok.Getter; | ||
| import org.slf4j.Logger; | ||
| import org.testcontainers.DockerClientFactory; | ||
| import org.testcontainers.containers.wait.strategy.WaitStrategyTarget; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
|
|
||
| /** | ||
| * Class to provide a wait strategy target for services started through docker-compose | ||
| */ | ||
| @EqualsAndHashCode | ||
| class ComposeServiceWaitStrategyTarget implements WaitStrategyTarget { | ||
|
|
||
| private final Container container; | ||
| private final GenericContainer proxyContainer; | ||
| @Getter | ||
| private final Logger logger; | ||
| private Map<Integer, Integer> mappedPorts = new HashMap<>(); | ||
| @Getter | ||
| private List<Integer> exposedPorts = new ArrayList<>(); | ||
| private InspectContainerResponse containerInfo; | ||
|
|
||
| ComposeServiceWaitStrategyTarget(Container container, GenericContainer proxyContainer, | ||
| Logger logger, Map<Integer, Integer> mappedPorts) { | ||
| this.container = container; | ||
|
|
||
| this.proxyContainer = proxyContainer; | ||
| this.logger = logger; | ||
|
|
||
| if (mappedPorts != null) { | ||
| this.mappedPorts.putAll(mappedPorts); | ||
| this.exposedPorts.addAll(this.mappedPorts.keySet()); | ||
|
There was a problem hiding this comment. why do you store There was a problem hiding this comment. the I didn't want to create a new There was a problem hiding this comment.
Also, maybe we should make There was a problem hiding this comment. Yes, sorry, obviously
Isn't that functionally the same as There was a problem hiding this comment. it is :) Both are equally fine to me since we're talking about collections with just a few elements There was a problem hiding this comment.
Ah, I've just realised why this won't work. default Set<Integer> getExposedPortNumbers() {
return getExposedPorts().stream()
.map(this::getMappedPort)
.collect(Collectors.toSet());
}There was a problem hiding this comment. just override (implement) both in There was a problem hiding this comment. oh, I didn't notice that! Yes, sure! P.S. I checked the implementation and it seems it is only used in Let's just remove this method and use it's implementation in There was a problem hiding this comment. Despite the similar names, I think e.g. I took the name from a private method in There was a problem hiding this comment. Sorry, I deleted the original comment, because I was starting to confuse myself. |
||
| } | ||
| } | ||
|
|
||
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
| @Override | ||
| public Integer getMappedPort(int originalPort) { | ||
| return this.proxyContainer.getMappedPort(this.mappedPorts.get(originalPort)); | ||
| } | ||
|
|
||
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
| @Override | ||
| public String getContainerId() { | ||
| return this.container.getId(); | ||
| } | ||
|
|
||
| @Override | ||
| public InspectContainerResponse getContainerInfo() { | ||
|
There was a problem hiding this comment. you can remove this method and use: @Getter(lazy=true)
private InspectContainerResponse containerInfo = DockerClientFactory.instance().client().inspectContainerCmd(getContainerId()).exec();There was a problem hiding this comment. it will also make it thread safe |
||
| if(containerInfo == null) { | ||
| containerInfo = DockerClientFactory.instance().client().inspectContainerCmd(getContainerId()).exec(); | ||
| } | ||
| return containerInfo; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| package org.testcontainers.containers; | ||
|
|
||
| import com.github.dockerjava.api.command.InspectContainerResponse; | ||
| import com.github.dockerjava.api.exception.DockerException; | ||
| import com.github.dockerjava.api.model.ExposedPort; | ||
| import com.github.dockerjava.api.model.PortBinding; | ||
| import com.github.dockerjava.api.model.Ports; | ||
| import org.testcontainers.DockerClientFactory; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public interface ContainerState { | ||
|
|
||
| /** | ||
| * Get the IP address that this container may be reached on (may not be the local machine). | ||
| * | ||
| * @return an IP address | ||
| */ | ||
| default String getContainerIpAddress() { | ||
| return DockerClientFactory.instance().dockerHostIpAddress(); | ||
| } | ||
|
|
||
| /** | ||
| * @return is the container currently running? | ||
| */ | ||
| default Boolean isRunning() { | ||
| try { | ||
| return getContainerId() != null && DockerClientFactory.instance().client().inspectContainerCmd(getContainerId()).exec().getState().getRunning(); | ||
| } catch (DockerException e) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Get the actual mapped port for a first port exposed by the container. | ||
| * | ||
| * @return the port that the exposed port is mapped to | ||
| * @throws IllegalStateException if there are no exposed ports | ||
| */ | ||
| default Integer getFirstMappedPort() { | ||
| return getExposedPorts() | ||
| .stream() | ||
| .findFirst() | ||
| .map(this::getMappedPort) | ||
| .orElseThrow(() -> new IllegalStateException("Container doesn't expose any ports")); | ||
| } | ||
|
|
||
| /** | ||
| * Get the actual mapped port for a given port exposed by the container. | ||
| * | ||
| * @param originalPort the original TCP port that is exposed | ||
| * @return the port that the exposed port is mapped to, or null if it is not exposed | ||
| */ | ||
| Integer getMappedPort(int originalPort); | ||
|
|
||
| /** | ||
| * @return the exposed ports | ||
| */ | ||
| List<Integer> getExposedPorts(); | ||
|
|
||
| /** | ||
| * @return the container exposed port numbers mapped to ports exposed on the docker host | ||
| */ | ||
| default List<Integer> getExposedPortNumbers() { | ||
| return getExposedPorts().stream() | ||
| .map(this::getMappedPort) | ||
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| /** | ||
| * @return the port bindings | ||
| */ | ||
| default List<String> getPortBindings() { | ||
| List<String> portBindings = new ArrayList<>(); | ||
| final Ports hostPortBindings = this.getContainerInfo().getHostConfig().getPortBindings(); | ||
| for (Map.Entry<ExposedPort, Ports.Binding[]> binding : hostPortBindings.getBindings().entrySet()) { | ||
| for (Ports.Binding portBinding : binding.getValue()) { | ||
| portBindings.add(String.format("%s:%s", portBinding.toString(), binding.getKey())); | ||
| } | ||
| } | ||
| return portBindings; | ||
| } | ||
|
|
||
| /** | ||
| * @return the bound port numbers | ||
| */ | ||
| default List<Integer> getBoundPortNumbers() { | ||
| return getPortBindings().stream() | ||
| .map(PortBinding::parse) | ||
| .map(PortBinding::getBinding) | ||
| .map(Ports.Binding::getHostPortSpec) | ||
| .map(Integer::valueOf) | ||
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| /** | ||
| * @return the id of the container | ||
| */ | ||
| String getContainerId(); | ||
|
|
||
| /** | ||
| * @return the name of the container | ||
| */ | ||
| default String getContainerName() { | ||
| return getContainerInfo().getName(); | ||
| } | ||
|
|
||
| /** | ||
| * @return the container info | ||
| */ | ||
| InspectContainerResponse getContainerInfo(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add
@NonNulltomappedPortsargument and usethis.mappedPorts = new HashMap(mappedPorts)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A
ComposeServiceWaitStrategyTargetis created for every service returned bylistChildContainers()inDockerComposeContainer, which can return services with no exposed ports.for example:
and a test with
Only the redis container has exposed/mapped ports, so the
ComposeServiceWaitStrategyTargetfor thedbservice would have a null value formappedPorts.I'll change the behaviour to only create a
ComposeServiceWaitStrategyTargetfor services that have been explicitly exposed.There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
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
ComposeServiceWaitStrategyTargetwith no exposed ports, so the constructor would have to accept a null or empty value formappedPorts, which would mean returning a empty list fromgetExposedPorts.That goes against this idea:
It also means you could use
DockerComposeContainerlike this:or like this:
Is that slightly confusing for the user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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