Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use implicit strategy when docker.host configuration is set #4175

Merged
merged 3 commits into from
Jul 18, 2021
Merged

Use implicit strategy when docker.host configuration is set #4175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ea1482c
Use implicit strategy when `docker.host` configuration is set
rnorth Jun 8, 2021
6f91758
Simplify conditional logic
rnorth Jul 16, 2021
2cbd7ed
Update test to include checks for correct ordering
rnorth Jul 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion core/src/main/java/org/testcontainers/utility/TestcontainersConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -172,7 +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.
return getEnvVarOrUserProperty("docker.client.strategy", environment.get("TESTCONTAINERS_DOCKER_CLIENT_STRATEGY"));

// 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previous implementation was prioritizing this over TESTCONTAINERS_DOCKER_CLIENT_STRATEGY

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW I am surprised why the tests are passing :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL oops...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting; I'll add some more checks in the tests, but actually I think this order is better: we currently check env vars before properties anyway.

A. Currently in this PR the check order is:

TESTCONTAINERS_DOCKER_CLIENT_STRATEGY
DOCKER_CLIENT_STRATEGY
docker.client.strategy

B. Whereas previously in this PR it went:

DOCKER_CLIENT_STRATEGY
docker.client.strategy
TESTCONTAINERS_DOCKER_CLIENT_STRATEGY

C. Compared with the last release version:

TESTCONTAINERS_DOCKER_CLIENT_STRATEGY
docker.client.strategy

It would feel weird if we prioritise environment variables over properties in some cases but not this one, so I think this is more correct. A is more compatible with C than B was.

if (unprefixedEnvVarOrProperty != null) {
return unprefixedEnvVarOrProperty;
}

// If docker.host is set then EnvironmentAndSystemPropertyClientProviderStrategy is likely to work
String dockerHostProperty = getEnvVarOrUserProperty("docker.host", null);
if (dockerHostProperty != null) {
return EnvironmentAndSystemPropertyClientProviderStrategy.class.getCanonicalName();
}

// No value set, and no implicit value to use either
return null;
}

public String getTransportType() {
Expand Down
29 changes: 29 additions & 0 deletions core/src/test/java/org/testcontainers/utility/TestcontainersConfigurationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -155,6 +156,34 @@ 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");
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
public void shouldNotReadReuseFromClasspathProperties() {
assertFalse("no reuse by default", newConfig().environmentSupportsReuse());
Expand Down