Closes issue #19. Support multiple Docker annotations with networks. #105

hanleyt · 2018-01-26T17:03:24Z

No description provided.

FaustXVI · 2018-01-27T14:44:39Z

src/main/java/com/github/junit5docker/ContainerInfo.java

+import static java.util.Collections.emptySet;
+import static java.util.Collections.unmodifiableCollection;
+
+class ContainerInfo {


Thanks for fixing my primitive obsession on the container ID :) 👍

FaustXVI · 2018-01-27T15:04:49Z

src/main/java/com/github/junit5docker/DefaultDockerClient.java

+                               String msg = String.format("Multiple networks found with the same name: %s and %s",
+                                                          name1,
+                                                          name2);
+                               throw new IllegalStateException(msg);


Is it really something that can happen or is it a docker bug ?
If it's a docker bug, I think we could just get the first network having the right name. What do you think ?

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.
But testing it again now it seems you're right, that you can't have multiple networks with the same name. So i'll change it back to find first :)

@hanleyt @FaustXVI I think there is a case you can if they don't have the same scope — i.e. one is local (bridge) and the other would be swarm (overaly — created elsewhere on the cluster). That said, I think it's an edge case and shouldn't really happen that much 😓

@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 ?

Both would work (i.e. let the thing crash at network creation, or adding a random value to make sure it's unique)

FaustXVI · 2018-01-27T15:14:35Z

src/test/java/com/github/junit5docker/StartMultipleDockerAnnotationsIT.java

+    @Docker(image = "faustxvi/simple-two-ports", ports = @Port(exposed = 8082, inner = 8080),
+        networks = {TEST_NETWORK_1, TEST_NETWORK_2})})
+@DisplayName("Multiple docker containers with multiple networks")
+class StartMultipleDockerAnnotationsIT {


Thanks a lot for that tests !

We also have the DefaultDockerClientIT that you way want to add tests to (for the network). It will allow us to detect misusage of the Docker-java framework more easily (already helped us a lot :) )

Another thing we like to do it to add scenarios to the index.feature file. It allows you to document your feature in a more natural way and is used to generate the website (so you won't repeat yourself and the website is always up-to-date).

By the way, it looks to me like you're adding two features : multiple containers and network. I agree that the network doesn't make sens without the multiple container feature. But I think having multiple containers without (shared) networks still makes sense. What do you think ?

yes, agreed :)
I'll get working on those

FaustXVI · 2018-01-27T15:25:23Z

src/main/java/com/github/junit5docker/DockerExtension.java

+
+        containerInfo.getNetworkIds().forEach(networkId -> {
+            dockerClient.disconnectFromNetwork(containerId, networkId);
+            dockerClient.maybeRemoveNetwork(networkId);


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 networkIds. The DockerClient can do it for us.

I think that stopAndRemoveContainer is the perfect place to do so. Maybe we should rename it to stopAndCleanContainer ?

Yes, good idea! that makes sense and would be much cleaner

FaustXVI · 2018-01-27T15:27:06Z

src/main/java/com/github/junit5docker/DockerExtension.java


 class DockerExtension implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {

    private final DockerClientAdapter dockerClient;

-    private String containerId;
+    private final DockerLogChecker dockerLogChecker;


Awesome refactoring ! Makes the code way simpler :) 👍

thanks! it was motivated by checkstyle/pmd saying the class had too many dependencies :)

FaustXVI · 2018-01-27T15:31:55Z

src/main/java/com/github/junit5docker/DockerExtension.java

+    }
+
+    private static ExtensionContext.Store getStore(ExtensionContext context) {
+        return context.getStore(ExtensionContext.Namespace.GLOBAL);


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 ?

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.
But apparently there are no guarantees about the exension lifecycle, so its better to use the store to store state. Plus it could be used by other extensions if they wanted to access the test state.

https://blog.codefx.org/design/architecture/junit-5-extension-model/#Stateless

Awesome ! Good catch !

FaustXVI · 2018-01-27T15:36:11Z

src/main/java/com/github/junit5docker/Docker.java

@@ -20,6 +21,7 @@
 @Target({ElementType.TYPE})
 @Retention(RetentionPolicy.RUNTIME)
 @ExtendWith(DockerExtension.class)
+@Repeatable(value = Dockers.class)


If reapeted, don't we risk to call the DockerExtension several times ? Would removing the ExtendWith fix the problem (since you added it to the Dockers annotation)?

From the docs it seems that registering an extension a second time is fine, that the second attempt will be ignored.
http://junit.org/junit5/docs/current/user-guide/#extensions-registration-inheritance

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.

Great ! Let's go for it then :)

FaustXVI · 2018-01-27T15:40:25Z

src/test/java/com/github/junit5docker/DefaultDockerClientIT.java

@@ -90,7 +89,8 @@ public void ensureImageIsPulled() {
            @Test
            @DisplayName("start a container without ports")
            public void shouldStartContainer() {
-                String containerId = defaultDockerClient.startContainer(WANTED_IMAGE, emptyMap());
+                String containerId = defaultDockerClient.startContainer(WANTED_IMAGE, emptyMap(), null)


Could we avoid using null ? An empty array would be fine for me (declared in constants ?)

FaustXVI · 2018-01-29T17:57:32Z

src/main/java/com/github/junit5docker/DockerExtension.java

+    }
+
+    private static ExtensionContext.Store getStore(ExtensionContext context) {
+        return context.getStore(ExtensionContext.Namespace.GLOBAL);


Awesome ! Good catch !

FaustXVI · 2018-01-29T17:58:33Z

src/main/java/com/github/junit5docker/Docker.java

@@ -20,6 +21,7 @@
 @Target({ElementType.TYPE})
 @Retention(RetentionPolicy.RUNTIME)
 @ExtendWith(DockerExtension.class)
+@Repeatable(value = Dockers.class)


Great ! Let's go for it then :)

FaustXVI · 2018-01-29T18:01:52Z

src/main/java/com/github/junit5docker/DefaultDockerClient.java

+                               String msg = String.format("Multiple networks found with the same name: %s and %s",
+                                                          name1,
+                                                          name2);
+                               throw new IllegalStateException(msg);


@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 ?

FaustXVI · 2018-01-29T18:03:40Z

src/main/java/com/github/junit5docker/ContainerInfo.java

+        this.networkIds = networkIds == null ? emptySet() : unmodifiableCollection(networkIds);
+    }
+
+    String getContainerId() {


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.
What's your opinion on this ?

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.
So sure, I'll make that change. Thanks! :)

FaustXVI · 2018-03-24T11:42:20Z

@hanleyt hi there :)
How are you ?
Any news on his PR ? That's awesome work ! Almost there ! Do you need any help ?
Regards,

Support multiple Docker annotations (containers) with networks. Fixes…

463c6ac

… issue FaustXVI#19

vdemeester requested review from vdemeester and FaustXVI January 26, 2018 17:07

hanleyt force-pushed the support-starting-multiple-containers branch 3 times, most recently from d4bd9ea to 9054d60 Compare January 26, 2018 19:02

Fix checkstyle errors

854ceb5

hanleyt force-pushed the support-starting-multiple-containers branch from 9054d60 to 854ceb5 Compare January 26, 2018 19:19

hanleyt changed the title ~~Support multiple Docker annotations (containers) with networks. Fixes…~~ Closes #19. Support multiple Docker annotations with networks. Jan 26, 2018

hanleyt changed the title ~~Closes #19. Support multiple Docker annotations with networks.~~ Closes issue #19. Support multiple Docker annotations with networks. Jan 26, 2018

FaustXVI reviewed Jan 27, 2018

View reviewed changes

FaustXVI reviewed Jan 29, 2018

View reviewed changes

Ameausoone mentioned this pull request Apr 25, 2018

feat(regex): add feature to wait for a regex pattern in docker contai… #107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes issue #19. Support multiple Docker annotations with networks. #105

Closes issue #19. Support multiple Docker annotations with networks. #105

hanleyt commented Jan 26, 2018 •

edited

Loading

FaustXVI Jan 27, 2018

hanleyt Jan 29, 2018

FaustXVI Jan 27, 2018

hanleyt Jan 29, 2018

vdemeester Jan 29, 2018

FaustXVI Jan 29, 2018

vdemeester Jan 29, 2018

FaustXVI Jan 27, 2018

hanleyt Jan 29, 2018

FaustXVI Jan 27, 2018

hanleyt Jan 29, 2018

FaustXVI Jan 27, 2018

hanleyt Jan 29, 2018

FaustXVI Jan 27, 2018

hanleyt Jan 29, 2018

FaustXVI Jan 29, 2018

FaustXVI Jan 27, 2018

hanleyt Jan 29, 2018

FaustXVI Jan 29, 2018

FaustXVI Jan 27, 2018

hanleyt Jan 29, 2018

FaustXVI Jan 29, 2018

FaustXVI Jan 29, 2018

FaustXVI Jan 29, 2018

FaustXVI Jan 29, 2018

hanleyt Jan 29, 2018 •

edited

Loading

FaustXVI commented Mar 24, 2018

Closes issue #19. Support multiple Docker annotations with networks. #105

Are you sure you want to change the base?

Closes issue #19. Support multiple Docker annotations with networks. #105

Conversation

hanleyt commented Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanleyt Jan 29, 2018 • edited Loading

Choose a reason for hiding this comment

FaustXVI commented Mar 24, 2018

hanleyt commented Jan 26, 2018 •

edited

Loading

hanleyt Jan 29, 2018 •

edited

Loading