Skip to content
Prev Previous commit
Next Next commit
Split out finding possible command locations from testing those locat…
…ions.
  • Loading branch information
hpryce committed Mar 7, 2017
commit 252a88efb4545c51010add15ee3da888f8fe8ebf
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/*
* Copyright 2016 Palantir Technologies, Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we keeping or removing the License? It's removed here but kept in the following file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All files should have the Apache licence. I'll fix the ones that had it removed.

*/

package com.palantir.docker.compose.execution;

import java.io.File;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Map;
import java.util.regex.Pattern;
import java.util.stream.Stream;
import javax.annotation.Nullable;
import org.immutables.value.Value;

@Value.Immutable
public abstract class DockerCommandLocations {

private static final Pattern PATH_SPLITTER = Pattern.compile(File.pathSeparator);

@Value.Default
protected Map<String, String> env() {
return System.getenv();
}

@Value.Derived
protected String path() {

Choose a reason for hiding this comment

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

This doesn't work on Windows, where the path variable is "Path". You could add another if (path == null) path = env().get("Path"), or iterate through the env() map to find any key that equals "path" ignoring case, e.g.

        String path = getEnv().entrySet().stream()
                .filter(e -> e.getKey().equalsIgnoreCase("path"))
                .findFirst()
                .map(Map.Entry::getValue)
                .orElseThrow(() -> new IllegalStateException("Could not find path variable in env"));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, this should now cover PATH, path and Path.

String path = env().get("PATH");
if (path == null) {
path = env().get("path");
}
if (path == null) {
throw new IllegalStateException("No path environment variable found");
}
return path;
}

@Value.Check
protected void pathIsNotEmpty() {
if (path().isEmpty()) {
throw new IllegalStateException("Path variable was empty");
}
}

@Nullable
protected abstract String locationOverride();

@Value.Default
protected Stream<String> macSearchLocations() {
return Stream.of("/usr/local/bin", "/usr/bin");
}

private Stream<String> pathLocations() {
Stream<String> pathLocations = Stream.concat(PATH_SPLITTER.splitAsStream(path()), macSearchLocations());
if (locationOverride() == null) {
return pathLocations;
}
return Stream.concat(Stream.of(locationOverride()), pathLocations);
}

public Stream<Path> forCommand() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we returning Stream? They have use once semantics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Purely because it was extracted out the middle of a stream, I'll make it a list.

return pathLocations().map(p -> Paths.get(p));
}

public static DockerCommandLocations withLocationOverride(String override) {
return ImmutableDockerCommandLocations.builder()
.locationOverride(override)
.build();
}

public static ImmutableDockerCommandLocations.Builder builder() {
return ImmutableDockerCommandLocations.builder();
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,14 @@
*/
package com.palantir.docker.compose.execution;

import java.io.File;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.regex.Pattern;
import java.util.stream.Stream;
import javax.annotation.Nullable;
import org.apache.commons.lang3.SystemUtils;
import org.immutables.value.Value;

@Value.Immutable
public abstract class DockerCommandLocator {
private static final Pattern PATH_SPLITTER = Pattern.compile(File.pathSeparator);

protected abstract String command();

Expand All @@ -44,42 +39,18 @@ protected String executableName() {
return command();
}

@Value.Default
protected String path() {
String path = System.getenv("PATH");
if (path == null) {
throw new IllegalStateException("No path environment variable found");
}
return path;
}

@Value.Check
protected void pathIsNotEmpty() {
if (path().isEmpty()) {
throw new IllegalStateException("Path variable was empty");
}
}

@Nullable
protected abstract String locationOverride();

@Value.Default
protected Stream<String> macSearchLocations() {
return Stream.of("/usr/local/bin", "/usr/bin");
}

@Value.Derived
protected Stream<String> searchLocations() {
Stream<String> pathLocations = Stream.concat(PATH_SPLITTER.splitAsStream(path()), macSearchLocations());
if (locationOverride() == null) {
return pathLocations;
}
return Stream.concat(Stream.of(locationOverride()), pathLocations);
protected DockerCommandLocations searchLocations() {
return DockerCommandLocations.withLocationOverride(locationOverride());
}

public String getLocation() {

Choose a reason for hiding this comment

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

Previously DOCKER_COMPOSE_LOCATION would be the path to the binary, e.g. /path/to/docker-compose, but the logic here requires it to point to the directory in which to find docker-compose, i.e. /path/to. I think the previous behaviour is more obvious/expected.

You could remove the searchLocations method, and do the check for the location override here instead, e..g

if (locationOverride() == null) {
  return DockerCommandLocations.pathLocations().stream()....;
} else {
  return locationOverride();
}

It might even be worth moving DockerCommandLocations.pathLocations() to this class, but that's just a design thing.

return searchLocations()
.map(p -> Paths.get(p, executableName()))
.forCommand()
.map(p -> p.resolve(executableName()))
.filter(Files::exists)
.findFirst()
.map(Path::toString)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
/*
* Copyright 2016 Palantir Technologies, Inc. All rights reserved.
*/

package com.palantir.docker.compose.execution;

import static java.util.stream.Collectors.toList;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.is;
import static org.junit.Assume.assumeTrue;

import java.io.File;
import java.io.IOException;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.stream.Stream;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import org.junit.rules.TemporaryFolder;

public class DockerCommandLocationsShould {

@Rule public TemporaryFolder folder = new TemporaryFolder();

@Rule public ExpectedException exception = ExpectedException.none();

private Path firstPathFolder;
private Path secondPathFolder;

@Before
public void setup() throws IOException {
firstPathFolder = folder.newFolder("first").toPath();
secondPathFolder = folder.newFolder("second").toPath();
}

@Test public void
contain_the_contents_of_the_path_variable() {
assumeTrue("Path variable present", System.getenv("PATH") != null);

DockerCommandLocations commandLocations = DockerCommandLocations.builder()
.macSearchLocations(Stream.empty())
.build();
Stream<Path> foundLocations = commandLocations.forCommand();

String[] pathComponents = System.getenv("PATH").split(File.pathSeparator);
List<Path> expectedPaths = Arrays.stream(pathComponents)
.map(p -> Paths.get(p))
.collect(toList());
assertThat(foundLocations.collect(toList()), is(expectedPaths));
}

@Test public void
contain_the_contents_of_the_path_with_a_single_folder() {
DockerCommandLocations commandLocations = DockerCommandLocations.builder()
.env(Collections.singletonMap("PATH", firstPathFolder.toString()))
.macSearchLocations(Stream.empty())
.build();

assertThat(commandLocations.forCommand().collect(toList()), contains(firstPathFolder));
}

@Test public void
contain_the_contents_of_the_path_with_two_folders() {
DockerCommandLocations commandLocations = DockerCommandLocations.builder()
.env(Collections.singletonMap("PATH", firstPathFolder.toString() + File.pathSeparator + secondPathFolder.toString()))
.macSearchLocations(Stream.empty())
.build();

assertThat(commandLocations.forCommand().collect(toList()), contains(firstPathFolder, secondPathFolder));
}

@Test public void
contain_the_location_override_before_the_contents_of_the_path() {
DockerCommandLocations commandLocations = DockerCommandLocations.builder()
.locationOverride(firstPathFolder.toString())
.env(Collections.singletonMap("PATH", secondPathFolder.toString()))
.macSearchLocations(Stream.empty())
.build();

assertThat(commandLocations.forCommand().collect(toList()), contains(firstPathFolder, secondPathFolder));
}

@Test public void
contain_the_docker_for_mac_install_location_after_the_path() {
DockerCommandLocations commandLocations = DockerCommandLocations.builder()
.env(Collections.singletonMap("PATH", firstPathFolder.toString()))
.macSearchLocations(Stream.of(secondPathFolder.toString()))
.build();

assertThat(commandLocations.forCommand().collect(toList()), contains(firstPathFolder, secondPathFolder));
}

@Test public void
contain_the_contents_of_the_path_with_a_lowercase_environment_variable() {
DockerCommandLocations commandLocations = DockerCommandLocations.builder()
.env(Collections.singletonMap("path", firstPathFolder.toString()))
.macSearchLocations(Stream.empty())
.build();

assertThat(commandLocations.forCommand().collect(toList()), contains(firstPathFolder));
}

@Test public void
throw_an_exception_if_no_path_variable_is_present() {
exception.expect(IllegalStateException.class);
exception.expectMessage("No path environment variable found");

DockerCommandLocations.builder()
.env(Collections.emptyMap())
.build();
}

@Test public void
throw_an_exception_if_the_path_variable_is_empty() {
exception.expect(IllegalStateException.class);
exception.expectMessage("Path variable was empty");

DockerCommandLocations.builder()
.env(Collections.singletonMap("PATH", ""))
.build();
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -18,56 +18,40 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.Is.is;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import org.junit.rules.TemporaryFolder;

public class DockerCommandLocatorShould {
private static final String command = "command";
private static final String command = "not-a-real-command!";
private static final String windowsCommand = command + ".exe";

@Rule public TemporaryFolder folder = new TemporaryFolder();

@Rule public ExpectedException exception = ExpectedException.none();

private Path emptyFolder;

private Path firstPathFolder;
private Path secondPathFolder;

private String commandFileLocation;
private String windowsCommandFileLocation;

private String pathString;

@Before
public void setup() throws IOException {
emptyFolder = folder.newFolder("empty").toPath();
firstPathFolder = folder.newFolder("first").toPath();
secondPathFolder = folder.newFolder("second").toPath();

commandFileLocation = Files.createFile(firstPathFolder.resolve(command)).toString();
windowsCommandFileLocation = Files.createFile(firstPathFolder.resolve(windowsCommand)).toString();
Files.createFile(secondPathFolder.resolve(command));
Files.createFile(secondPathFolder.resolve(windowsCommand));

pathString = Stream.of(emptyFolder, firstPathFolder, secondPathFolder)
.map(Path::toString)
.collect(Collectors.joining(File.pathSeparator));
}

@Test public void
provide_the_first_command_location() {
DockerCommandLocator locator = DockerCommandLocator.forCommand(command)
.path(pathString)
.locationOverride(firstPathFolder.toString())
.isWindows(false)
.build();
assertThat(locator.getLocation(), is(commandFileLocation));
Expand All @@ -76,37 +60,16 @@ public void setup() throws IOException {
@Test public void
provide_the_first_command_location_on_windows() {
DockerCommandLocator locator = DockerCommandLocator.forCommand(command)
.path(pathString)
.locationOverride(firstPathFolder.toString())
.isWindows(true)
.build();
assertThat(locator.getLocation(), is(windowsCommandFileLocation));
}

@Test public void
provide_command_in_override_location() {
DockerCommandLocator locator = DockerCommandLocator.forCommand(command)
.path(firstPathFolder.toString())
.locationOverride(secondPathFolder.toString())
.isWindows(false)
.build();
assertThat(locator.getLocation(), is(secondPathFolder.resolve(command).toString()));
}

@Test public void
provide_command_in_docker_for_mac_install_location() {
DockerCommandLocator locator = DockerCommandLocator.forCommand(command)
.path(emptyFolder.toString())
.macSearchLocations(Stream.of(secondPathFolder.toString()))
.isWindows(false)
.build();
assertThat(locator.getLocation(), is(secondPathFolder.resolve(command).toString()));
}

@Test public void
fail_when_no_paths_contain_command() {

Choose a reason for hiding this comment

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

This is no longer true

Choose a reason for hiding this comment

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

Sorry, ignore this one. I got an error running this but I now can't recreate it.

Choose a reason for hiding this comment

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

Ah, found it again (I had @ignore'd the test, woops!)

Expected: (an instance of java.lang.IllegalStateException and exception with message a string containing "Could not find not-a-real-command! in [/usr/local/bin, /usr/bin]") but: exception with message a string containing "Could not find not-a-real-command! in [/usr/local/bin, /usr/bin]" message was "Could not find not-a-real-command! in [\usr\local\bin, \usr\bin]"

The "/usr/local/bin" string is converted to a Path in DockerCommandLocations.pathLocations(), which on Windows converts it to "\usr\local\bin" (aren't path separators fun?!)

You could change the expected message to just "Could not find " + command, or create a List<Path> defaultPaths = ImmutableList.of(Paths.get("/usr/local/bin"), Paths.get("/usr/bin")) and expect "Could not find " + command + " in " + defaultPaths.

DockerCommandLocator locator = DockerCommandLocator.forCommand(command)
.path(emptyFolder.toString())
.macSearchLocations(Stream.empty())
.locationOverride(null)
.isWindows(false)
.build();

Expand All @@ -115,13 +78,4 @@ public void setup() throws IOException {
locator.getLocation();
}

@Test public void
fail_when_no_path_variable_is_set() {
exception.expect(IllegalStateException.class);
exception.expectMessage("Path variable was empty");

DockerCommandLocator.forCommand(command)
.path("")
.build();
}
}