-
Notifications
You must be signed in to change notification settings - Fork 7
Closes issue #19. Support multiple Docker annotations with networks. #105
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
package com.github.junit5docker; | ||
|
||
import java.util.Collection; | ||
|
||
import static java.util.Collections.emptySet; | ||
import static java.util.Collections.unmodifiableCollection; | ||
|
||
class ContainerInfo { | ||
|
||
private final String containerId; | ||
|
||
private final Collection<String> networkIds; | ||
|
||
ContainerInfo(String containerId, Collection<String> networkIds) { | ||
this.containerId = containerId; | ||
this.networkIds = networkIds == null ? emptySet() : unmodifiableCollection(networkIds); | ||
} | ||
|
||
String getContainerId() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, i forgot it last time :) Since this is just a immutable data structure, public fields or fine for me. I don't see any value in getters. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm I've always done this out of habit after reading effective java. But on further inspection its fine to access fields directly if the class isn't public, and even more so if the fields are immutable. |
||
return containerId; | ||
} | ||
|
||
Collection<String> getNetworkIds() { | ||
return networkIds; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,12 +3,17 @@ | |
import com.github.dockerjava.api.DockerClient; | ||
import com.github.dockerjava.api.exception.NotFoundException; | ||
import com.github.dockerjava.api.model.ExposedPort; | ||
import com.github.dockerjava.api.model.Network; | ||
import com.github.dockerjava.api.model.Ports; | ||
import com.github.dockerjava.core.DockerClientBuilder; | ||
import com.github.dockerjava.core.command.PullImageResultCallback; | ||
|
||
import java.util.Collection; | ||
import java.util.Collections; | ||
import java.util.HashSet; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Optional; | ||
import java.util.stream.Stream; | ||
|
||
import static com.github.dockerjava.api.model.ExposedPort.tcp; | ||
|
@@ -25,12 +30,67 @@ class DefaultDockerClient implements DockerClientAdapter { | |
} | ||
|
||
@Override | ||
public String startContainer(String wantedImage, Map<String, String> environment, PortBinding... portBinding) { | ||
public ContainerInfo startContainer(String wantedImage, | ||
Map<String, String> environment, | ||
String[] networkNames, | ||
PortBinding... portBinding) { | ||
Ports bindings = createPortBindings(portBinding); | ||
List<String> environmentStrings = createEnvironmentList(environment); | ||
String containerId = createContainer(wantedImage, bindings, environmentStrings); | ||
dockerClient.startContainerCmd(containerId).exec(); | ||
return containerId; | ||
Collection<String> networkIds = joinNetworks(networkNames, containerId); | ||
return new ContainerInfo(containerId, networkIds); | ||
} | ||
|
||
private Collection<String> joinNetworks(String[] networkNames, String containerId) { | ||
if (networkNames == null || networkNames.length == 0) { | ||
return Collections.emptySet(); | ||
} | ||
Collection<String> networkIds = new HashSet<>(); | ||
|
||
for (String network : networkNames) { | ||
String networkId = getNetworkId(network); | ||
dockerClient.connectToNetworkCmd() | ||
.withNetworkId(networkId) | ||
.withContainerId(containerId) | ||
.exec(); | ||
networkIds.add(networkId); | ||
} | ||
return networkIds; | ||
} | ||
|
||
private String getNetworkId(String networkName) { | ||
return getExistingNetworkId(networkName) | ||
.orElseGet(() -> dockerClient.createNetworkCmd().withName(networkName).exec().getId()); | ||
} | ||
|
||
private Optional<String> getExistingNetworkId(String networkName) { | ||
return dockerClient.listNetworksCmd() | ||
.exec() | ||
.stream() | ||
.filter(name -> name.getName().equals(networkName)) | ||
.reduce((name1, name2) -> { | ||
String msg = String.format("Multiple networks found with the same name: %s and %s", | ||
name1, | ||
name2); | ||
throw new IllegalStateException(msg); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it really something that can happen or is it a docker bug ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm for some reason i thought i saw this happening during development when the networks weren't being cleaned up correctly so I added this guard. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vdemeester well, I think it's the kind of edge case we actually don't want to handle. I think that when you start to have this kind of configuration, you're not really on a development machine (dev or ci). So maybe we should actually crash at the network creation time if the network already exists. What do you think ? or we can avoid the crash by appending a random value to it ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both would work (i.e. let the thing crash at network creation, or adding a random value to make sure it's unique) |
||
}) | ||
.map(Network::getId); | ||
} | ||
|
||
@Override | ||
public void disconnectFromNetwork(String containerId, String networkId) { | ||
dockerClient.disconnectFromNetworkCmd() | ||
.withContainerId(containerId) | ||
.withNetworkId(networkId) | ||
.exec(); | ||
} | ||
|
||
@Override | ||
public void maybeRemoveNetwork(String networkId) { | ||
if (dockerClient.inspectNetworkCmd().withNetworkId(networkId).exec().getContainers().isEmpty()) { | ||
dockerClient.removeNetworkCmd(networkId).exec(); | ||
} | ||
} | ||
|
||
@Override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,14 +3,15 @@ | |
import org.junit.jupiter.api.extension.ExtendWith; | ||
|
||
import java.lang.annotation.ElementType; | ||
import java.lang.annotation.Repeatable; | ||
import java.lang.annotation.Retention; | ||
import java.lang.annotation.RetentionPolicy; | ||
import java.lang.annotation.Target; | ||
|
||
import static com.github.junit5docker.WaitFor.NOTHING; | ||
|
||
/** | ||
* <p>This annotation is the junit-docker entry point.</p> | ||
* <p>This annotation is a junit-docker entry point.</p> | ||
* | ||
* <p>Adding this annotation to a test's class will start a docker container before running the tests and will be stop | ||
* at the end of the tests. This is done once per class.</p> | ||
|
@@ -20,6 +21,7 @@ | |
@Target({ElementType.TYPE}) | ||
@Retention(RetentionPolicy.RUNTIME) | ||
@ExtendWith(DockerExtension.class) | ||
@Repeatable(value = Dockers.class) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If reapeted, don't we risk to call the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the docs it seems that registering an extension a second time is fine, that the second attempt will be ignored. By keeping it this way we allow users to still use the single Docker annotation by itself, without always having to wrap it in the Dockers annotation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great ! Let's go for it then :) |
||
public @interface Docker { | ||
|
||
/** | ||
|
@@ -51,4 +53,11 @@ | |
* False if it should be created only once for the test class. | ||
*/ | ||
boolean newForEachCase() default true; | ||
|
||
/** | ||
* @return the names of the networks for the container to join. | ||
* Empty if the container has no networks to join. | ||
*/ | ||
String[] networks() default {}; | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,90 +6,69 @@ | |
import org.junit.jupiter.api.extension.BeforeEachCallback; | ||
import org.junit.jupiter.api.extension.ExtensionContext; | ||
|
||
import java.util.Arrays; | ||
import java.util.HashMap; | ||
import java.util.Map; | ||
import java.util.concurrent.CompletableFuture; | ||
import java.util.concurrent.ExecutorService; | ||
import java.util.concurrent.Executors; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.function.Supplier; | ||
import java.util.stream.Stream; | ||
|
||
import static java.util.concurrent.CompletableFuture.supplyAsync; | ||
import java.util.function.BiConsumer; | ||
import java.util.function.Predicate; | ||
|
||
class DockerExtension implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback { | ||
|
||
private final DockerClientAdapter dockerClient; | ||
|
||
private String containerId; | ||
private final DockerLogChecker dockerLogChecker; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome refactoring ! Makes the code way simpler :) 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks! it was motivated by checkstyle/pmd saying the class had too many dependencies :) |
||
|
||
DockerExtension() { | ||
this(new DefaultDockerClient()); | ||
} | ||
|
||
DockerExtension(DockerClientAdapter dockerClient) { | ||
this.dockerClient = dockerClient; | ||
this.dockerLogChecker = new DockerLogChecker(dockerClient); | ||
} | ||
|
||
@Override | ||
public void beforeAll(ExtensionContext containerExtensionContext) { | ||
Docker dockerAnnotation = findDockerAnnotation(containerExtensionContext); | ||
if (!dockerAnnotation.newForEachCase()) startContainer(dockerAnnotation); | ||
public void beforeAll(ExtensionContext context) { | ||
forEachDocker(context, docker -> !docker.newForEachCase(), this::startContainer); | ||
} | ||
|
||
@Override | ||
public void beforeEach(ExtensionContext context) { | ||
Docker dockerAnnotation = findDockerAnnotation(context); | ||
if (dockerAnnotation.newForEachCase()) startContainer(dockerAnnotation); | ||
forEachDocker(context, Docker::newForEachCase, this::startContainer); | ||
} | ||
|
||
private static void forEachDocker(ExtensionContext context, | ||
Predicate<Docker> predicate, | ||
BiConsumer<ExtensionContext, Docker> action) { | ||
Arrays.stream(findDockerAnnotations(context)) | ||
.filter(predicate) | ||
.forEach(docker -> action.accept(context, docker)); | ||
} | ||
|
||
private void startContainer(Docker dockerAnnotation) { | ||
private void startContainer(ExtensionContext context, Docker dockerAnnotation) { | ||
PortBinding[] portBindings = createPortBindings(dockerAnnotation); | ||
Map<String, String> environmentMap = createEnvironmentMap(dockerAnnotation); | ||
String imageReference = findImageName(dockerAnnotation); | ||
WaitFor waitFor = dockerAnnotation.waitFor(); | ||
containerId = dockerClient.startContainer(imageReference, environmentMap, portBindings); | ||
waitForLogAccordingTo(waitFor); | ||
} | ||
|
||
private void waitForLogAccordingTo(WaitFor waitFor) { | ||
String expectedLog = waitFor.value(); | ||
if (!WaitFor.NOTHING.equals(expectedLog)) { | ||
ExecutorService executor = Executors.newSingleThreadExecutor(); | ||
CompletableFuture<Boolean> logFound = supplyAsync(findFirstLogContaining(expectedLog), executor); | ||
executor.shutdown(); | ||
try { | ||
boolean termination = executor.awaitTermination(waitFor.timeoutInMillis(), TimeUnit.MILLISECONDS); | ||
if (!termination) { | ||
throw new AssertionError("Timeout while waiting for log : \"" + expectedLog + "\""); | ||
} | ||
if (!logFound.getNow(false)) { | ||
throw new AssertionError("\"" + expectedLog + "\" not found in logs and container stopped"); | ||
} | ||
} catch (InterruptedException e) { | ||
Thread.currentThread().interrupt(); | ||
} | ||
} | ||
} | ||
|
||
private Supplier<Boolean> findFirstLogContaining(String logToFind) { | ||
return () -> { | ||
try (Stream<String> logs = dockerClient.logs(containerId)) { | ||
return logs.anyMatch(log -> log.contains(logToFind)); | ||
} | ||
}; | ||
String[] networkNames = dockerAnnotation.networks(); | ||
ContainerInfo containerInfo = dockerClient.startContainer(imageReference, | ||
environmentMap, | ||
networkNames, | ||
portBindings); | ||
dockerLogChecker.waitForLogAccordingTo(waitFor, containerInfo.getContainerId()); | ||
getStore(context).put(dockerAnnotation, containerInfo); | ||
} | ||
|
||
private Docker findDockerAnnotation(ExtensionContext extensionContext) { | ||
private static Docker[] findDockerAnnotations(ExtensionContext extensionContext) { | ||
Class<?> testClass = extensionContext.getTestClass().get(); | ||
return testClass.getAnnotation(Docker.class); | ||
return testClass.getAnnotationsByType(Docker.class); | ||
} | ||
|
||
private String findImageName(Docker dockerAnnotation) { | ||
private static String findImageName(Docker dockerAnnotation) { | ||
return dockerAnnotation.image(); | ||
} | ||
|
||
private Map<String, String> createEnvironmentMap(Docker dockerAnnotation) { | ||
private static Map<String, String> createEnvironmentMap(Docker dockerAnnotation) { | ||
Map<String, String> environmentMap = new HashMap<>(); | ||
Environment[] environments = dockerAnnotation.environments(); | ||
for (Environment environment : environments) { | ||
|
@@ -98,7 +77,7 @@ private Map<String, String> createEnvironmentMap(Docker dockerAnnotation) { | |
return environmentMap; | ||
} | ||
|
||
private PortBinding[] createPortBindings(Docker dockerAnnotation) { | ||
private static PortBinding[] createPortBindings(Docker dockerAnnotation) { | ||
Port[] ports = dockerAnnotation.ports(); | ||
PortBinding[] portBindings = new PortBinding[ports.length]; | ||
for (int i = 0; i < ports.length; i++) { | ||
|
@@ -109,14 +88,29 @@ private PortBinding[] createPortBindings(Docker dockerAnnotation) { | |
} | ||
|
||
@Override | ||
public void afterAll(ExtensionContext containerExtensionContext) { | ||
Docker dockerAnnotation = findDockerAnnotation(containerExtensionContext); | ||
if (!dockerAnnotation.newForEachCase()) dockerClient.stopAndRemoveContainer(containerId); | ||
public void afterAll(ExtensionContext context) { | ||
forEachDocker(context, docker -> !docker.newForEachCase(), this::stopAndRemove); | ||
} | ||
|
||
@Override | ||
public void afterEach(ExtensionContext context) { | ||
Docker dockerAnnotation = findDockerAnnotation(context); | ||
if (dockerAnnotation.newForEachCase()) dockerClient.stopAndRemoveContainer(containerId); | ||
forEachDocker(context, Docker::newForEachCase, this::stopAndRemove); | ||
} | ||
|
||
private void stopAndRemove(ExtensionContext context, Docker docker) { | ||
ContainerInfo containerInfo = getStore(context).remove(docker, ContainerInfo.class); | ||
|
||
String containerId = containerInfo.getContainerId(); | ||
|
||
containerInfo.getNetworkIds().forEach(networkId -> { | ||
dockerClient.disconnectFromNetwork(containerId, networkId); | ||
dockerClient.maybeRemoveNetwork(networkId); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The goal of the dockerClient interface is to define the interface we'd like to have with docker. Since we want to clean up behind us, I would argue that we want to disconnect from all networks and (always?) remove them. If so, the Docker extension doesn't need to know the I think that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, good idea! that makes sense and would be much cleaner |
||
}); | ||
dockerClient.stopAndRemoveContainer(containerId); | ||
} | ||
|
||
private static ExtensionContext.Store getStore(ExtensionContext context) { | ||
return context.getStore(ExtensionContext.Namespace.GLOBAL); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't know about the store. Looks ok to me but I don't understand why you prefered this vs having a field in the class ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I understand its the recommended way to store state in extensions, and that extension classes should be stateless. It is essentially just a namespaced, hierarchical map, and having a map as a class field I imagine would perform in the exact same way. https://blog.codefx.org/design/architecture/junit-5-extension-model/#Stateless There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome ! Good catch ! |
||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
package com.github.junit5docker; | ||
|
||
import java.util.concurrent.CompletableFuture; | ||
import java.util.concurrent.ExecutorService; | ||
import java.util.concurrent.Executors; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.function.Supplier; | ||
import java.util.stream.Stream; | ||
|
||
import static java.util.concurrent.CompletableFuture.supplyAsync; | ||
|
||
class DockerLogChecker { | ||
|
||
private final DockerClientAdapter dockerClient; | ||
|
||
DockerLogChecker(DockerClientAdapter dockerClient) { | ||
this.dockerClient = dockerClient; | ||
} | ||
|
||
void waitForLogAccordingTo(WaitFor waitFor, String containerId) { | ||
String expectedLog = waitFor.value(); | ||
if (!WaitFor.NOTHING.equals(expectedLog)) { | ||
ExecutorService executor = Executors.newSingleThreadExecutor(); | ||
CompletableFuture<Boolean> logFound = supplyAsync(findFirstLogContaining(expectedLog, containerId), | ||
executor); | ||
executor.shutdown(); | ||
try { | ||
boolean termination = executor.awaitTermination(waitFor.timeoutInMillis(), TimeUnit.MILLISECONDS); | ||
if (!termination) { | ||
throw new AssertionError("Timeout while waiting for log : \"" + expectedLog + "\""); | ||
} | ||
if (!logFound.getNow(false)) { | ||
throw new AssertionError("\"" + expectedLog + "\" not found in logs and container stopped"); | ||
} | ||
} catch (InterruptedException e) { | ||
Thread.currentThread().interrupt(); | ||
} | ||
} | ||
} | ||
|
||
private Supplier<Boolean> findFirstLogContaining(String logToFind, String containerId) { | ||
return () -> { | ||
try (Stream<String> logs = dockerClient.logs(containerId)) { | ||
return logs.anyMatch(log -> log.contains(logToFind)); | ||
} | ||
}; | ||
} | ||
} |
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.
Thanks for fixing my primitive obsession on the container ID :) 👍
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.
:)