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.

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

wants to merge 4 commits into from

Conversation

cfredri4
Copy link

To fix issue with docker-compose when system terminal is narrow. Also harmonize code according to RemoteBuilder.build.

Before this PR

Parsing of docker-compose output fails when system terminal is narrow

After this PR

==COMMIT_MSG==
Make parsing of docker-compose output independent of local system terminal column width.
==COMMIT_MSG==

Possible downsides?

Probably none.

To fix issue with docker-compose when system terminal is narrow. Also harmonize code according to RemoteBuilder.build.
@changelog-app
Copy link

changelog-app bot commented Jul 14, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Make parsing of docker-compose output independent of local system terminal column width.

Check the box to generate changelog(s)

  • Generate changelog entry

@policy-bot policy-bot bot requested a review from fawind July 14, 2020 08:47
Oops, wrong order
@cfredri4
Copy link
Author

Unit tests are failing but this seems unrelated?

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:

@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. ;-)

cfredri4 added 2 commits July 15, 2020 12:02
After review comment
Instead harmonize RemoteBuilder with LocalBuilder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants