From ea1482ca63b635b0c2bc543cae1c4e4ba8f4cacb Mon Sep 17 00:00:00 2001 From: Richard North Date: Tue, 8 Jun 2021 14:13:02 +0100 Subject: [PATCH 1/3] Use implicit strategy when `docker.host` configuration is set --- .../utility/TestcontainersConfiguration.java | 16 ++++++++++++++- .../TestcontainersConfigurationTest.java | 20 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/testcontainers/utility/TestcontainersConfiguration.java b/core/src/main/java/org/testcontainers/utility/TestcontainersConfiguration.java index 9b2ab707a2e..e9757991937 100644 --- a/core/src/main/java/org/testcontainers/utility/TestcontainersConfiguration.java +++ b/core/src/main/java/org/testcontainers/utility/TestcontainersConfiguration.java @@ -13,6 +13,7 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.testcontainers.UnstableAPI; +import org.testcontainers.dockerclient.EnvironmentAndSystemPropertyClientProviderStrategy; import java.io.File; import java.io.FileNotFoundException; @@ -23,6 +24,7 @@ import java.net.MalformedURLException; import java.net.URL; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Properties; import java.util.concurrent.atomic.AtomicReference; @@ -172,7 +174,19 @@ public String getDockerClientStrategyClassName() { // Because of this overlap, and the desire to not change this specific TESTCONTAINERS_DOCKER_CLIENT_STRATEGY setting, // we special-case the logic here so that docker.client.strategy is used when reading properties files and // TESTCONTAINERS_DOCKER_CLIENT_STRATEGY is used when searching environment variables. - return getEnvVarOrUserProperty("docker.client.strategy", environment.get("TESTCONTAINERS_DOCKER_CLIENT_STRATEGY")); + String prefixedEnvVarStrategy = environment.get("TESTCONTAINERS_DOCKER_CLIENT_STRATEGY"); + String unprefixedEnvVarOrProperty = getEnvVarOrUserProperty("docker.client.strategy", null); + + // If docker.host is set then EnvironmentAndSystemPropertyClientProviderStrategy is likely to work + String implicitStrategy = null; + if (getEnvVarOrUserProperty("docker.host", null) != null) { + implicitStrategy = EnvironmentAndSystemPropertyClientProviderStrategy.class.getCanonicalName(); + } + + return Stream.of(prefixedEnvVarStrategy, unprefixedEnvVarOrProperty, implicitStrategy) + .filter(Objects::nonNull) + .findFirst() + .orElse(null); } public String getTransportType() { diff --git a/core/src/test/java/org/testcontainers/utility/TestcontainersConfigurationTest.java b/core/src/test/java/org/testcontainers/utility/TestcontainersConfigurationTest.java index 0fca36f0f2f..4024e219aab 100644 --- a/core/src/test/java/org/testcontainers/utility/TestcontainersConfigurationTest.java +++ b/core/src/test/java/org/testcontainers/utility/TestcontainersConfigurationTest.java @@ -2,6 +2,7 @@ import org.junit.Before; import org.junit.Test; +import org.testcontainers.dockerclient.EnvironmentAndSystemPropertyClientProviderStrategy; import java.util.HashMap; import java.util.Map; @@ -155,6 +156,25 @@ public void shouldReadDockerClientStrategyFromEnvironment() { assertEquals("Docker client strategy is changed by env var", "foo", newConfig().getDockerClientStrategyClassName()); } + @Test + public void shouldUseImplicitDockerClientStrategyWhenDockerHostPropertyIsSet() { + userProperties.remove("docker.client.strategy"); + userProperties.put("docker.host", "tcp://1.2.3.4:5678"); + assertEquals("Docker client strategy is implicitly set when docker host property is set", EnvironmentAndSystemPropertyClientProviderStrategy.class.getCanonicalName(), newConfig().getDockerClientStrategyClassName()); + } + + @Test + public void shouldNotUseImplicitDockerClientStrategyWhenDockerHostAndStrategyAreBothSet() { + userProperties.put("docker.client.strategy", "foo"); + userProperties.put("docker.host", "tcp://1.2.3.4:5678"); + assertEquals("Docker client strategy is can be explicitly set", "foo", newConfig().getDockerClientStrategyClassName()); + + userProperties.remove("docker.client.strategy"); + + environment.put("TESTCONTAINERS_DOCKER_CLIENT_STRATEGY", "bar"); + assertEquals("Docker client strategy is can be explicitly set", "bar", newConfig().getDockerClientStrategyClassName()); + } + @Test public void shouldNotReadReuseFromClasspathProperties() { assertFalse("no reuse by default", newConfig().environmentSupportsReuse()); From 6f9175808175bd25ae5f0f74385422f5b6ae1bb8 Mon Sep 17 00:00:00 2001 From: Richard North Date: Fri, 16 Jul 2021 15:18:24 +0100 Subject: [PATCH 2/3] Simplify conditional logic --- .../utility/TestcontainersConfiguration.java | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/testcontainers/utility/TestcontainersConfiguration.java b/core/src/main/java/org/testcontainers/utility/TestcontainersConfiguration.java index e9757991937..ca72e1bf6a3 100644 --- a/core/src/main/java/org/testcontainers/utility/TestcontainersConfiguration.java +++ b/core/src/main/java/org/testcontainers/utility/TestcontainersConfiguration.java @@ -174,19 +174,27 @@ public String getDockerClientStrategyClassName() { // Because of this overlap, and the desire to not change this specific TESTCONTAINERS_DOCKER_CLIENT_STRATEGY setting, // we special-case the logic here so that docker.client.strategy is used when reading properties files and // TESTCONTAINERS_DOCKER_CLIENT_STRATEGY is used when searching environment variables. + + // looks for TESTCONTAINERS_ prefixed env var only String prefixedEnvVarStrategy = environment.get("TESTCONTAINERS_DOCKER_CLIENT_STRATEGY"); + if (prefixedEnvVarStrategy != null) { + return prefixedEnvVarStrategy; + } + + // looks for unprefixed env var or unprefixed property String unprefixedEnvVarOrProperty = getEnvVarOrUserProperty("docker.client.strategy", null); + if (unprefixedEnvVarOrProperty != null) { + return unprefixedEnvVarOrProperty; + } // If docker.host is set then EnvironmentAndSystemPropertyClientProviderStrategy is likely to work - String implicitStrategy = null; - if (getEnvVarOrUserProperty("docker.host", null) != null) { - implicitStrategy = EnvironmentAndSystemPropertyClientProviderStrategy.class.getCanonicalName(); + String dockerHostProperty = getEnvVarOrUserProperty("docker.host", null); + if (dockerHostProperty != null) { + return EnvironmentAndSystemPropertyClientProviderStrategy.class.getCanonicalName(); } - return Stream.of(prefixedEnvVarStrategy, unprefixedEnvVarOrProperty, implicitStrategy) - .filter(Objects::nonNull) - .findFirst() - .orElse(null); + // No value set, and no implicit value to use either + return null; } public String getTransportType() { From 2cbd7ed967be127a52e157124f59ee4088823891 Mon Sep 17 00:00:00 2001 From: Richard North Date: Fri, 16 Jul 2021 16:23:08 +0100 Subject: [PATCH 3/3] Update test to include checks for correct ordering --- .../utility/TestcontainersConfigurationTest.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/core/src/test/java/org/testcontainers/utility/TestcontainersConfigurationTest.java b/core/src/test/java/org/testcontainers/utility/TestcontainersConfigurationTest.java index 4024e219aab..ff56f62e4e4 100644 --- a/core/src/test/java/org/testcontainers/utility/TestcontainersConfigurationTest.java +++ b/core/src/test/java/org/testcontainers/utility/TestcontainersConfigurationTest.java @@ -172,7 +172,16 @@ public void shouldNotUseImplicitDockerClientStrategyWhenDockerHostAndStrategyAre userProperties.remove("docker.client.strategy"); environment.put("TESTCONTAINERS_DOCKER_CLIENT_STRATEGY", "bar"); + userProperties.put("docker.client.strategy", "foo"); + assertEquals("Docker client strategy is can be explicitly set", "bar", newConfig().getDockerClientStrategyClassName()); + + environment.put("TESTCONTAINERS_DOCKER_CLIENT_STRATEGY", "bar"); + userProperties.remove("docker.client.strategy"); assertEquals("Docker client strategy is can be explicitly set", "bar", newConfig().getDockerClientStrategyClassName()); + + environment.remove("TESTCONTAINERS_DOCKER_CLIENT_STRATEGY"); + userProperties.put("docker.client.strategy", "foo"); + assertEquals("Docker client strategy is can be explicitly set", "foo", newConfig().getDockerClientStrategyClassName()); } @Test