#446 'set permission when copying' #467
#446 'set permission when copying' #467glebsts wants to merge 4 commits intotestcontainers:masterfrom glebsts:446-set-file-mode
Conversation
glebsts
commented
Sep 30, 2017
glebsts
commented
- added method overloads, logic for setting file mode, few tests, some javadoc
…r setting file mode, few tests, some javadoc
| import org.testcontainers.images.builder.Transferable; | ||
|
|
||
| import java.io.*; | ||
| import java.io.File; |
There was a problem hiding this comment.
my intellij configured to not allow wildcard imports. Dunno what's your policy.
There was a problem hiding this comment.
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").
| 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) { |
There was a problem hiding this comment.
Copied from stackoverflow. AssertJ has it built-in :\
| private static final int BASE_DIR_MODE = 0040000; | ||
|
|
||
| private final String path; | ||
| private final int permissions; |
There was a problem hiding this comment.
I first made it Integer, with default value null, but 0-check still looks better than null-check. Wdyt?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Makes sense. I am still for int type. Check for -1 may be?
There was a problem hiding this comment.
Go for it. Since the primitive nature doesn't leak outside of the class, I think it's reasonable.
| @Slf4j | ||
| public class MountableFile implements Transferable { | ||
|
|
||
| private static final int BASE_FILE_MODE = 0100000; |
There was a problem hiding this comment.
Base taken from Transferable, probably could be some bit math on Transferable.DEFAULT_FILE_MODE with making last three digits to zero.
There was a problem hiding this comment.
Probably unlikely to change, so 👍
rnorth
left a comment
rnorth
left a comment
There was a problem hiding this comment.
Sorry for taking so long to get around to reviewing. This change looks good, so thank you for submitting it!
| private static final int BASE_DIR_MODE = 0040000; | ||
|
|
||
| private final String path; | ||
| private final int permissions; |
There was a problem hiding this comment.
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?
| import org.testcontainers.images.builder.Transferable; | ||
|
|
||
| import java.io.*; | ||
| import java.io.File; |
There was a problem hiding this comment.
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").
| @Slf4j | ||
| public class MountableFile implements Transferable { | ||
|
|
||
| private static final int BASE_FILE_MODE = 0100000; |
There was a problem hiding this comment.
Probably unlikely to change, so 👍
| 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) { |
| - 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) |
There was a problem hiding this comment.
As I've released 1.4.3 now, please could you shift up to the top of the changelog?
There was a problem hiding this comment.
So it will get to 1.4.4?
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 :) |
|
@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. |
# Conflicts: # CHANGELOG.md
…t can force 0 value; renamed field; updated changelog update
|
Sorry, also took a while to deal with testcontainers, had been busy with other projects. Updated PR based on your comments. |
|
could smbd pls restart travis run? |
|
@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. |
2 participants
