Add new withDockerfile methods to ImageFromDockerfile #1535

aguibert · 2019-06-06T17:45:51Z

These methods are preferable to the existing withDockerfilePath
methods because the docker-java API will honor .dockerignore files
if withDockerfile(File) is used.

aguibert · 2019-06-06T17:46:06Z

Fixes #1534

rnorth

Thanks for this - it's a good idea. I have a few suggestions; hopefully we can resolve these fairly easily.

rnorth · 2019-06-11T07:39:01Z

core/src/main/java/org/testcontainers/images/builder/ImageFromDockerfile.java

+     * It is recommended to use {@link #withDockerfile} instead because it honors .dockerignore
+     * files and therefore will be more efficient
+     * @param relativePathFromBuildRoot
+     */
    public ImageFromDockerfile withDockerfilePath(String relativePathFromBuildRoot) {


I wonder, would it be possible to make the existing withDockerfilePath take advantage of buildImageCmd::withDockerfile as well? I think it would be nice to change this PR so that:

Existing users of the withDockerfilePath method get the benefits of .dockerignore

We don't need to add (an effectively duplicate) method for withDockerfile(String pathname)

Good suggestion, I had thought of this too and experimented a bit initially. Unfortunately I don't think this is feasible because we'd have to take into account things like setting the docker build path. I'm not sure if I'm missing any more, but I'm not familiar enough with the docker-java API (yet) to confidently make this change with certainty it will not break existing users.

Shoud we deperecate the old method in this case?

That makes sense @aguibert - I can see how complexity could run away in trying to make it work.

I agree with @kiview re deprecating the existing method

core/src/main/java/org/testcontainers/images/builder/ImageFromDockerfile.java

rnorth · 2019-06-11T07:44:14Z

core/src/main/java/org/testcontainers/images/builder/ImageFromDockerfile.java

+            log.info("Transferred {} KB to Docker daemon", bytesToDockerDaemon % 1024);
+            if (bytesToDockerDaemon > 50 * 1024 * 1024) // warn if >50MB sent to docker daemon
+                log.warn("A large amount of data was sent to the Docker daemon ({}). Consider using a .dockerignore file for better performance.", 
+                        bytesToDockerDaemon % (1024 * 1024));


As above, shouldn't it be?

Suggested change

bytesToDockerDaemon % (1024 * 1024));

bytesToDockerDaemon / (1024 * 1024));

and add MB to the log message so that it's clear.

yes, good catch!

rnorth · 2019-06-11T07:44:52Z

core/src/main/java/org/testcontainers/images/builder/ImageFromDockerfile.java

                }
                tarArchive.finish();
            }
+
+            log.info("Transferred {} KB to Docker daemon", bytesToDockerDaemon % 1024);


Are you sure about this calculation?

Shouldn't it be:

Suggested change

log.info("Transferred {} KB to Docker daemon", bytesToDockerDaemon % 1024);

log.info("Transferred {} KB to Docker daemon", bytesToDockerDaemon / 1024);

Also, why not use a consistent unit (either KB or MB) for both log messages?

I wanted to use MB for the "woa you transferred a lot of data" message, because at that order of magnitude it will be more easily readable. However, for this log statement there will be a lot of cases where <1MB will be transferred and I didn't want to report "0 MB of data transferred" because that's confusing. Also didn't think it was worth branching the code to do "report in KB if <1MB otherwise report in MB"

We happen to have commons-io FileUtils on the classpath - we could actually delegate all this formatting to its byteCountToDisplaySize method, and not have to worry about which is the most appropriate unit for either message. WDYT?

kiview · 2019-06-12T12:54:20Z

core/src/test/java/org/testcontainers/images/builder/DockerignoreTest.java

+    }
+
+    @Test
+    public void testDockerignoreWithString() throws Exception {


Am I missing something, or is this the same test as testDockerignoreWithFile()?

core/src/test/java/org/testcontainers/images/builder/DockerignoreTest.java

kiview · 2019-06-12T12:58:47Z

core/src/main/java/org/testcontainers/images/builder/ImageFromDockerfile.java

                }
                tarArchive.finish();
            }
+
+            log.info("Transferred {} KB to Docker daemon", bytesToDockerDaemon % 1024);
+            if (bytesToDockerDaemon > 50 * 1024 * 1024) // warn if >50MB sent to docker daemon


What about adding constants for those magic numbers? 50_MB, MB_FACTOR for 1024 * 1024?

I see commons-io FileUtils has some constants so I'll use those instead

kiview · 2019-06-12T13:02:26Z

core/src/main/java/org/testcontainers/images/builder/ImageFromDockerfile.java

+     * It is recommended to use {@link #withDockerfile} instead because it honors .dockerignore
+     * files and therefore will be more efficient
+     * @param relativePathFromBuildRoot
+     */
    public ImageFromDockerfile withDockerfilePath(String relativePathFromBuildRoot) {


Shoud we deperecate the old method in this case?

bsideup · 2019-06-12T13:08:36Z

core/src/main/java/org/testcontainers/images/builder/ImageFromDockerfile.java

+        return this;
+    }
+
+    public ImageFromDockerfile withDockerfile(String pathname) {


let's not have this method, since it is unclear whether it will be using the classpath or FS

Good point. It could also be a dockerfile as a string!

I don't see why we couldn't remove this method.

I was thinking it may be convenient so people don't have to do withDockerfile(new FIle("Dockerfile")) in the normal case. Since it may cause confusion I'll remove it though

bsideup · 2019-06-12T13:09:07Z

core/src/main/java/org/testcontainers/images/builder/ImageFromDockerfile.java

@@ -63,6 +64,7 @@
    private final Map<String, Transferable> transferables = new HashMap<>();
    private final Map<String, String> buildArgs = new HashMap<>();
    private Optional<String> dockerFilePath = Optional.empty();
+    private Optional<File> dockerfile = Optional.empty();


can we have Path here please?

@bsideup do you mean hold an Optional<Path> object in the field and make the with-setter accept a Path, or just change the field type?

I have File here because the underlying docker-java API takes a File, so I thought it would be best to not do the unnecessary conversion. Is there a particular reason we should be using Path over File?

Path is a bit more forward thinking abstraction, easier to work with and I hope docker-java will adopt Path API soon too, so that we can avoid temporary files when it is possible.
I would prefer to have it in Testcontainers, because that's our public API, and to not block us from making changes to it later :)

aguibert · 2019-06-12T21:52:31Z

@rnorth @kiview @bsideup thanks for the reviews! I've updated my PR which I believe addresses all of the review comments thus far -- please have another look when you get some time

bsideup · 2019-06-21T10:13:40Z

core/src/test/java/org/testcontainers/images/builder/DockerignoreTest.java

+
+    @Test
+    public void testValidDockerignore() throws Exception {
+        try (final GenericContainer container = new GenericContainer(new ImageFromDockerfile()


nit1: please extract the image definition

nit2: could you please add a line break after try ( and also before ) {? I know that we're a bit inconsistent, but usually it helps, especially with long fluent-style chains

bsideup · 2019-06-21T10:14:14Z

core/src/test/resources/dockerfile-build-test/should_not_be_ignored.txt

@@ -0,0 +1 @@
+this file should be ignored by a .dockerignore file


filename says that it should not be ignored

core/src/main/java/org/testcontainers/images/builder/ImageFromDockerfile.java

bsideup · 2019-06-26T14:49:44Z

core/src/test/java/org/testcontainers/images/builder/DockerignoreTest.java

+                .withDockerfile(DockerfileBuildTest.RESOURCE_PATH.resolve("Dockerfile-currentdir"));
+        try (final GenericContainer<?> container = new GenericContainer<>(img.get())
+            .withStartupCheckStrategy(new OneShotStartupCheckStrategy())
+            .withCommand("ls", "/")) 


If would be nice to format it like here:

testcontainers-java/core/src/test/java/org/testcontainers/junit/CopyFileToContainerTest.java

Lines 15 to 19 in a74b77f

try(

GenericContainer container = new GenericContainer("alpine:latest")

.withCommand("sleep","3000")

.withCopyFileToContainer(MountableFile.forClasspathResource("/mappable-resource/"), containerPath)

) {

(maybe this a space between try and (

bsideup · 2019-06-26T14:49:50Z

core/src/test/java/org/testcontainers/images/builder/DockerignoreTest.java

+                .get();
+            fail("Should not be able to build an image with an invalid .dockerignore file");
+        } 
+        catch(DockerClientException e) {


Suggested change

catch(DockerClientException e) {

catch (DockerClientException e) {

I must say you are quite particular on code formatting @bsideup ;) Perhaps if you want to enforce this strict of formatting conventions, you could add a checkstyle plugin to the project?

bsideup · 2019-06-26T14:51:30Z

core/src/test/java/org/testcontainers/images/builder/DockerfileBuildTest.java

-            .withCommand("cat", "/test.txt")) {
-
+            .withCommand("cat", "/test.txt")) 
+        {


unrelated change?

yes, this is an unrelated change. Just cleaning up formatting to match what you had requested in another review comment since I was changing this file already

These methods are preferable to the existing withDockerfilePath methods because the docker-java API will honor .dockerignore files if withDockerfile(File) is used.

aguibert · 2019-07-16T13:46:20Z

rebased branch so it's up-to-date with master. Should be good to merge now

kiview

Thanks for the useful PR, just had one more question, we can address this later.

core/src/main/java/org/testcontainers/images/builder/ImageFromDockerfile.java

…Dockerfile.java Co-Authored-By: Kevin Wittek <kiview@users.noreply.github.com>

rnorth · 2019-07-17T19:42:37Z

That's 3 approvals, I'll merge. Thanks for another great contribution @aguibert, and sorry that it's taken a while to get this in.

aguibert requested review from bsideup, kiview and rnorth as code owners June 6, 2019 17:45

rnorth requested changes Jun 11, 2019

View reviewed changes

kiview requested changes Jun 12, 2019

View reviewed changes

bsideup reviewed Jun 12, 2019

View reviewed changes

aguibert force-pushed the ImageFromDockerfile-withDockerfile branch from dd002ce to 96cc9ae Compare June 12, 2019 21:51

bsideup reviewed Jun 21, 2019

View reviewed changes

aguibert force-pushed the ImageFromDockerfile-withDockerfile branch from 96cc9ae to a74b77f Compare June 21, 2019 19:51

bsideup reviewed Jun 26, 2019

View reviewed changes

core/src/main/java/org/testcontainers/images/builder/ImageFromDockerfile.java Show resolved Hide resolved

bsideup reviewed Jun 26, 2019

View reviewed changes

aguibert force-pushed the ImageFromDockerfile-withDockerfile branch from a74b77f to 7c87b38 Compare July 8, 2019 04:25

bsideup requested review from rnorth and kiview July 16, 2019 09:50

bsideup added this to the next milestone Jul 16, 2019

bsideup added the type/feature label Jul 16, 2019

bsideup approved these changes Jul 16, 2019

View reviewed changes

aguibert added 2 commits July 16, 2019 08:45

Add new withDockerfile methods to ImageFromDockerfile

63b444b

These methods are preferable to the existing withDockerfilePath methods because the docker-java API will honor .dockerignore files if withDockerfile(File) is used.

Review comments for .dockerignore support PR

aeb7557

aguibert force-pushed the ImageFromDockerfile-withDockerfile branch from ee8e534 to aeb7557 Compare July 16, 2019 13:45

bsideup added 2 commits July 16, 2019 21:16

Merge branch 'master' into ImageFromDockerfile-withDockerfile

8b43f67

Merge branch 'master' into ImageFromDockerfile-withDockerfile

e69ebe0

kiview approved these changes Jul 17, 2019

View reviewed changes

core/src/main/java/org/testcontainers/images/builder/ImageFromDockerfile.java Outdated Show resolved Hide resolved

aguibert and others added 2 commits July 17, 2019 14:18

Update core/src/main/java/org/testcontainers/images/builder/ImageFrom…

16252a5

…Dockerfile.java Co-Authored-By: Kevin Wittek <kiview@users.noreply.github.com>

Merge branch 'master' into ImageFromDockerfile-withDockerfile

15b75f4

rnorth approved these changes Jul 17, 2019

View reviewed changes

rnorth merged commit 7ca3f9f into testcontainers:master Jul 17, 2019

	bytesToDockerDaemon % (1024 * 1024));
	bytesToDockerDaemon / (1024 * 1024));

	log.info("Transferred {} KB to Docker daemon", bytesToDockerDaemon % 1024);
	log.info("Transferred {} KB to Docker daemon", bytesToDockerDaemon / 1024);

		@@ -0,0 +1 @@
		this file should be ignored by a .dockerignore file

	try(
	GenericContainer container = new GenericContainer("alpine:latest")
	.withCommand("sleep","3000")
	.withCopyFileToContainer(MountableFile.forClasspathResource("/mappable-resource/"), containerPath)
	) {

	catch(DockerClientException e) {
	catch (DockerClientException e) {

Add new withDockerfile methods to ImageFromDockerfile #1535

Add new withDockerfile methods to ImageFromDockerfile #1535

Conversation

aguibert commented Jun 6, 2019

aguibert commented Jun 6, 2019

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

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

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

aguibert commented Jun 12, 2019

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

aguibert commented Jul 16, 2019

kiview left a comment

Choose a reason for hiding this comment

rnorth commented Jul 17, 2019