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

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
* 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 com.google.common.collect.ImmutableList;
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 {

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

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.Default
protected List<String> macSearchLocations() {
return MAC_SEARCH_LOCATIONS;
}

@Value.Derived
protected String executableName() {
if (isWindows()) {
return command() + ".exe";
}
return command();
}

@Nullable
protected abstract String locationOverride();

public String getLocation() {
if (locationOverride() != null) {
return locationOverride();
}
return macSearchLocations()
.stream()
.map(p -> Paths.get(p, executableName()))
.filter(Files::exists)
.findFirst()
.map(Path::toString)
.orElse(executableName());
}

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 All @@ -66,4 +59,5 @@ public Process execute(String... commands) throws IOException {
public static ImmutableDockerExecutable.Builder builder() {
return ImmutableDockerExecutable.builder();
}

}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/*
* 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 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.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import org.junit.rules.TemporaryFolder;

public class DockerCommandLocatorShould {
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();

@Test public void
returns_the_command_name_when_no_other_paths_contain_command() {
DockerCommandLocator locator = DockerCommandLocator.forCommand(command)
.isWindows(false)
.build();

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

@Test public void
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(windowsCommand));
}

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

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

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

@Test public void
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)
.macSearchLocations(singletonList(macSearchFolder.toString()))
.isWindows(false)
.build();

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

}