#446 'set permission when copying' #467

glebsts · 2017-09-30T13:54:14Z

added method overloads, logic for setting file mode, few tests, some javadoc

…r setting file mode, few tests, some javadoc

glebsts · 2017-10-03T07:39:53Z

core/src/main/java/org/testcontainers/utility/MountableFile.java

@@ -10,7 +10,11 @@
 import org.jetbrains.annotations.NotNull;
 import org.testcontainers.images.builder.Transferable;

-import java.io.*;
+import java.io.File;


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

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

glebsts · 2017-10-03T07:42:22Z

core/src/test/java/org/testcontainers/utility/DirectoryTarResourceTest.java

@@ -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) {


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

😬 sorry!

glebsts · 2017-10-03T07:43:08Z

core/src/main/java/org/testcontainers/utility/MountableFile.java

    private final String path;
+    private final int permissions;


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

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?

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

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

glebsts · 2017-10-03T07:43:44Z

core/src/main/java/org/testcontainers/utility/MountableFile.java

@@ -33,7 +37,11 @@
 @Slf4j
 public class MountableFile implements Transferable {

+    private static final int BASE_FILE_MODE = 0100000;


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

Probably unlikely to change, so 👍

rnorth

Sorry for taking so long to get around to reviewing. This change looks good, so thank you for submitting it!

rnorth · 2017-10-15T18:54:20Z

core/src/main/java/org/testcontainers/utility/MountableFile.java

    private final String path;
+    private final int permissions;


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?

rnorth · 2017-10-15T18:59:54Z

core/src/main/java/org/testcontainers/utility/MountableFile.java

@@ -10,7 +10,11 @@
 import org.jetbrains.annotations.NotNull;
 import org.testcontainers.images.builder.Transferable;

-import java.io.*;
+import java.io.File;


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

rnorth · 2017-10-15T19:00:28Z

core/src/main/java/org/testcontainers/utility/MountableFile.java

@@ -33,7 +37,11 @@
 @Slf4j
 public class MountableFile implements Transferable {

+    private static final int BASE_FILE_MODE = 0100000;


Probably unlikely to change, so 👍

rnorth · 2017-10-15T19:04:28Z

core/src/test/java/org/testcontainers/utility/DirectoryTarResourceTest.java

@@ -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) {


😬 sorry!

rnorth · 2017-10-15T19:06:21Z

CHANGELOG.md

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


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

So it will get to 1.4.4?

glebsts · 2017-10-15T20:18:21Z

Sorry for taking so long to get around to reviewing. This change looks good, so thank you for submitting it!

Its ok. Just sad_panda because it was not reviewed, updated and merged before release. Now have to wait even more. Feels like lack of maintainers time :)
Despite of that still very happy of this library.

rnorth · 2017-10-16T20:19:59Z

@glebsts thanks for your understanding, and sorry again. There was a big existing queue of merged-but-unreleased things, and I had to draw the line somewhere.
I'd like to make the next release be small, so hopefully you won't have to wait much more!

# Conflicts: # CHANGELOG.md

…t can force 0 value; renamed field; updated changelog update

glebsts · 2017-11-09T08:57:23Z

Sorry, also took a while to deal with testcontainers, had been busy with other projects. Updated PR based on your comments.

glebsts · 2017-11-09T23:43:46Z

could smbd pls restart travis run?

rnorth · 2017-11-11T20:32:07Z

@glebsts Github is complaining (unnecessarily?) about merge conflicts, so I'm doing a manual rebase myself. It's looking good to me, so I'll merge in a bit if you don't object.

Gleb Stsenov added 2 commits September 30, 2017 16:31

#446 'set permission when copying' - added method overloads, logic fo…

cd5de8b

…r setting file mode, few tests, some javadoc

#446 'set permission when copying' - changelog update

7a453c3

glebsts mentioned this pull request Sep 30, 2017

ability to set file mode (when copying from Windows host to *nix container) #446

Closed

glebsts commented Oct 3, 2017

View reviewed changes

rnorth reviewed Oct 15, 2017

View reviewed changes

Gleb Stsenov added 2 commits November 9, 2017 10:21

Merge branch 'master' into 446-set-file-mode

7e08215

# Conflicts: # CHANGELOG.md

#446 'set permission when copying' - changed default value -1, so tha…

20f7d17

…t can force 0 value; renamed field; updated changelog update

rnorth mentioned this pull request Nov 11, 2017

Set file mode #486

Merged

rnorth closed this in #486 Nov 11, 2017

glebsts deleted the 446-set-file-mode branch September 13, 2021 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#446 'set permission when copying' #467

#446 'set permission when copying' #467

glebsts commented Sep 30, 2017

glebsts Oct 3, 2017 •

edited

Loading

rnorth Oct 15, 2017

glebsts Oct 3, 2017

rnorth Oct 15, 2017

glebsts Oct 3, 2017

rnorth Oct 15, 2017

glebsts Oct 15, 2017

rnorth Oct 16, 2017

glebsts Oct 3, 2017 •

edited

Loading

rnorth Oct 15, 2017

rnorth left a comment

rnorth Oct 15, 2017

rnorth Oct 15, 2017

rnorth Oct 15, 2017

rnorth Oct 15, 2017

rnorth Oct 15, 2017

glebsts Oct 15, 2017

rnorth Oct 16, 2017

glebsts commented Oct 15, 2017 •

edited

Loading

rnorth commented Oct 16, 2017

glebsts commented Nov 9, 2017

glebsts commented Nov 9, 2017

rnorth commented Nov 11, 2017

#446 'set permission when copying' #467

#446 'set permission when copying' #467

Conversation

glebsts commented Sep 30, 2017

glebsts Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glebsts Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glebsts commented Oct 15, 2017 • edited Loading

rnorth commented Oct 16, 2017

glebsts commented Nov 9, 2017

glebsts commented Nov 9, 2017

rnorth commented Nov 11, 2017

glebsts Oct 3, 2017 •

edited

Loading

glebsts Oct 3, 2017 •

edited

Loading

glebsts commented Oct 15, 2017 •

edited

Loading