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

#446 'set permission when copying' #467

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 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 CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ All notable changes to this project will be documented in this file.
- Clarified wording of pre-flight check messages (#457, #436)
- Added caching of failure to find a docker daemon, so that subsequent tests fail fast. This is likely to be a significant improvement in situations where there is no docker daemon available, dramatically reducing run time and log output when further attempts to find the docker daemon cannot succeed.
- Allowing JDBC containers' username, password and DB name to be customized (#400, #354)
- Added support for explicitly setting file mode when copying file into container (#446)
Copy link
Member

Choose a reason for hiding this comment

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

As I've released 1.4.3 now, please could you shift up to the top of the changelog?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it will get to 1.4.4?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.


## [1.4.2] - 2017-07-25
### Fixed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,46 @@
*/
public interface FilesTrait<SELF extends FilesTrait<SELF> & BuildContextBuilderTrait<SELF>> {

/**
* Adds file to tarball copied into container.
* @param path in tarball
* @param file in host filesystem
* @return self
*/
default SELF withFileFromFile(String path, File file) {
return withFileFromPath(path, file.toPath());
return withFileFromPath(path, file.toPath(), 0);
}

/**
* Adds file to tarball copied into container.
* @param path in tarball
* @param filePath in host filesystem
* @return self
*/
default SELF withFileFromPath(String path, Path filePath) {
final MountableFile mountableFile = MountableFile.forHostPath(filePath);
return withFileFromPath(path, filePath, 0);
}

/**
* Adds file with given mode to tarball copied into container.
* @param path in tarball
* @param file in host filesystem
* @param mode octal value of posix file mode (000..777)
* @return self
*/
default SELF withFileFromFile(String path, File file, int mode) {
return withFileFromPath(path, file.toPath(), mode);
}

/**
* Adds file with given mode to tarball copied into container.
* @param path in tarball
* @param filePath in host filesystem
* @param mode octal value of posix file mode (000..777)
* @return self
*/
default SELF withFileFromPath(String path, Path filePath, int mode) {
final MountableFile mountableFile = MountableFile.forHostPath(filePath, mode);
return ((SELF) this).withFileFromTransferable(path, mountableFile);
}
}
59 changes: 55 additions & 4 deletions core/src/main/java/org/testcontainers/utility/MountableFile.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@
import org.jetbrains.annotations.NotNull;
import org.testcontainers.images.builder.Transferable;

import java.io.*;
import java.io.File;
Copy link
Contributor Author

@glebsts glebsts Oct 3, 2017

Choose a reason for hiding this comment

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

my intellij configured to not allow wildcard imports. Dunno what's your policy.

Copy link
Member

Choose a reason for hiding this comment

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

The policy is... clearly not well defined enough to be useful!
I've enabled a Codacy check to discourage wildcard imports and will check my IDE settings match for the future (I have no strong preference, but "shouldn't use" is easier to lint for than "should use").

import java.io.IOException;
import java.io.InputStream;
import java.io.UncheckedIOException;
import java.io.UnsupportedEncodingException;
import java.net.URL;
import java.net.URLDecoder;
import java.nio.file.Files;
Expand All @@ -33,7 +37,11 @@
@Slf4j
public class MountableFile implements Transferable {

private static final int BASE_FILE_MODE = 0100000;
Copy link
Contributor Author

@glebsts glebsts Oct 3, 2017

Choose a reason for hiding this comment

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

Base taken from Transferable, probably could be some bit math on Transferable.DEFAULT_FILE_MODE with making last three digits to zero.

Copy link
Member

Choose a reason for hiding this comment

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

Probably unlikely to change, so 👍

private static final int BASE_DIR_MODE = 0040000;

private final String path;
private final int permissions;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I first made it Integer, with default value null, but 0-check still looks better than null-check. Wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

I was going to say I'm fine with this, but had a thought: if we do this, then it's not possible for people to deliberately assign an all-zeroes file mode. That would be an incredibly strange thing for someone to do, but the resulting behaviour would be quite confusing.

Just a thought on naming too: given that the concept is referred to as * mode throughout the class, would it make more sense to use something similar, e.g. forcedMode or overrideMode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I am still for int type. Check for -1 may be?

Copy link
Member

Choose a reason for hiding this comment

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

Go for it. Since the primitive nature doesn't leak outside of the class, I think it's reasonable.


@Getter(lazy = true)
private final String resolvedPath = resolvePath();
Expand All @@ -50,7 +58,7 @@ public class MountableFile implements Transferable {
* @return a {@link MountableFile} that may be used to obtain a mountable path
*/
public static MountableFile forClasspathResource(@NotNull final String resourceName) {
return new MountableFile(getClasspathResource(resourceName, new HashSet<>()).toString());
return forClasspathResource(resourceName, 0);
}

/**
Expand All @@ -60,7 +68,7 @@ public static MountableFile forClasspathResource(@NotNull final String resourceN
* @return a {@link MountableFile} that may be used to obtain a mountable path
*/
public static MountableFile forHostPath(@NotNull final String path) {
return new MountableFile(new File(path).toURI().toString());
return forHostPath(path, 0);
}

/**
Expand All @@ -70,9 +78,43 @@ public static MountableFile forHostPath(@NotNull final String path) {
* @return a {@link MountableFile} that may be used to obtain a mountable path
*/
public static MountableFile forHostPath(final Path path) {
return new MountableFile(path.toAbsolutePath().toString());
return forHostPath(path, 0);
}

/**
* Obtains a {@link MountableFile} corresponding to a resource on the classpath (including resources in JAR files)
*
* @param resourceName the classpath path to the resource
* @param mode octal value of posix file mode (000..777)
* @return a {@link MountableFile} that may be used to obtain a mountable path
*/
public static MountableFile forClasspathResource(@NotNull final String resourceName, int mode) {
return new MountableFile(getClasspathResource(resourceName, new HashSet<>()).toString(), mode);
}

/**
* Obtains a {@link MountableFile} corresponding to a file on the docker host filesystem.
*
* @param path the path to the resource
* @param mode octal value of posix file mode (000..777)
* @return a {@link MountableFile} that may be used to obtain a mountable path
*/
public static MountableFile forHostPath(@NotNull final String path, int mode) {
return new MountableFile(new File(path).toURI().toString(), mode);
}

/**
* Obtains a {@link MountableFile} corresponding to a file on the docker host filesystem.
*
* @param path the path to the resource
* @param mode octal value of posix file mode (000..777)
* @return a {@link MountableFile} that may be used to obtain a mountable path
*/
public static MountableFile forHostPath(final Path path, int mode) {
return new MountableFile(path.toAbsolutePath().toString(), mode);
}


@NotNull
private static URL getClasspathResource(@NotNull final String resourcePath, @NotNull final Set<ClassLoader> classLoaders) {

Expand Down Expand Up @@ -291,6 +333,10 @@ public int getFileMode() {

private int getUnixFileMode(final String pathAsString) {
final Path path = Paths.get(pathAsString);
if (this.permissions > 0) {
return this.getModeValue(path);
}

try {
return (int) Files.getAttribute(path, "unix:mode");
} catch (IOException | UnsupportedOperationException e) {
Expand All @@ -305,4 +351,9 @@ private int getUnixFileMode(final String pathAsString) {
return mode;
}
}

private int getModeValue(final Path path) {
int result = Files.isDirectory(path) ? BASE_DIR_MODE : BASE_FILE_MODE;
return result | this.permissions;
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
package org.testcontainers.utility;

import org.hamcrest.Description;
import org.hamcrest.Matcher;
import org.hamcrest.core.IsCollectionContaining;
import org.junit.Test;
import org.testcontainers.containers.GenericContainer;
import org.testcontainers.containers.output.ToStringConsumer;
Expand All @@ -8,9 +11,13 @@
import org.testcontainers.images.builder.ImageFromDockerfile;

import java.io.File;
import java.util.Arrays;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;

import static org.hamcrest.CoreMatchers.allOf;
import static org.hamcrest.CoreMatchers.containsString;
import static org.rnorth.visibleassertions.VisibleAssertions.assertThat;
import static org.rnorth.visibleassertions.VisibleAssertions.assertTrue;

public class DirectoryTarResourceTest {
Expand Down Expand Up @@ -41,6 +48,35 @@ public void simpleRecursiveFileTest() throws TimeoutException {
assertTrue("The container has a file that was copied in via a recursive copy", results.contains("Used for DirectoryTarResourceTest"));
}

@Test
public void simpleRecursiveFileWithPermissionTest() throws TimeoutException {

WaitingConsumer wait = new WaitingConsumer();

final ToStringConsumer toString = new ToStringConsumer();

GenericContainer container = new GenericContainer(
new ImageFromDockerfile()
.withDockerfileFromBuilder(builder ->
builder.from("alpine:3.3")
.copy("/tmp/foo", "/foo")
.cmd("ls", "-al", "/")
.build()
).withFileFromFile("/tmp/foo", new File("/mappable-resource/test-resource.txt"),
0754))
.withStartupCheckStrategy(new OneShotStartupCheckStrategy())
.withLogConsumer(wait.andThen(toString));

container.start();
wait.waitUntilEnd(60, TimeUnit.SECONDS);

String listing = toString.toUtf8String();

assertThat("Listing shows that file is copied with mode requested.",
Arrays.asList(listing.split("\\n")),
exactlyNItems(1, allOf(containsString("-rwxr-xr--"), containsString("foo"))));
}

@Test
public void simpleRecursiveClasspathResourceTest() throws TimeoutException {
// This test combines the copying of classpath resources from JAR files with the recursive TAR approach, to allow JARed classpath resources to be copied in to an image
Expand Down Expand Up @@ -68,4 +104,31 @@ public void simpleRecursiveClasspathResourceTest() throws TimeoutException {
// ExternalResource.class is known to exist in a subdirectory of /org/junit so should be successfully copied in
assertTrue("The container has a file that was copied in via a recursive copy from a JAR resource", results.contains("content.txt"));
}

public static <T> Matcher<Iterable<? super T>> exactlyNItems(final int n, Matcher<? super T> elementMatcher) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from stackoverflow. AssertJ has it built-in :\

Copy link
Member

Choose a reason for hiding this comment

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

😬 sorry!

return new IsCollectionContaining<T>(elementMatcher) {
@Override
protected boolean matchesSafely(Iterable<? super T> collection, Description mismatchDescription) {
int count = 0;
boolean isPastFirst = false;

for (Object item : collection) {

if (elementMatcher.matches(item)) {
count++;
}
if (isPastFirst) {
mismatchDescription.appendText(", ");
}
elementMatcher.describeMismatch(item, mismatchDescription);
isPastFirst = true;
}

if (count != n) {
mismatchDescription.appendText(". Expected exactly " + n + " but got " + count);
}
return count == n;
}
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,16 @@
import java.nio.file.Files;
import java.nio.file.Path;

import static org.rnorth.visibleassertions.VisibleAssertions.assertEquals;
import static org.rnorth.visibleassertions.VisibleAssertions.assertFalse;
import static org.rnorth.visibleassertions.VisibleAssertions.assertTrue;

public class MountableFileTest {

private static final int TEST_FILE_MODE = 0532;
private static final int BASE_FILE_MODE = 0100000;
private static final int BASE_DIR_MODE = 0040000;

@Test
public void forClasspathResource() throws Exception {
final MountableFile mountableFile = MountableFile.forClasspathResource("mappable-resource/test-resource.txt");
Expand Down Expand Up @@ -60,6 +65,31 @@ public void forHostPathWithSpaces() throws Exception {
assertFalse("The resolved path does not contain an escaped space", mountableFile.getResolvedPath().contains("\\ "));
}

@Test
public void forClasspathResourceWithPermission() throws Exception {
final MountableFile mountableFile = MountableFile.forClasspathResource("mappable-resource/test-resource.txt",
TEST_FILE_MODE);

performChecks(mountableFile);
assertEquals("Valid file mode.", BASE_FILE_MODE | TEST_FILE_MODE, mountableFile.getFileMode());
}

@Test
public void forHostFilePathWithPermission() throws Exception {
final Path file = createTempFile("somepath");
final MountableFile mountableFile = MountableFile.forHostPath(file.toString(), TEST_FILE_MODE);
performChecks(mountableFile);
assertEquals("Valid file mode.", BASE_FILE_MODE | TEST_FILE_MODE, mountableFile.getFileMode());
}

@Test
public void forHostDirPathWithPermission() throws Exception {
final Path dir = createTempDir();
final MountableFile mountableFile = MountableFile.forHostPath(dir.toString(), TEST_FILE_MODE);
performChecks(mountableFile);
assertEquals("Valid dir mode.", BASE_DIR_MODE | TEST_FILE_MODE, mountableFile.getFileMode());
}

@SuppressWarnings("ResultOfMethodCallIgnored")
@NotNull
private Path createTempFile(final String name) throws IOException {
Expand All @@ -72,6 +102,11 @@ private Path createTempFile(final String name) throws IOException {
return file;
}

@NotNull
private Path createTempDir() throws IOException {
return Files.createTempDirectory("testcontainers");
}

private void performChecks(final MountableFile mountableFile) {
final String mountablePath = mountableFile.getFilesystemPath();
assertTrue("The filesystem path '" + mountablePath + "' can be found", new File(mountablePath).exists());
Expand Down