Skip to content

Feature/look in path #175

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

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

Feature/look in path #175

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
195d9af
Locate docker and docker-compose using the path variable.
Mar 1, 2017
ab078a6
Merge branch 'feature/use-path-to-find-executables' of https://github…
hpryce Mar 7, 2017
a12076d
Reintroduce support for explicitly setting docker-compose location.
hpryce Mar 7, 2017
35ab870
Docker for Mac is not in the path when tests run from IDE.
hpryce Mar 7, 2017
252a88e
Split out finding possible command locations from testing those locat…
hpryce Mar 7, 2017
0a61921
Add jsr dep
hpryce Mar 7, 2017
d39fb31
Refactor to simply logic and improve test coverage by using Environme…
hpryce Mar 8, 2017
19791dd
Add case for Windows path variable "Path"
hpryce Mar 8, 2017
a734f16
Process builder will look for command on the path
hpryce Mar 8, 2017
fa10060
Merge branch 'master' into feature/look_in_path
hpryce Mar 8, 2017
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
Prev Previous commit
Next Next commit
Process builder will look for command on the path
  • Loading branch information
@hpryce
hpryce committed Mar 8, 2017
commit a734f16a6036699312d9b743e3f9fb55e9ffcf24
48 changes: 0 additions & 48 deletions ...rule-core/src/main/java/com/palantir/docker/compose/execution/DockerCommandLocations.java
View file
Open in desktop

This file was deleted.

27 changes: 14 additions & 13 deletions ...e-rule-core/src/main/java/com/palantir/docker/compose/execution/DockerCommandLocator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@
*/
package com.palantir.docker.compose.execution;

import static java.util.Collections.singletonList;

import com.google.common.collect.ImmutableList;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
Expand All @@ -28,13 +27,20 @@
@Value.Immutable
public abstract class DockerCommandLocator {

private static final List<String> MAC_SEARCH_LOCATIONS = ImmutableList.of("/usr/local/bin", "/usr/bin");

protected abstract String command();

@Value.Default
protected boolean isWindows() {
Copy link
Contributor

@felixdesouza felixdesouza Mar 7, 2017

Choose a reason for hiding this comment

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

returning a constant to represent a default where the constant is framed as a question is somewhat misleading (I still don't know what the default is)

Copy link
Contributor Author

@hpryce hpryce Mar 7, 2017

Choose a reason for hiding this comment

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

The default is to allow it to be overriden in tests, otherwise it should behave as a constant.

Would "useWindowsExecutableNaming" have been easier to understand?

Copy link
Contributor

@felixdesouza felixdesouza Mar 7, 2017

Choose a reason for hiding this comment

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

isWindows is fine, but I'm starting to think that making it a default doesn't really make any sense. Whoever creates this builder will know at the time what environment they're in, and it feels odd to be like "I'm not going to specify if I'm running on windows because defaults"

Unless of course this is for backcompat and it's used in client code then nevermind.

return SystemUtils.IS_OS_WINDOWS;
}

@Value.Default
protected List<String> macSearchLocations() {
return MAC_SEARCH_LOCATIONS;
}

@Value.Derived
protected String executableName() {
if (isWindows()) {
Expand All @@ -46,22 +52,17 @@ protected String executableName() {
@Nullable
protected abstract String locationOverride();

@Value.Derived
protected List<Path> searchLocations() {
if (locationOverride() == null) {
return DockerCommandLocations.pathLocations();
}
return singletonList(Paths.get(locationOverride()));
}

public String getLocation() {
Copy link

@samwright samwright Mar 8, 2017

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()
if (locationOverride() != null) {
return locationOverride();
}
return macSearchLocations()
.stream()
.map(p -> p.resolve(executableName()))
.map(p -> Paths.get(p, executableName()))
.filter(Files::exists)
.findFirst()
.map(Path::toString)
.orElseThrow(() -> new IllegalStateException("Could not find " + command() + " in " + searchLocations()));
.orElse(executableName());
}

public static ImmutableDockerCommandLocator.Builder forCommand(String command) {
Expand Down
1 change: 1 addition & 0 deletions ...mpose-rule-core/src/main/java/com/palantir/docker/compose/execution/DockerExecutable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,5 @@ public Process execute(String... commands) throws IOException {
public static ImmutableDockerExecutable.Builder builder() {
return ImmutableDockerExecutable.builder();
}

}
69 changes: 0 additions & 69 deletions ...ore/src/test/java/com/palantir/docker/compose/execution/DockerCommandLocationsShould.java
View file
Open in desktop

This file was deleted.

50 changes: 17 additions & 33 deletions ...-core/src/test/java/com/palantir/docker/compose/execution/DockerCommandLocatorShould.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,15 @@
*/
package com.palantir.docker.compose.execution;

import static java.util.Collections.singletonList;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.Is.is;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.contrib.java.lang.system.EnvironmentVariables;
import org.junit.rules.ExpectedException;
import org.junit.rules.TemporaryFolder;

Expand All @@ -36,63 +35,48 @@ public class DockerCommandLocatorShould {

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

@Rule public EnvironmentVariables environmentVariables = new EnvironmentVariables();

private Path firstPathFolder;

private String commandFileLocation;
private String windowsCommandFileLocation;

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

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

environmentVariables.set("PATH", firstPathFolder.toString());
}

@Test public void
provide_the_first_command_location() {
returns_the_command_name_when_no_other_paths_contain_command() {
DockerCommandLocator locator = DockerCommandLocator.forCommand(command)
.isWindows(false)
.build();
assertThat(locator.getLocation(), is(commandFileLocation));

assertThat(locator.getLocation(), is(command));
}

@Test public void
provide_the_first_command_location_on_windows() {
returns_the_command_name_with_exe_on_windows_when_no_other_paths_contain_command() {
DockerCommandLocator locator = DockerCommandLocator.forCommand(command)
.isWindows(true)
.build();
assertThat(locator.getLocation(), is(windowsCommandFileLocation));

assertThat(locator.getLocation(), is(windowsCommand));
}

@Test public void
fail_when_no_paths_contain_command() {
environmentVariables.set("PATH", null);
allow_the_path_to_be_overriden() throws IOException {
Path overrideFolder = folder.newFolder("override").toPath();
String overridenCommand = Files.createFile(overrideFolder.resolve(command)).toString();

DockerCommandLocator locator = DockerCommandLocator.forCommand(command)
.locationOverride(overridenCommand)
.isWindows(false)
.build();

exception.expect(IllegalStateException.class);
exception.expectMessage("Could not find " + command + " in [/usr/local/bin, /usr/bin]");
locator.getLocation();
assertThat(locator.getLocation(), is(overridenCommand));
}

@Test public void
allow_the_path_to_be_overriden() throws IOException {
Path overrideFolder = folder.newFolder("override").toPath();
String overridenCommand = Files.createFile(overrideFolder.resolve(command)).toString();
search_in_known_mac_install_locations_for_the_command() throws IOException {
Path macSearchFolder = folder.newFolder("override").toPath();
String commandLocation = Files.createFile(macSearchFolder.resolve(command)).toString();

DockerCommandLocator locator = DockerCommandLocator.forCommand(command)
.locationOverride(overrideFolder.toString())
.macSearchLocations(singletonList(macSearchFolder.toString()))
.isWindows(false)
.build();

assertThat(locator.getLocation(), is(overridenCommand));
assertThat(locator.getLocation(), is(commandLocation));
}

}