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

Allow to create a container file from a Transferable (#3814) #3815

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions core/build.gradle
Expand Up @@ -45,6 +45,8 @@ tasks.japicmp {
"org.testcontainers.utility.ResourceReaper#start(com.github.dockerjava.api.DockerClient)",
"org.testcontainers.utility.ResourceReaper#registerNetworkForCleanup(java.lang.String)",
"org.testcontainers.utility.ResourceReaper#removeNetworks(java.lang.String)",
"org.testcontainers.images.builder.Transferable#of(java.lang.String)",
"org.testcontainers.images.builder.Transferable#updateChecksum(java.util.zip.Checksum)"
]

fieldExcludes = []
Expand Down
15 changes: 14 additions & 1 deletion core/src/main/java/org/testcontainers/containers/Container.java
Expand Up @@ -12,6 +12,7 @@
import org.testcontainers.containers.startupcheck.StartupCheckStrategy;
import org.testcontainers.containers.traits.LinkableContainer;
import org.testcontainers.containers.wait.strategy.WaitStrategy;
import org.testcontainers.images.builder.Transferable;
import org.testcontainers.utility.LogUtils;
import org.testcontainers.utility.MountableFile;

Expand Down Expand Up @@ -169,11 +170,23 @@ default SELF withFileSystemBind(String hostPath, String containerPath) {
* Set the file to be copied before starting a created container
*
* @param mountableFile a Mountable file with path of source file / folder on host machine
* @param containerPath a destination path on conatiner to which the files / folders to be copied
* @param containerPath a destination path on container to which the files / folders to be copied
* @return this
*
* @deprecated Use {@link #withCopyToContainer(Transferable, String)} instead
*/
@Deprecated
SELF withCopyFileToContainer(MountableFile mountableFile, String containerPath);

/**
* Set the content to be copied before starting a created container
*
* @param transferable a Transferable
* @param containerPath a destination path on container to which the files / folders to be copied
* @return this
*/
SELF withCopyToContainer(Transferable transferable, String containerPath);

/**
* Add an environment variable to be passed to the container.
*
Expand Down
Expand Up @@ -23,6 +23,7 @@
import com.google.common.hash.Hashing;
import lombok.AccessLevel;
import lombok.Data;
import lombok.Getter;
import lombok.NonNull;
import lombok.Setter;
import lombok.SneakyThrows;
Expand All @@ -49,6 +50,7 @@
import org.testcontainers.containers.wait.strategy.WaitStrategyTarget;
import org.testcontainers.images.ImagePullPolicy;
import org.testcontainers.images.RemoteDockerImage;
import org.testcontainers.images.builder.Transferable;
import org.testcontainers.lifecycle.Startable;
import org.testcontainers.lifecycle.Startables;
import org.testcontainers.lifecycle.TestDescription;
Expand Down Expand Up @@ -186,8 +188,15 @@ public class GenericContainer<SELF extends GenericContainer<SELF>>
private Long shmSize;

// Maintain order in which entries are added, as earlier target location may be a prefix of a later location.
@Deprecated
private Map<MountableFile, String> copyToFileContainerPathMap = new LinkedHashMap<>();

// Maintain order in which entries are added, as earlier target location may be a prefix of a later location.
@Setter(AccessLevel.NONE)
bsideup marked this conversation as resolved.
Show resolved Hide resolved
@Getter(AccessLevel.MODULE)
@VisibleForTesting
private Map<Transferable, String> copyToTransferableContainerPathMap = new LinkedHashMap<>();

protected final Set<Startable> dependencies = new HashSet<>();

/**
Expand Down Expand Up @@ -415,6 +424,8 @@ private void tryStart(Instant startedAt) {

// TODO use single "copy" invocation (and calculate an hash of the resulting tar archive)
copyToFileContainerPathMap.forEach(this::copyFileToContainer);

copyToTransferableContainerPathMap.forEach(this::copyFileToContainer);
kiview marked this conversation as resolved.
Show resolved Hide resolved
}

connectToPortForwardingNetwork(createCommand.getNetworkMode());
Expand Down Expand Up @@ -530,33 +541,18 @@ private void tryStart(Instant startedAt) {
@VisibleForTesting
Checksum hashCopiedFiles() {
Checksum checksum = new Adler32();
copyToFileContainerPathMap.entrySet().stream().sorted(Entry.comparingByValue()).forEach(entry -> {
byte[] pathBytes = entry.getValue().getBytes();
// Add path to the hash
checksum.update(pathBytes, 0, pathBytes.length);

File file = new File(entry.getKey().getResolvedPath());
checksumFile(file, checksum);
});
Stream.of(copyToFileContainerPathMap, copyToTransferableContainerPathMap)
.flatMap(it -> it.entrySet().stream())
.sorted(Entry.comparingByValue()).forEach(entry -> {
byte[] pathBytes = entry.getValue().getBytes();
// Add path to the hash
checksum.update(pathBytes, 0, pathBytes.length);

entry.getKey().updateChecksum(checksum);
});
return checksum;
}

@VisibleForTesting
@SneakyThrows(IOException.class)
void checksumFile(File file, Checksum checksum) {
Path path = file.toPath();
checksum.update(MountableFile.getUnixFileMode(path));
if (file.isDirectory()) {
try (Stream<Path> stream = Files.walk(path)) {
stream.filter(it -> it != path).forEach(it -> {
checksumFile(it.toFile(), checksum);
});
}
} else {
FileUtils.checksum(file, checksum);
}
}

@UnstableAPI
@SneakyThrows(JsonProcessingException.class)
final String hash(CreateContainerCmd createCommand) {
Expand Down Expand Up @@ -1296,6 +1292,15 @@ public SELF withCopyFileToContainer(MountableFile mountableFile, String containe
return self();
}

/**
* {@inheritDoc}
*/
@Override
public SELF withCopyToContainer(Transferable transferable, String containerPath) {
copyToTransferableContainerPathMap.put(transferable, containerPath);
return self();
}

/**
* Get the IP address that this container may be reached on (may not be the local machine).
*
Expand Down
Expand Up @@ -5,12 +5,18 @@
import org.apache.commons.io.IOUtils;

import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.zip.Checksum;

public interface Transferable {

int DEFAULT_FILE_MODE = 0100644;
int DEFAULT_DIR_MODE = 040755;

static Transferable of(String string) {
return of(string.getBytes(StandardCharsets.UTF_8));
}
Comment on lines +16 to +18
Copy link
Member

Choose a reason for hiding this comment

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

This is the other change leading to japicmp error. I am not sure what is the desired pattern to avoid this.

Copy link
Member

Choose a reason for hiding this comment

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

wait, what? Added static method makes japicmp report it as binary incompatible change? o_O What does it say exactly?

Copy link
Member

Choose a reason for hiding this comment

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

See the other comment, it fails because of METHOD_NEW_DEFAULT rule violation. But I can't say if this is a FP in this case.


static Transferable of(byte[] bytes) {
return of(bytes, DEFAULT_FILE_MODE);
}
Expand All @@ -27,6 +33,11 @@ public byte[] getBytes() {
return bytes;
}

@Override
public void updateChecksum(Checksum checksum) {
checksum.update(bytes, 0, bytes.length);
}

@Override
public int getFileMode() {
return fileMode;
Expand Down Expand Up @@ -78,4 +89,8 @@ default byte[] getBytes() {
default String getDescription() {
return "";
}

default void updateChecksum(Checksum checksum) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we just remove this, to fix japicmp error?

Copy link
Member

Choose a reason for hiding this comment

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

What does it say?

Default methods should be fine, and, worst case scenario, we could just allowlist it

Copy link
Member

Choose a reason for hiding this comment

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

This is the report:
image

This is the explanation from Oracle docs on binary compatibility:

Adding a default method, or changing a method from abstract to default, does not break compatibility with pre-existing binaries, but may cause an IncompatibleClassChangeError if a pre-existing binary attempts to invoke the method.

Copy link
Member

Choose a reason for hiding this comment

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

hmm, of is clearly false positive (isn't a new default method, but static)

updateChecksum can be allow-listed too 👍

throw new UnsupportedOperationException("Provide implementation in subclass");
}
}
26 changes: 26 additions & 0 deletions core/src/main/java/org/testcontainers/utility/MountableFile.java
@@ -1,12 +1,15 @@
package org.testcontainers.utility;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Charsets;
import lombok.Getter;
import lombok.RequiredArgsConstructor;
import lombok.SneakyThrows;
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.compress.archivers.tar.TarConstants;
import org.apache.commons.io.FileUtils;
import org.apache.commons.lang3.SystemUtils;
import org.jetbrains.annotations.NotNull;
import org.testcontainers.DockerClientFactory;
Expand All @@ -28,6 +31,8 @@
import java.util.Set;
import java.util.jar.JarEntry;
import java.util.jar.JarFile;
import java.util.stream.Stream;
import java.util.zip.Checksum;

import static lombok.AccessLevel.PACKAGE;
import static org.testcontainers.utility.PathUtils.recursiveDeleteDir;
Expand Down Expand Up @@ -361,6 +366,27 @@ public String getDescription() {
return this.getResolvedPath();
}

@Override
public void updateChecksum(Checksum checksum) {
File file = new File(getResolvedPath());
checksumFile(file, checksum);
}

@SneakyThrows(IOException.class)
private void checksumFile(File file, Checksum checksum) {
Path path = file.toPath();
checksum.update(MountableFile.getUnixFileMode(path));
if (file.isDirectory()) {
try (Stream<Path> stream = Files.walk(path)) {
stream.filter(it -> it != path).forEach(it -> {
checksumFile(it.toFile(), checksum);
});
}
} else {
FileUtils.checksum(file, checksum);
}
}

@Override
public int getFileMode() {
return getUnixFileMode(this.getResolvedPath());
Expand Down
Expand Up @@ -20,7 +20,11 @@
import org.testcontainers.containers.startupcheck.OneShotStartupCheckStrategy;
import org.testcontainers.containers.startupcheck.StartupCheckStrategy;
import org.testcontainers.containers.wait.strategy.AbstractWaitStrategy;
import org.testcontainers.images.builder.Transferable;

import java.nio.charset.StandardCharsets;
import org.testcontainers.images.builder.ImageFromDockerfile;
import org.testcontainers.utility.MountableFile;

import java.util.Arrays;
import java.util.List;
Expand Down Expand Up @@ -75,6 +79,38 @@ public void shouldReportErrorAfterWait() {
}
}

@Test
public void shouldCopyTransferableAsFile() {
kiview marked this conversation as resolved.
Show resolved Hide resolved
try (
GenericContainer<?> container = new GenericContainer<>(TestImages.TINY_IMAGE)
.withStartupCheckStrategy(new NoopStartupCheckStrategy())
.withCopyToContainer(Transferable.of("test"), "/tmp/test")
.waitingFor(new WaitForExitedState(state -> state.getExitCodeLong() > 0))
.withCommand("sh", "-c", "grep -q test /tmp/test && exit 100")
) {
assertThatThrownBy(container::start)
.hasStackTraceContaining("Container exited with code 100");
}
}

@Test
public void shouldCopyTransferableAfterMountableFile() {
try (
GenericContainer<?> container = new GenericContainer<>(TestImages.TINY_IMAGE)
.withStartupCheckStrategy(new NoopStartupCheckStrategy())
.withCopyFileToContainer(
MountableFile.forClasspathResource("test_copy_to_container.txt"),
"/tmp/test"
)
.withCopyToContainer(Transferable.of("test"), "/tmp/test")
.waitingFor(new WaitForExitedState(state -> state.getExitCodeLong() > 0))
.withCommand("sh", "-c", "grep -q test /tmp/test && exit 100")
) {
assertThatThrownBy(container::start)
.hasStackTraceContaining("Container exited with code 100");
}
}

@Test
public void shouldOnlyPublishExposedPorts() {
ImageFromDockerfile image = new ImageFromDockerfile("publish-multiple")
Expand Down