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

Feature/look in path #175

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
3 changes: 2 additions & 1 deletion .baseline/findbugs/excludeFilter.xml
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,14 @@
<!-- Ignore the following bug patterns in test code -->
<!-- (i.e., classes ending in 'Test' or 'Tests', and inner classes of same) -->
<Match>
<Class name="~.*\.*Tests?(\$.*)?" />
<Class name="~.*\.*(Test|Tests|Should)?(\$.*)?" />
<Or>
<Bug pattern="NP_NONNULL_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR"/> <!-- common in tests to have non-final variables instantiated in @Before methods, which FindBugs can't detect -->
<Bug pattern="NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE"/> <!-- if a null is dereferenced, test will fail anyway. Plus assertNotNull() is often a bad pattern. -->
<Bug pattern="RV_RETURN_VALUE_IGNORED_INFERRED"/> <!-- common in tests to call a method [that has a return value] to deliberately cause an exception, then test that exception -->
<Bug pattern="RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT"/> <!-- ditto above -->
<Bug pattern="URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD"/> <!-- @Rule ivars can be public and unused -->
<Bug pattern="DMI_HARDCODED_ABSOLUTE_FILENAME"/> <!-- tests should be able to refer to notional directories -->
</Or>
</Match>
</FindBugsFilter>
2 changes: 1 addition & 1 deletion docker-compose-rule-core/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ dependencies {
compile "com.google.guava:guava:$guavaVersion"
compile "joda-time:joda-time:$jodaVersion"
compile "com.github.zafarkhaja:java-semver:$javaSemverVersion"
compile "com.google.code.findbugs:jsr305:3.0.0"

compile 'com.jayway.awaitility:awaitility:1.6.5'

Expand All @@ -18,7 +19,6 @@ dependencies {
testCompile "org.hamcrest:hamcrest-all:$hamcrestVersion"
testCompile "org.mockito:mockito-core:$mockitoVersion"
testCompile "com.github.tomakehurst:wiremock:2.0.6-beta"
testCompile "com.google.code.findbugs:jsr305:3.0.0"
testCompile "com.github.stefanbirkner:system-rules:1.16.1"
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,35 +13,36 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.palantir.docker.compose.execution;

import static java.util.Arrays.asList;
import static java.util.stream.Collectors.toList;

import com.google.common.collect.ImmutableList;
import java.io.File;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.List;
import java.util.Optional;
import java.util.function.Predicate;
import java.util.stream.Stream;

public class DockerCommandLocations {
private static final Predicate<String> IS_NOT_NULL = path -> path != null;
private static final Predicate<String> FILE_EXISTS = path -> new File(path).exists();

private final List<String> possiblePaths;

public DockerCommandLocations(String... possiblePaths) {
this.possiblePaths = asList(possiblePaths);
}
private static final List<String> MAC_SEARCH_LOCATIONS = ImmutableList.of("/usr/local/bin", "/usr/bin");

public Optional<String> preferredLocation() {
private DockerCommandLocations() {

return possiblePaths.stream()
.filter(IS_NOT_NULL)
.filter(FILE_EXISTS)
.findFirst();
}

@Override
public String toString() {
return "DockerCommandLocations{possiblePaths=" + possiblePaths + "}";
public static List<Path> pathLocations() {
String path = Stream.of(System.getenv("PATH"), System.getenv("path"), System.getenv("Path"))

Choose a reason for hiding this comment

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

But what about CrazyOS that uses "pATh"? /s

.filter(s -> s != null)
.findFirst()
.orElse(File.pathSeparator);
Stream<String> pathLocations = Arrays.stream(path.split(File.pathSeparator));
return Stream.concat(pathLocations, MAC_SEARCH_LOCATIONS.stream())
.map(p -> Paths.get(p))
.collect(toList());
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
* Copyright 2016 Palantir Technologies, Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.palantir.docker.compose.execution;

import static java.util.Collections.singletonList;

import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.List;
import javax.annotation.Nullable;
import org.apache.commons.lang3.SystemUtils;
import org.immutables.value.Value;

@Value.Immutable
public abstract class DockerCommandLocator {

protected abstract String command();

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

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

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

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.Derived
protected String executableName() {
if (isWindows()) {
return command() + ".exe";
}
return command();
}

@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() {

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()
.stream()
.map(p -> p.resolve(executableName()))
.filter(Files::exists)
.findFirst()
.map(Path::toString)
.orElseThrow(() -> new IllegalStateException("Could not find " + command() + " in " + searchLocations()));
}

public static ImmutableDockerCommandLocator.Builder forCommand(String command) {
return ImmutableDockerCommandLocator.builder()
.command(command);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,12 @@
public abstract class DockerComposeExecutable implements Executable {
private static final Logger log = LoggerFactory.getLogger(DockerComposeExecutable.class);

private static final DockerCommandLocations DOCKER_COMPOSE_LOCATIONS = new DockerCommandLocations(
System.getenv("DOCKER_COMPOSE_LOCATION"),
"/usr/local/bin/docker-compose",
"/usr/bin/docker-compose"
);

private static String defaultDockerComposePath() {
String pathToUse = DOCKER_COMPOSE_LOCATIONS.preferredLocation()
.orElseThrow(() -> new IllegalStateException(
"Could not find docker-compose, looked in: " + DOCKER_COMPOSE_LOCATIONS));

DockerCommandLocator commandLocator = DockerCommandLocator.forCommand("docker-compose")
Copy link
Contributor

Choose a reason for hiding this comment

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

returning a builder just to do this looks kinda meh, might as well do the entire builder from here and delete the forCommand method

.locationOverride(System.getenv("DOCKER_COMPOSE_LOCATION"))
.build();
String pathToUse = commandLocator.getLocation();
log.debug("Using docker-compose found at " + pathToUse);

return pathToUse;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,6 @@
public abstract class DockerExecutable implements Executable {
private static final Logger log = LoggerFactory.getLogger(DockerExecutable.class);

private static final DockerCommandLocations DOCKER_LOCATIONS = new DockerCommandLocations(
System.getenv("DOCKER_LOCATION"),
"/usr/local/bin/docker",
"/usr/bin/docker"
);

@Value.Parameter protected abstract DockerConfiguration dockerConfiguration();

@Override
Expand All @@ -41,12 +35,11 @@ public final String commandName() {

@Value.Derived
protected String dockerPath() {
String pathToUse = DOCKER_LOCATIONS.preferredLocation()
.orElseThrow(() -> new IllegalStateException(
"Could not find docker, looked in: " + DOCKER_LOCATIONS));

DockerCommandLocator commandLocator = DockerCommandLocator.forCommand("docker")
.locationOverride(System.getenv("DOCKER_LOCATION"))
.build();
String pathToUse = commandLocator.getLocation();
log.debug("Using docker found at " + pathToUse);

return pathToUse;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,70 +1,69 @@
/*
* Copyright 2016 Palantir Technologies, Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.palantir.docker.compose.execution;

import static java.util.Optional.empty;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.Is.is;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.hasItems;

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

public class DockerCommandLocationsShould {
private static final String badLocation = "file/that/does/not/exist";
private static final String otherBadLocation = "another/file/that/does/not/exist";

@Rule public TemporaryFolder folder = new TemporaryFolder();
@Rule public EnvironmentVariables environmentVariables = new EnvironmentVariables();

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

private String goodLocation;
@Test public void
contain_the_contents_of_the_path_with_a_single_folder() {
environmentVariables.set("PATH", "/folder");

@Before
public void setup() throws IOException {
goodLocation = folder.newFile("docker-compose.yml").getAbsolutePath();
assertThat(DockerCommandLocations.pathLocations(), hasItem(Paths.get("/folder")));
}

@Test public void
provide_the_first_docker_command_location_if_it_exists() {
DockerCommandLocations dockerCommandLocations = new DockerCommandLocations(
badLocation,
goodLocation,
otherBadLocation);

assertThat(dockerCommandLocations.preferredLocation().get(),
is(goodLocation));
contain_the_contents_of_the_path_with_two_folders() {
environmentVariables.set("PATH", "/first" + File.pathSeparator + "/second");

assertThat(DockerCommandLocations.pathLocations(), hasItems(Paths.get("/first"), Paths.get("/second")));
}

@Test public void
skip_paths_from_environment_variables_that_are_unset() {
DockerCommandLocations dockerCommandLocations = new DockerCommandLocations(
System.getenv("AN_UNSET_DOCKER_COMPOSE_PATH"),
goodLocation);
contain_the_docker_for_mac_install_location_after_the_path() {
environmentVariables.set("PATH", "/folder");

assertThat(dockerCommandLocations.preferredLocation().get(),
is(goodLocation));
assertThat(DockerCommandLocations.pathLocations(), contains(Paths.get("/folder"), Paths.get("/usr/local/bin"), Paths.get("/usr/bin")));
}

@Test public void
have_no_preferred_path_when_all_possible_paths_are_all_invalid() {
DockerCommandLocations dockerCommandLocations = new DockerCommandLocations(
badLocation);
contain_the_contents_of_the_path_with_a_lowercase_environment_variable() {
environmentVariables.set("PATH", null);
environmentVariables.set("path", "/lowercase");

assertThat(dockerCommandLocations.preferredLocation(),
is(empty()));
assertThat(DockerCommandLocations.pathLocations(), hasItem(Paths.get("/lowercase")));
}

@Test public void
contain_the_contents_of_the_path_with_a_windows_environment_variable() {
environmentVariables.set("PATH", null);
environmentVariables.set("Path", "/windows");

assertThat(DockerCommandLocations.pathLocations(), hasItem(Paths.get("/windows")));
}

@Test public void
return_just_the_mac_locations_if_no_path_variable_is_present() {
environmentVariables.set("PATH", null);

assertThat(DockerCommandLocations.pathLocations(), contains(Paths.get("/usr/local/bin"), Paths.get("/usr/bin")));
}

}
Loading