Recursive TAR copy for docker context #313
Conversation
|
|
||
| @Override | ||
| public int getFileMode() { | ||
| return DEFAULT_FILE_MODE | (Files.isExecutable(filePath) ? 0755 : 0); |
There was a problem hiding this comment.
please make sure that we don't lose executable flags
There was a problem hiding this comment.
Yep - this is important. My implementation is in MountableFile - do you see any issues with it? I've not yet tested this on Windows, which is something I need to do.
|
|
||
| performChecks(mountableFile); | ||
|
|
||
| assertTrue("The resolved path contains the original space", mountableFile.getResolvedPath().contains(" "));assertFalse("The resolved path does not contain an escaped space", mountableFile.getResolvedPath().contains("\\ ")); |
There was a problem hiding this comment.
Hmm, no idea how that happened - will fix.
|
|
||
| @Test | ||
| public void forClasspathResourceFromJar() throws Exception { | ||
| final MountableFile mountableFile = MountableFile.forClasspathResource("docker-java.properties"); |
There was a problem hiding this comment.
Maybe we should use some other file to avoid the situation when they change docker-java.properties to something else? Some `META-INF/MANIFEST.MD" IMO is better
There was a problem hiding this comment.
Fair point. I wanted to make it be something that's specific and ~ only present in one/few JARs so that we can be sure that the full classpath is searched. i.e. it felt like searching for a resource that's trivially findable in any JAR might not be all that thorough. I used docker-java.properties but this is admittedly a fairly lazy choice 🙉
I guess the real non-lazy solution would be to create a synthetic JAR with some interesting resources in it for testing purposes (maybe published with jitpack or as a local system scoped dependency). WDYT?
There was a problem hiding this comment.
Why not MANIFEST.MD? It's a part of JAR spec, and when we run the tests, our classpath will not contain such because it's not a JAR file. Sounds bulletproof for me :)
There was a problem hiding this comment.
I created a synthetic JAR so that we can be sure we're picking up JARs we're really interested in!
| private final String resolvedPath = resolvePath(); | ||
|
|
||
| @Getter(lazy = true) | ||
| private final String resolvedPathWithShellEscaping = resolvePathWithShellEscaping(); |
There was a problem hiding this comment.
FYI about lazy=true:
Since resolvePathWithShellEscaping is a one-liner, you can put it here without extracting a method. Of course, we can keep a separate method, just saying that it's not a must.
There was a problem hiding this comment.
Extremely good point! Actually I think resolvedPathWithShellEscaping can be removed altogether - will try.
| /** | ||
| * Logger to be used specifically for tracking even | ||
| */ | ||
| public class AuditLogger { |
There was a problem hiding this comment.
Bleurgh, didn't mean to commit this - will remove.
| final String results = toString.toUtf8String(); | ||
|
|
||
| // 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("ExternalResource.class")); |
There was a problem hiding this comment.
Probably another opportunity for the improvement described here
rnorth
force-pushed
the
285-recursive-tar-copy
branch
from
1b5f253
to
577f1cc
Compare
This encapsulates all the complexity of generating a path that the Docker daemon is about to create a volume mount for. This should resolve a general problem with spaces in paths, which was seen in one particular form as #263. Use recursive copy for docker context TAR to allow contents of directories to be used in Dockerfiles and Docker Compose contexts
rnorth
force-pushed
the
285-recursive-tar-copy
branch
from
158e429
to
6351b1f
Compare
To address #285