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

Fix problems with copying files and Docker-Compose on Windows #514

Merged
merged 7 commits into from
Dec 11, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
1 change: 1 addition & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
*.sh text eol=lf
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ All notable changes to this project will be documented in this file.

## UNRELEASED
### Fixed
- Problems with using container based docker-compose on Windows ([\#514](https://github.com/testcontainers/testcontainers-java/pull/514))
- Problems with copying files on Windows ([\#514](https://github.com/testcontainers/testcontainers-java/pull/514))
- Fixed regression in 1.4.3 when using Docker Compose on Windows ([\#439](https://github.com/testcontainers/testcontainers-java/issues/439))
- Fixed local Docker Compose executable name resolution on Windows (#416)
- Fixed TAR composition on Windows (#444)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import com.google.common.util.concurrent.Uninterruptibles;
import org.apache.commons.lang.StringUtils;
import org.apache.commons.lang.SystemUtils;
import org.jetbrains.annotations.NotNull;
Copy link
Member

Choose a reason for hiding this comment

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

I would rather not use JB's annotations in our code base. Adds another dependency

Copy link
Member Author

@kiview kiview Dec 11, 2017

Choose a reason for hiding this comment

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

Oh that was not intended, wonder how it got in, thanks for spotting it.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are already using in in multiple places btw.

➜  testcontainers-java git:(docker-for-windows-compat) ✗ grep -r "import org.jetbrains.annotations.NotNull;" *
core/src/main/java/org/testcontainers/dockerclient/WindowsClientProviderStrategy.java:import org.jetbrains.annotations.NotNull;
core/src/main/java/org/testcontainers/dockerclient/AuditLoggingDockerClient.java:import org.jetbrains.annotations.NotNull;
core/src/main/java/org/testcontainers/dockerclient/UnixSocketClientProviderStrategy.java:import org.jetbrains.annotations.NotNull;
core/src/main/java/org/testcontainers/utility/MountableFile.java:import org.jetbrains.annotations.NotNull;
core/src/main/java/org/testcontainers/utility/AuditLogger.java:import org.jetbrains.annotations.NotNull;
core/src/main/java/org/testcontainers/utility/CommandLine.java:import org.jetbrains.annotations.NotNull;
core/src/main/java/org/testcontainers/utility/ComparableVersion.java:import org.jetbrains.annotations.NotNull;
core/src/main/java/org/testcontainers/containers/GenericContainer.java:import org.jetbrains.annotations.NotNull;
core/src/main/java/org/testcontainers/containers/FixedHostPortGenericContainer.java:import org.jetbrains.annotations.NotNull;
core/src/main/java/org/testcontainers/containers/VncRecordingSidekickContainer.java:import org.jetbrains.annotations.NotNull;
core/src/main/java/org/testcontainers/containers/startupcheck/MinimumDurationRunningStartupCheckStrategy.java:import org.jetbrains.annotations.NotNull;
core/src/test/java/org/testcontainers/junit/wait/HttpWaitStrategyTest.java:import org.jetbrains.annotations.NotNull;
core/src/test/java/org/testcontainers/junit/wait/AbstractWaitStrategyTest.java:import org.jetbrains.annotations.NotNull;
core/src/test/java/org/testcontainers/junit/wait/LogMessageWaitStrategyTest.java:import org.jetbrains.annotations.NotNull;
core/src/test/java/org/testcontainers/junit/wait/HostPortWaitStrategyTest.java:import org.jetbrains.annotations.NotNull;
core/src/test/java/org/testcontainers/junit/BaseDockerComposeTest.java:import org.jetbrains.annotations.NotNull;
core/src/test/java/org/testcontainers/utility/MountableFileTest.java:import org.jetbrains.annotations.NotNull;
modules/selenium/src/main/java/org/testcontainers/containers/BrowserWebDriverContainer.java:import org.jetbrains.annotations.NotNull;
modules/selenium/src/test/java/org/testcontainers/junit/BaseWebDriverContainerTest.java:import org.jetbrains.annotations.NotNull;
modules/postgresql/src/main/java/org/testcontainers/containers/PostgreSQLContainer.java:import org.jetbrains.annotations.NotNull;
modules/mysql/src/main/java/org/testcontainers/containers/MySQLContainer.java:import org.jetbrains.annotations.NotNull;
modules/jdbc/src/main/java/org/testcontainers/containers/JdbcDatabaseContainer.java:import org.jetbrains.annotations.NotNull;
modules/nginx/src/main/java/org/testcontainers/containers/NginxContainer.java:import org.jetbrains.annotations.NotNull;

I don't really care about using it in this place, shall I remove it?

Copy link
Member

Choose a reason for hiding this comment

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

I lose track of what the best No[nt]Null annotation is 😞
I don't think there's any harm in leaving this here, and if we want to get rid of the JetBrain annotation altogether we could do it everywhere at once.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed it, since I don't think it added anything useful.

import org.junit.runner.Description;
import org.rnorth.ducttape.ratelimits.RateLimiter;
import org.rnorth.ducttape.ratelimits.RateLimiterBuilder;
Expand Down Expand Up @@ -395,6 +396,10 @@ default void validateFileList(List<File> composeFiles) {
* Use Docker Compose container.
*/
class ContainerisedDockerCompose extends GenericContainer<ContainerisedDockerCompose> implements DockerCompose {

private static final String DOCKER_SOCKET_PATH = "//var/run/docker.sock";
Copy link
Member

Choose a reason for hiding this comment

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

should not start with double / (you add it in getDockerSocketHostPath)

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, still works in Linux interestingly ^^

Copy link
Member

Choose a reason for hiding this comment

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

well, on *nix systems, you can use as many / as you want :D

$ ls -la /////usr
total 0
drwxr-xr-x@  10 root  wheel    320 Sep 27 08:04 .
drwxr-xr-x   31 root  wheel    992 Nov 30 13:02 ..
drwxr-xr-x  976 root  wheel  31232 Nov 30 13:02 bin
drwxr-xr-x  263 root  wheel   8416 Nov 16 11:38 include
drwxr-xr-x  311 root  wheel   9952 Nov 30 13:02 lib
drwxr-xr-x  235 root  wheel   7520 Nov 30 13:02 libexec
drwxr-xr-x   15 root  wheel    480 Sep 26 22:58 local
drwxr-xr-x  246 root  wheel   7872 Nov 30 12:51 sbin
drwxr-xr-x   47 root  wheel   1504 Sep 27 08:04 share
drwxr-xr-x    5 root  wheel    160 Sep 21 06:32 standalone

public static final char UNIX_PATH_SEPERATOR = ':';

public ContainerisedDockerCompose(List<File> composeFiles, String identifier) {

super(TestcontainersConfiguration.getInstance().getDockerComposeContainerImage());
Expand All @@ -405,14 +410,14 @@ public ContainerisedDockerCompose(List<File> composeFiles, String identifier) {
// Map the docker compose file into the container
final File dockerComposeBaseFile = composeFiles.get(0);
final String pwd = dockerComposeBaseFile.getAbsoluteFile().getParentFile().getAbsolutePath();
final String containerPwd = MountableFile.forHostPath(pwd).getResolvedPath();
final String containerPwd = MountableFile.forHostPath(pwd).getFilesystemPath();

final List<String> absoluteDockerComposeFiles = composeFiles.stream()
.map(File::getAbsolutePath)
.map(MountableFile::forHostPath)
.map(MountableFile::getResolvedPath)
.map(MountableFile::getFilesystemPath)
.collect(toList());
final String composeFileEnvVariableValue = Joiner.on(File.pathSeparator).join(absoluteDockerComposeFiles);
final String composeFileEnvVariableValue = Joiner.on(UNIX_PATH_SEPERATOR).join(absoluteDockerComposeFiles); // we always need the UNIX path separator
logger().debug("Set env COMPOSE_FILE={}", composeFileEnvVariableValue);
addEnv(ENV_COMPOSE_FILE, composeFileEnvVariableValue);
addFileSystemBind(pwd, containerPwd, READ_ONLY);
Expand All @@ -421,12 +426,19 @@ public ContainerisedDockerCompose(List<File> composeFiles, String identifier) {
// as the docker daemon, just mapping the docker control socket is OK.
// As there seems to be a problem with mapping to the /var/run directory in certain environments (e.g. CircleCI)
// we map the socket file outside of /var/run, as just /docker.sock
addFileSystemBind("/var/run/docker.sock", "/docker.sock", READ_WRITE);
addFileSystemBind(getDockerSocketHostPath(), "/docker.sock", READ_WRITE);
addEnv("DOCKER_HOST", "unix:///docker.sock");
setStartupCheckStrategy(new IndefiniteWaitOneShotStartupCheckStrategy());
setWorkingDirectory(containerPwd);
}

@NotNull
private String getDockerSocketHostPath() {
return SystemUtils.IS_OS_WINDOWS
Copy link

@trajano trajano Jan 4, 2019

Choose a reason for hiding this comment

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

I think this is making an assumption that we are only using docker via their sockets interface. However in Docker Machine setups this may not be available It should be able to use the DOCKER_HOSTand DOCKER_CERT_PATH and DOCKER_TLS values.

I just checked master it appears to make an assumption that the socket file does exist.

? "/" + DOCKER_SOCKET_PATH
: DOCKER_SOCKET_PATH;
}

@Override
public void invoke() {
super.start();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,6 @@ public interface ClasspathTrait<SELF extends ClasspathTrait<SELF> & BuildContext
default SELF withFileFromClasspath(String path, String resourcePath) {
final MountableFile mountableFile = MountableFile.forClasspathResource(resourcePath);

return ((SELF) this).withFileFromPath(path, Paths.get(mountableFile.getFilesystemPath()));
return ((SELF) this).withFileFromPath(path, Paths.get(mountableFile.getResolvedPath()));
}
}
37 changes: 20 additions & 17 deletions core/src/main/java/org/testcontainers/utility/MountableFile.java
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
package org.testcontainers.utility;

import static lombok.AccessLevel.PACKAGE;
import static org.testcontainers.utility.PathUtils.recursiveDeleteDir;

import com.google.common.base.Charsets;
import lombok.Getter;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.compress.archivers.tar.TarArchiveEntry;
import org.apache.commons.compress.archivers.tar.TarArchiveOutputStream;
import org.apache.commons.lang.SystemUtils;
import org.jetbrains.annotations.NotNull;
import org.testcontainers.images.builder.Transferable;

import java.io.File;
import java.io.IOException;
import java.io.InputStream;
Expand All @@ -19,14 +25,9 @@
import java.util.Set;
import java.util.jar.JarEntry;
import java.util.jar.JarFile;
import lombok.Getter;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.compress.archivers.tar.TarArchiveEntry;
import org.apache.commons.compress.archivers.tar.TarArchiveOutputStream;
import org.apache.commons.lang.SystemUtils;
import org.jetbrains.annotations.NotNull;
import org.testcontainers.images.builder.Transferable;

import static lombok.AccessLevel.PACKAGE;
import static org.testcontainers.utility.PathUtils.recursiveDeleteDir;

/**
* An abstraction over files and classpath resources aimed at encapsulating all the complexity of generating
Expand Down Expand Up @@ -165,8 +166,8 @@ private static String unencodeResourceURIToFilePath(@NotNull final String resour
private String resolvePath() {
String result = getResourcePath();

if (SystemUtils.IS_OS_WINDOWS) {
result = PathUtils.createMinGWPath(result);
if (SystemUtils.IS_OS_WINDOWS && result.startsWith("/")) {
result = result.substring(1);
}

return result;
Expand All @@ -177,13 +178,15 @@ private String resolvePath() {
* into a container. If this is a classpath resource residing in a JAR, it will be extracted to
* a temporary location so that the Docker daemon is able to access it.
*
* TODO: rename method accordingly and check if really needed like this
*
* @return
*/
private String resolveFilesystemPath() {
String result = getResourcePath();

if (SystemUtils.IS_OS_WINDOWS && result.startsWith("/")) {
result = result.substring(1);
result = PathUtils.createMinGWPath(result).substring(1);
}

return result;
Expand Down Expand Up @@ -285,7 +288,7 @@ private void deleteOnExit(final Path path) {
*/
@Override
public void transferTo(final TarArchiveOutputStream outputStream, String destinationPathInTar) {
recursiveTar(destinationPathInTar, this.getFilesystemPath(), this.getFilesystemPath(), outputStream);
recursiveTar(destinationPathInTar, this.getResolvedPath(), this.getResolvedPath(), outputStream);
}

/*
Expand Down Expand Up @@ -325,7 +328,7 @@ private void recursiveTar(String entryFilename, String rootPath, String itemPath
@Override
public long getSize() {

final File file = new File(this.getFilesystemPath());
final File file = new File(this.getResolvedPath());
if (file.isFile()) {
return file.length();
} else {
Expand All @@ -340,7 +343,7 @@ public String getDescription() {

@Override
public int getFileMode() {
return getUnixFileMode(this.getFilesystemPath());
return getUnixFileMode(this.getResolvedPath());
}

private int getUnixFileMode(final String pathAsString) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ private Path createTempDir() throws IOException {
}

private void performChecks(final MountableFile mountableFile) {
final String mountablePath = mountableFile.getFilesystemPath();
final String mountablePath = mountableFile.getResolvedPath();
assertTrue("The filesystem path '" + mountablePath + "' can be found", new File(mountablePath).exists());
assertFalse("The filesystem path '" + mountablePath + "' does not contain any URL escaping", mountablePath.contains("%20"));
}
Expand Down