Feature/look in path by hpryce · Pull Request #175 · palantir/docker-compose-rule

hpryce · 2017-03-07T16:04:43Z

This combines #170 and #152 to provide support for finding docker from the path both on Windows and in existing setups.

This searches through the path variable's directories for the docker commands, instead of looking in predetermined OS-specific locations or requiring new environmental variables to be set. This also renames DockerCommandLocations to DockerCommandLocator, which better reflects what it is.

….com/samwright/docker-compose-rule into samwright-feature/use-path-to-find-executables

…ions.

felixdesouza · 2017-03-07T16:12:17Z

...se-rule-core/src/main/java/com/palantir/docker/compose/execution/DockerCommandLocations.java

@@ -1,47 +1,75 @@
 /*
 * Copyright 2016 Palantir Technologies, Inc. All rights reserved.


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

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

felixdesouza · 2017-03-07T16:13:10Z

...se-rule-core/src/main/java/com/palantir/docker/compose/execution/DockerCommandLocations.java

+        return Stream.concat(Stream.of(locationOverride()), pathLocations);
+    }
+
+    public Stream<Path> forCommand() {


Why are we returning Stream? They have use once semantics

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

felixdesouza · 2017-03-07T16:16:23Z

...pose-rule-core/src/main/java/com/palantir/docker/compose/execution/DockerCommandLocator.java

+    protected abstract String command();
+
+    @Value.Default
+    protected boolean isWindows() {


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)

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?

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.

felixdesouza · 2017-03-07T16:17:37Z

...e-rule-core/src/main/java/com/palantir/docker/compose/execution/DockerComposeExecutable.java

-                .orElseThrow(() -> new IllegalStateException(
-                        "Could not find docker-compose, looked in: " + DOCKER_COMPOSE_LOCATIONS));
-
+        DockerCommandLocator commandLocator = DockerCommandLocator.forCommand("docker-compose")


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

samwright · 2017-03-08T10:39:21Z

...se-rule-core/src/main/java/com/palantir/docker/compose/execution/DockerCommandLocations.java


-    public Optional<String> preferredLocation() {
+    @Value.Derived
+    protected String path() {


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"));

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

…ntVariables junit rule.

samwright · 2017-03-08T11:00:55Z

...ule-core/src/test/java/com/palantir/docker/compose/execution/DockerCommandLocatorShould.java

+    }
+
+    @Test public void
+    fail_when_no_paths_contain_command() {


This is no longer true

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

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.

samwright · 2017-03-08T11:05:32Z

...se-rule-core/src/main/java/com/palantir/docker/compose/execution/DockerCommandLocations.java

-    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"))


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

samwright · 2017-03-08T11:14:29Z

...pose-rule-core/src/main/java/com/palantir/docker/compose/execution/DockerCommandLocator.java

+        return singletonList(Paths.get(locationOverride()));
+    }
+
+    public String getLocation() {


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.

hpryce · 2017-03-08T16:24:30Z

Okay, it turns out ProcessBuilder looks for commands on the path (at least on Mac/Linux) and so there is no need to do this manually. I've updated the PR to do the simpler thing of only looking in the known Mac install locations and allow for explicitly setting it through an environment variable.

@samwright could you give this a spin and see if it is actually sufficient for Windows? If not we should find a library that will give us as because as @ChrisAlice pointed out the Windows path variable can contain expansions that would require us to look at other environment variables to correctly use.

samwright · 2017-03-08T16:40:51Z

Yep this works in Windows. I also tried just running "docker-compose.exe" without specifying its location and that worked too:

ProcessBuilder pb = new ProcessBuilder("docker-compose.exe", "-v");
Process p = pb.start();
p.waitFor();
BufferedReader r = new BufferedReader(new InputStreamReader(p.getInputStream(), StandardCharsets.UTF_8));
System.out.println(r.lines().collect(Collectors.toList())); // Prints: docker-compose version 1.11.2, build f963d76f

So it would probably be best to get rid of all that path variable stuff. Sorry for giving you a red herring!

dotCipher · 2018-04-17T19:52:16Z

@hpryce , is this still relevant / needed to merge? Or is this overcome-by-events?

nedtwigg · 2018-07-10T23:53:53Z

On windows, the just-released 0.34.0 is giving me Could not find docker-compose, even though docker-compose.exe is right on the path. Seems like this PR would fix it.

Sam Wright and others added 6 commits March 2, 2017 10:03

Merge branch 'feature/use-path-to-find-executables' of https://github…

ab078a6

….com/samwright/docker-compose-rule into samwright-feature/use-path-to-find-executables

Reintroduce support for explicitly setting docker-compose location.

a12076d

Docker for Mac is not in the path when tests run from IDE.

35ab870

Split out finding possible command locations from testing those locat…

252a88e

…ions.

Add jsr dep

0a61921

This was referenced Mar 7, 2017

Look for "docker-compose" in path #152

Closed

Feature/use path to find executables #170

Closed

felixdesouza reviewed Mar 7, 2017

View reviewed changes

samwright reviewed Mar 8, 2017

View reviewed changes

hpryce added 2 commits March 8, 2017 10:51

Refactor to simply logic and improve test coverage by using Environme…

d39fb31

…ntVariables junit rule.

Add case for Windows path variable "Path"

19791dd

samwright suggested changes Mar 8, 2017

View reviewed changes

Process builder will look for command on the path

a734f16

Merge branch 'master' into feature/look_in_path

fa10060

dotCipher mentioned this pull request Apr 30, 2018

Possible run docker-compose-rule on Windows? #232

Open

		@@ -1,47 +1,75 @@
		/*
		* Copyright 2016 Palantir Technologies, Inc. All rights reserved.

Conversation

hpryce commented Mar 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hpryce commented Mar 8, 2017

Uh oh!

samwright commented Mar 8, 2017

Uh oh!

dotCipher commented Apr 17, 2018

Uh oh!

nedtwigg commented Jul 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants