-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix docker-compose executable check for Win10 (#460) #461
Fix docker-compose executable check for Win10 (#460) #461
Conversation
Do we run tests for local compose on Windows? I have to check at home on my Windows box. I also wonder how https://github.com/bsnisar/testcontainers-java/blob/61d9b0e1972a8be0530d22368ee9ff200172ca6d/core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java#L512 works, if we have to append |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would prefer to make the value of COMPOSE_EXECUTABLE
OS dependent, if that's what we also need when calling the docker-compose
executable.
@@ -530,6 +531,17 @@ public void invoke() { | |||
} | |||
} | |||
|
|||
// #460 : docker-compose are not resolvable on Win10 machines because there is docker-compose.exe binary, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can omit this comment (or change it to correct Javadoc comment syntax, but I prefer omitting it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
@@ -530,6 +531,17 @@ public void invoke() { | |||
} | |||
} | |||
|
|||
// #460 : docker-compose are not resolvable on Win10 machines because there is docker-compose.exe binary, | |||
// but not docker-compose | |||
private boolean isDockerComposeExists() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer renaming the method to something like dockerComposeExecutableExists
.
Hi @kiview , in my case for Win10 docker-compose is accessible locally on my cmd: But during the check inside CommandLine(executable = "docker-compose"):
this loop will find (among other locations) somthing like C:\Program Files\Docker\Docker\Resources\bin where no docker-compose file: C:\Program Files\Docker\Docker\resources\bin>dir Directory of C:\Program Files\Docker\Docker\resources\bin 09/10/2017 12:51 PM .09/10/2017 12:51 PM .. 09/10/2017 12:51 PM 6,380,236 docker-compose.exe 09/10/2017 12:51 PM 2,436,096 docker-credential-wincred.exe 09/10/2017 12:51 PM 26,885,120 docker-machine.exe 09/10/2017 12:51 PM 19,135,488 docker.exe 09/10/2017 12:51 PM 8,162,816 notary.exe As a result even if the command is executable, run will be failed by
So, for me simple fix for executable check looks good enough to satisfy this case. |
@@ -530,6 +531,17 @@ public void invoke() { | |||
} | |||
} | |||
|
|||
// #460 : docker-compose are not resolvable on Win10 machines because there is docker-compose.exe binary, | |||
// but not docker-compose | |||
private boolean isDockerComposeExists() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we change this method to:
if (SystemUtils.IS_OS_WINDOWS) {
return CommandLine.executableExists(COMPOSE_EXECUTABLE + ".exe");
} else {
return CommandLine.executableExists(COMPOSE_EXECUTABLE);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variant more readable and looks better, but narrows all possible options to *.exe variant on Win machines.
How docker-compose executable looks on other Win platforms (not Win10) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we currently only support Docker for Windows on Windows 10 (see Windows support), I would be fine with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I will rewrite it.
Also, what about this variant:
return SystemUtils.IS_OS_WINDOWS
? CommandLine.executableExists(COMPOSE_EXECUTABLE + ".exe")
: CommandLine.executableExists(COMPOSE_EXECUTABLE);
@@ -530,6 +531,17 @@ public void invoke() { | |||
} | |||
} | |||
|
|||
// #460 : docker-compose are not resolvable on Win10 machines because there is docker-compose.exe binary, | |||
// but not docker-compose | |||
private boolean isDockerComposeExists() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we currently only support Docker for Windows on Windows 10 (see Windows support), I would be fine with this.
@kiview I have updated it. |
@@ -3,7 +3,7 @@ All notable changes to this project will be documented in this file. | |||
|
|||
## UNRELEASED | |||
### Fixed | |||
- Fixed local Docker Compose executable name resolution on Windows (#416) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant, was referencing both issues here ;)
@kiview Ouh, its my misstate. I have just fixed it. |
@bsnisar @kiview Seems the issue is still remains. testcontainers-java/core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java Line 468 in 540f567
And second time here: testcontainers-java/core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java Line 536 in 540f567
UPD: |
This is fix for issue: #460