Add support for Docker networks. #372

bsideup · 2017-06-17T18:39:23Z

Hi.

One of the highly requested features, especially from users who wanted to run a fleet of containers, i.e. Hazelcast (hey @gAmUssA :D )

Also very helpful for Selenium + container-under-the-test scenario

As you can see, I experimented a bit with DSL, and it seems very usable :)

martin-g · 2017-06-19T20:37:55Z

It would be nice if some user documentation is added.

The signature of the following two is not very intuitive:

 public Network.JUnitRule network = newNetwork().as(Network.JUnitRule.class);

 Network network = newNetwork().as(Network.AutoCreated.class);

# Conflicts: # CHANGELOG.md

rnorth · 2017-06-21T20:55:08Z

I'd second @martin-g's comment here; the as approach is clever but I worry that it's not intuitive. I think anybody using this would have to be copying-and-pasting from an example. I'm not sure that somebody using just method signatures in-IDE or Javadocs would be able to work out that they have to do this.

Please can we re-think? Maybe something simpler (more coupled to JUnit) for the initial release, then go with a universal as(JUnit4Rule.class) pattern for both networks and containers in v2 (or similar)...

rnorth

Implementation wise I think this is fine, but the general comment on discoverability of this API is my only worry. Once we've worked out that it's 👍 from me!

rnorth · 2017-06-21T20:57:23Z

core/src/main/java/org/testcontainers/containers/GenericContainer.java

@@ -412,7 +418,14 @@ private void applyConfiguration(CreateContainerCmd createCommand) {
                .toArray(String[]::new);
        createCommand.withExtraHosts(extraHostsArray);

-        if (networkMode != null) {
+        if (network != null) {
+            if (!network.isCreated()) {


Is this likely to happen? If the network hasn't been created, could we infer that the user wants it to be created and trigger creation?

rnorth · 2017-06-21T20:59:38Z

core/src/test/java/org/testcontainers/containers/NetworkTest.java

+import static org.rnorth.visibleassertions.VisibleAssertions.assertEquals;
+import static org.testcontainers.containers.Network.newNetwork;
+
+@RunWith(Enclosed.class)


Neat - I didn't know about this!

rnorth · 2017-06-21T21:07:31Z

CHANGELOG.md

@@ -7,6 +7,7 @@ All notable changes to this project will be documented in this file.
 - Added pre-flight checks (can be disabled with `checks.disable` configuration property) (#363)
 - Removed unused Jersey dependencies (#361)
 - Fixed non-POSIX fallback for file attribute reading (#371)
+- Added support for the Docker networks (#372)


Trivial grammar note - the is unnecessary here

Thanks! 👍 One day I will learn how to use them properly :D

# Conflicts: # CHANGELOG.md

asafm

Nice addition!

asafm · 2017-06-23T13:55:22Z

core/src/main/java/org/testcontainers/containers/Network.java

+    boolean isCreated();
+
+    default boolean create() {
+        return getId() != null;


I would leave out the default implementation- it's too magic: you need to guess that getId is doing the actual create.
Regarding the Boolean - why not fail if can't create? In most chances you can't continue without a network.

default implementation here assumes that there is kinda no need to create a network. If id is known then what else to "create"?

The method returns boolean instead of an exception because it's up to the consumer how to react on that. However, I agree that before should throw if it returns false

Personally, I prefer the previous approach with .as() where the contract is clear, but for now we decided to go with an old approach until 2.0

In the impl you placed the logic of creating the network in getId() - that was my point. In the default implementation you assumed getId will be called this create the network if needed. Am I missing something?

I think @asafm point is, that getId() method here is definitly not a regular getter as one might expect, since it does the actual creation as well. So maybe it's having elegant code leveraging Lombok vs. self-explanatory code at this point.

id is the only thing needed to use this network. Default implementation assumes "no creation needed" semantic and returns true if id exists. Do not read the implementation to understand the idea of default implementation here.

Example:

class ExistingNetwork implements Network { String getId() { return "some-predefined-id"; } }

asafm · 2017-06-23T14:04:18Z

core/src/main/java/org/testcontainers/utility/ResourceReaper.java

+                // First try to remove by name
+                dockerClient.removeNetworkCmd(networkName).exec();
+            } catch (Exception e) {
+                LOGGER.trace("Error encountered removing network by name ({}) - it may not have been removed", networkName);


Two tests running with same network name will fail , at least the second create will. They will not understand why since the failure is hidden under trace level. I would keep this at error level or warn.

network's name is random (see NetworkImpl)

So no way of allowing me to specify a name? I'm Starting to build Testclusters, where I'm reusing containers across runs to shorten test duration. My assumption is same network name across runs. Would love to use the class created here for this.

If your use case would reuse the containers, it would reuse the network as well, I assume?

@asafm we don't allow users to set names of containers (and now networks) because it might lead to the names conflict (especially with parallel execution).

As @kiview said: if you reuse things, reuse the networks also

I'm reusing across builds, on same machine. Quite similar to docker compose. I can code it with the client my self, but I think it's harmless to leave the opening to set the name to who ever needs it. I'm ok with any direction you take.

Let's think about whether we could allow reuse between runs separately. It's probably quite doable, but for the sake of this PR let's resume later.

kiview · 2017-06-23T16:08:24Z

core/src/main/java/org/testcontainers/containers/Network.java

+
+    String getName();
+
+    Boolean getEnableIpv6();


Do you really want a Boolean?

kiview · 2017-06-23T16:13:20Z

core/src/main/java/org/testcontainers/containers/Network.java

+    boolean isCreated();
+
+    default boolean create() {
+        return getId() != null;


I think @asafm point is, that getId() method here is definitly not a regular getter as one might expect, since it does the actual creation as well. So maybe it's having elegant code leveraging Lombok vs. self-explanatory code at this point.

kiview · 2017-06-23T16:14:00Z

core/src/main/java/org/testcontainers/containers/Network.java

+
+        @Getter(lazy = true)
+        private final String id = ((Supplier<String>) () -> {
+            ResourceReaper.instance().registerNetworkForCleanup(getName());


I'd prefer to have this in a private method.

kiview · 2017-06-23T16:15:15Z

core/src/main/java/org/testcontainers/utility/ResourceReaper.java

+                // First try to remove by name
+                dockerClient.removeNetworkCmd(networkName).exec();
+            } catch (Exception e) {
+                LOGGER.trace("Error encountered removing network by name ({}) - it may not have been removed", networkName);


If your use case would reuse the containers, it would reuse the network as well, I assume?

kiview · 2017-06-23T16:16:15Z

core/src/main/java/org/testcontainers/containers/Network.java

+
+        private final String name = UUID.randomUUID().toString();
+
+        private Boolean enableIpv6;


Same thing with the Boolean here.

null here means "the value wasn't provided and withEnableIpv6 should not be called (use a default value from CreateNetworkCmd)

asafm · 2017-06-23T16:21:42Z

core/src/main/java/org/testcontainers/containers/Network.java

+
+    @SneakyThrows
+    @SuppressWarnings("unchecked")
+    default <T extends Network> T as(Class<T> type) {


I'm curious to that purpose of this class, which is related to another question I had: why have an interface? Do we plan on users extending and supplying their own Network implementation?

asafm · 2017-06-23T16:25:16Z

core/src/main/java/org/testcontainers/containers/Network.java

+        private Set<Consumer<CreateNetworkCmd>> createNetworkCmdModifiers = new LinkedHashSet<>();
+
+        @Getter(lazy = true)
+        private final String id = ((Supplier<String>) () -> {


I don't understand why a variable of type String receives a lambda function, casted to a Suplier of String and then executed? I mean - what's going on?

lazy = true is the answer

asafm · 2017-06-23T16:26:35Z

core/src/main/java/org/testcontainers/containers/Network.java

+        @Override
+        public boolean isCreated() {
+            // Lombok with @Getter(lazy = true) will use AtomicReference as a field type for id
+            return ((AtomicReference<String>) (Object) id).get() != null;


Just a personal opinion - that's too much magic for such a simple thing as having a field called Id of type String. Maybe it will more clear if for this field, Lombok won't be used.

bsideup · 2017-06-24T19:54:56Z

Simplified the code
Removed getters from Network interface (can be accessed with NetworkImpl if needed)
Removed isCreated
Less Lombok magic

Add support for the Docker networks.

a265dc0

bsideup added the enhancement label Jun 17, 2017

bsideup added this to the 1.3.1 milestone Jun 17, 2017

bsideup requested review from rnorth and kiview June 17, 2017 18:39

bsideup added 5 commits June 17, 2017 21:02

fix ResourceReaper

0de27b9

add an entry to CHANGELOG.md [ci skip]

93f97ed

Make Codacy happy

7fd6802

move JUnit tests upper in NetworkTest.java, minor fixes

12a777e

Codacy should learn some Lombok :)

9e690d5

bsideup added 2 commits June 20, 2017 22:21

Merge branch 'master' into networks

9e5a3f6

# Conflicts: # CHANGELOG.md

fix Travis

06d6e1d

rnorth requested changes Jun 21, 2017

View reviewed changes

bsideup added 3 commits June 23, 2017 10:23

Simplify network support ( the world is not ready yet :D )

4ebffb0

Merge branch 'master' into networks

c8c5530

# Conflicts: # CHANGELOG.md

re-add CHANGELOG.md entry

a2e6caa

bsideup modified the milestones: 1.4.0, 1.3.1 Jun 23, 2017

asafm reviewed Jun 23, 2017

View reviewed changes

kiview requested changes Jun 23, 2017

View reviewed changes

asafm reviewed Jun 23, 2017

View reviewed changes

bsideup changed the title ~~Add support for the Docker networks.~~ Add support for Docker networks. Jun 24, 2017

bsideup added 2 commits June 24, 2017 21:51

simplify Network implementation

14e698d

put back Getters on NetworkImpl

83368e8

kiview approved these changes Jun 24, 2017

View reviewed changes

rnorth approved these changes Jun 25, 2017

View reviewed changes

rnorth merged commit 3e513be into master Jun 25, 2017

rnorth deleted the networks branch June 25, 2017 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Docker networks. #372

Add support for Docker networks. #372

bsideup commented Jun 17, 2017

martin-g commented Jun 19, 2017

rnorth commented Jun 21, 2017

rnorth left a comment

rnorth Jun 21, 2017

rnorth Jun 21, 2017

rnorth Jun 21, 2017

bsideup Jun 23, 2017

asafm left a comment

asafm Jun 23, 2017

bsideup Jun 23, 2017

bsideup Jun 23, 2017

asafm Jun 23, 2017

kiview Jun 23, 2017

bsideup Jun 24, 2017 •

edited

Loading

asafm Jun 23, 2017

bsideup Jun 23, 2017

asafm Jun 23, 2017

kiview Jun 23, 2017

bsideup Jun 24, 2017

asafm Jun 24, 2017

rnorth Jun 25, 2017

kiview Jun 23, 2017

kiview Jun 23, 2017

kiview Jun 23, 2017

kiview Jun 23, 2017

kiview Jun 23, 2017

bsideup Jun 24, 2017

asafm Jun 23, 2017

asafm Jun 23, 2017

bsideup Jun 24, 2017

asafm Jun 23, 2017

bsideup commented Jun 24, 2017


		private final String name = UUID.randomUUID().toString();

		private Boolean enableIpv6;

Add support for Docker networks. #372

Add support for Docker networks. #372

Conversation

bsideup commented Jun 17, 2017

martin-g commented Jun 19, 2017

rnorth commented Jun 21, 2017

rnorth left a 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

asafm left a 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

bsideup Jun 24, 2017 • 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

bsideup commented Jun 24, 2017

bsideup Jun 24, 2017 •

edited

Loading