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

Also add fixed COLUMNS to localMachine #511

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Also add fixed COLUMNS to localMachine #511

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fcec8a0
Also add fixed COLUMNS to localMachine
cfredri4 Jul 14, 2020
7d47035
Update DockerMachine.java
cfredri4 Jul 14, 2020
c0f8872
Revert LocalBuilder changes
cfredri4 Jul 15, 2020
a7b0383
Typo
cfredri4 Jul 15, 2020
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
15 changes: 11 additions & 4 deletions ...compose-rule-core/src/main/java/com/palantir/docker/compose/connection/DockerMachine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,19 @@ public LocalBuilder withEnvironment(Map<String, String> newEnvironment) {
public DockerMachine build() {
dockerType.validateEnvironmentVariables(systemEnvironment);
AdditionalEnvironmentValidator.validate(additionalEnvironment);
Map<String, String> combinedEnvironment = newHashMap();
combinedEnvironment.putAll(systemEnvironment);
combinedEnvironment.putAll(additionalEnvironment);

String dockerHost = systemEnvironment.getOrDefault(DOCKER_HOST, "");
return new DockerMachine(dockerType.resolveIp(dockerHost), ImmutableMap.copyOf(combinedEnvironment));
String hostIp = dockerType.resolveIp(dockerHost);

Map<String, String> environment = ImmutableMap.<String, String>builder()
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR @cfredri4!

For reference, this is the failing test:

docker-compose-rule/docker-compose-rule-core/src/test/java/com/palantir/docker/compose/connection/LocalBuilderShould.java

Lines 114 to 128 in 915c47f

@Test
public void override_system_environment_with_additional_environment() {
Map<String, String> systemEnv = ImmutableMap.<String, String>builder()
.put("ENV_1", "VAL_1")
.build();
Map<String, String> overrideEnv = ImmutableMap.<String, String>builder()
.put("ENV_1", "DIFFERENT_VALUE")
.build();
DockerMachine localMachine = new LocalBuilder(DAEMON, systemEnv)
.withEnvironment(overrideEnv)
.build();
assertThat(localMachine, not(containsEnvironment(systemEnv)));
assertThat(localMachine, containsEnvironment(overrideEnv));
}

It fails because it expects to override variables in the systemEnvironment with variables from the additionalEnvironment when both contain the same key. Previously, we used a HashMap for combining the environments, which allowed to overwrite an existing key with a new key-value pair.
When using ImmutableMap#builder, this is not allowed and results in an exception.

I would continue using the HashMap for building the combined environment as before.

Copy link
Author

Choose a reason for hiding this comment

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

Got it! Did this change in order to harmonize LocalBuilder and RemoteBuilder.
Now changed to allow same functionality in RemoteBuilder instead. ;-)

// 2019-12-17: newer docker-compose adjusts its output based on the number of columns available
// in the terminal. This interferes with parsing of the output of docker-compose, so "COLUMNS" is
// set to an artificially large value.
.putAll(systemEnvironment)
.putAll(additionalEnvironment)
.put("COLUMNS", "10000")
.build();
return new DockerMachine(hostIp, environment);
}
}

Expand Down