enable copyFileToContainer feature during container startup #742

dharanikesav · 2018-06-11T09:03:17Z

Currently, testcontainer is capable of copying files only after starting a container. But, we have a requirement to copy files to container after creation and then start the container. This code change is to enable copy files to container immediately after creation.

kiview

Can you please add some test cases as well?

kiview · 2018-06-12T12:59:27Z

core/src/main/java/org/testcontainers/containers/GenericContainer.java

-        if (!isRunning()) {
-            throw new IllegalStateException("copyFileToContainer can only be used while the Container is running");
+        if (!isRunning() && !isCreated()) {
+            throw new IllegalStateException("opyFileToContainer can only be used with created / running container");


Typo in exception message

kiview · 2018-06-12T13:00:10Z

core/src/main/java/org/testcontainers/containers/Container.java

+     * @param mountableFile a Mountable file with path of source file / folder on host machine
+     * @param containerPath a destination path on conatiner to which the files / folders to be copied
+     */
+    void addCopyFileToContainer(MountableFile mountableFile, String containerPath);


We've decided to deprecate the non-fluent setters, so you can omit this method.

kiview · 2018-06-12T13:00:27Z

core/src/main/java/org/testcontainers/containers/GenericContainer.java

@@ -835,6 +838,21 @@ public SELF withWorkingDirectory(String workDir) {
        return self();
    }

+
+    @Override
+    public void addCopyFileToContainer(MountableFile mountableFile, String containerPath) {


See above, you can remove this method.

bsideup · 2018-06-13T20:19:59Z

core/out/production/resources/META-INF/services/README.md

@@ -0,0 +1 @@
+The contents of this folder is a workaround for issues described here: https://github.com/KostyaSha/docker-java-shade/issues/1


this file (as well as other /out files) should not be commited

rnorth

A few minor comments from me, but this looks like a good change to be making - thank you very much!

rnorth · 2018-06-16T19:42:38Z

core/src/main/java/org/testcontainers/containers/ContainerState.java

+
+        try {
+            Boolean created = getCurrentContainerInfo().getState().getStatus().equalsIgnoreCase("Created");
+            return Boolean.TRUE.equals(created);


In the isRunning method the result of getRunning() is a nullable Boolean, which is why we do the TRUE.equals call in that method.

Here, equalsIgnoreCase just returns a primitive boolean, and further, getStatus() may return null.

As such, I think it would actually be safer to just replace these two lines with:
return "created".equalsIgnoreCase(getCurrentContainerInfo().getState().getStatus());

Please could we do that instead?

Also, this method will return false if the container is not just created but also started

rnorth · 2018-06-16T19:43:10Z

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

+
+            Assert.assertTrue(filesList.contains(fileName));
+        } catch (Exception e) {
+            e.printStackTrace();


Please let the exception be thrown from the test method, as it could be important.

rnorth · 2018-06-16T19:44:22Z

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

+
+    @Before
+    public void before() {
+        container = new GenericContainer("couchbase:latest")


I think we can get away with using a smaller image here (and save some startup time). Would an alpine image be OK?

we need container to be up and running to perform this test. Alpine container is exiting immediately after start. Because of this, i am unable to use alpine container

Use the alpine image and provide a long running command (like sleep), using withCommand() on the container.

rnorth · 2018-06-16T19:46:06Z

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

+
+    @Test
+    public void checkFileCopied() {
+        container.start();


Do we need to split the instantiation/start of the container like this, or could the container just be a field with an @Rule annotation for this test? I think I'd prefer the latter, as it's the more idiomatic way.

actually, didn't we agree to use try-with-resources style? Otherwise, if you have 5 rules in your test and run a single test all of them will start - that sucks :(

Maybe it depends? This class has only one test method (and would have only one Rule), which is why I suggested this way. I'm very relaxed about it though; try-with-resources would also be fine.

What is this try-with-resources? could you please provide a reference to understand this

Sure, see here:

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

Lines 346 to 363 in 609f9a1

public void createContainerCmdHookTest() {

// Use random name to avoid the conflicts between the tests

String randomName = Base58.randomString(5);

try(

GenericContainer container = new GenericContainer<>("redis:3.0.2")

.withCommand("redis-server", "--help")

.withCreateContainerCmdModifier(cmd -> cmd.withName("overrideMe"))

// Preserves the order

.withCreateContainerCmdModifier(cmd -> cmd.withName(randomName))

// Allows to override pre-configured values by GenericContainer

.withCreateContainerCmdModifier(cmd -> cmd.withCmd("redis-server", "--port", "6379"))

) {

container.start();

assertEquals("Name is configured", "/" + randomName, container.getContainerInfo().getName());

assertEquals("Command is configured", "[redis-server, --port, 6379]", Arrays.toString(container.getContainerInfo().getConfig().getCmd()));

}

}

@bsideup Thank you.. that helps

dharanikesav · 2018-06-20T05:13:08Z

@kiview @bsideup I have done the changes requested. Is it ok to merge this branch?

bsideup · 2018-06-20T05:53:59Z

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

+        try(
+            GenericContainer container = new GenericContainer("alpine:latest")
+                .withCommand("sleep","3000")
+                .withCopyFileToContainer(MountableFile.forHostPath(folderPath), containerPath)


please use MountableFile.forClasspathResource("/mappable-resource/test-resource.txt")

dharanikesav · 2018-06-20T07:48:26Z

@rnorth @bsideup @kiview Incorporated all the changes requested.

kiview · 2018-06-20T08:10:16Z

core/src/test/resources/logback-test.xml

This is of course an unnecessary change, but rest LGTM.

kiview · 2018-06-22T13:37:36Z

@rnorth Have your remarks been addressed?

dharanikesav · 2018-06-26T05:23:20Z

@rnorth Please verify whether the changes are done as requested by you and share your view in merging this branch

bsideup · 2018-07-10T08:09:47Z

@dharanikesav sorry for taking so long for the review! Richard is a bit busy, so I took an action to merge it :)

Thanks for your contribution! A really good one! 👍

dharanikesav requested review from bsideup, kiview and rnorth as code owners June 11, 2018 09:03

dharanikesav force-pushed the enable-copy-files-to-container-during-startup branch 2 times, most recently from 259e12d to b82cbab Compare June 11, 2018 09:35

kiview requested changes Jun 12, 2018

View reviewed changes

dharanikesav force-pushed the enable-copy-files-to-container-during-startup branch 6 times, most recently from a12aeae to 5ea4c94 Compare June 13, 2018 12:00

bsideup reviewed Jun 13, 2018

View reviewed changes

dharanikesav force-pushed the enable-copy-files-to-container-during-startup branch from 5ea4c94 to 0b78a2e Compare June 14, 2018 10:27

rnorth requested changes Jun 16, 2018

View reviewed changes

dharanikesav force-pushed the enable-copy-files-to-container-during-startup branch from 0b78a2e to 871f47c Compare June 18, 2018 08:17

bsideup reviewed Jun 20, 2018

View reviewed changes

enable copyFileToContainer feature during container startup

15ebab0

dharanikesav force-pushed the enable-copy-files-to-container-during-startup branch from 871f47c to 15ebab0 Compare June 20, 2018 07:34

kiview approved these changes Jun 20, 2018

View reviewed changes

core/src/test/resources/logback-test.xml

</configuration>

Copy link

Member

kiview Jun 20, 2018

Choose a reason for hiding this comment

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

This is of course an unnecessary change, but rest LGTM.

bsideup approved these changes Jun 20, 2018

View reviewed changes

bsideup merged commit 22cab1f into testcontainers:master Jul 10, 2018

bsideup added this to the next milestone Jul 10, 2018

bsideup added the type/feature label Jul 10, 2018

dharanikesav deleted the enable-copy-files-to-container-during-startup branch July 18, 2018 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable copyFileToContainer feature during container startup #742

enable copyFileToContainer feature during container startup #742

dharanikesav commented Jun 11, 2018

kiview left a comment

kiview Jun 12, 2018

kiview Jun 12, 2018

kiview Jun 12, 2018

bsideup Jun 13, 2018

rnorth left a comment

rnorth Jun 16, 2018

bsideup Jun 16, 2018

rnorth Jun 16, 2018

rnorth Jun 16, 2018

dharanikesav Jun 18, 2018

kiview Jun 18, 2018

rnorth Jun 16, 2018

bsideup Jun 16, 2018

rnorth Jun 16, 2018

dharanikesav Jun 18, 2018 •

edited

Loading

bsideup Jun 18, 2018 •

edited

Loading

dharanikesav Jun 18, 2018

dharanikesav commented Jun 20, 2018

bsideup Jun 20, 2018

dharanikesav commented Jun 20, 2018

kiview Jun 20, 2018

kiview commented Jun 22, 2018

dharanikesav commented Jun 26, 2018

bsideup commented Jul 10, 2018

		@@ -0,0 +1 @@
		The contents of this folder is a workaround for issues described here: https://github.com/KostyaSha/docker-java-shade/issues/1

	public void createContainerCmdHookTest() {
	// Use random name to avoid the conflicts between the tests
	String randomName = Base58.randomString(5);
	try(
	GenericContainer container = new GenericContainer<>("redis:3.0.2")
	.withCommand("redis-server", "--help")
	.withCreateContainerCmdModifier(cmd -> cmd.withName("overrideMe"))
	// Preserves the order
	.withCreateContainerCmdModifier(cmd -> cmd.withName(randomName))
	// Allows to override pre-configured values by GenericContainer
	.withCreateContainerCmdModifier(cmd -> cmd.withCmd("redis-server", "--port", "6379"))
	) {
	container.start();

	assertEquals("Name is configured", "/" + randomName, container.getContainerInfo().getName());
	assertEquals("Command is configured", "[redis-server, --port, 6379]", Arrays.toString(container.getContainerInfo().getConfig().getCmd()));
	}
	}

enable copyFileToContainer feature during container startup #742

enable copyFileToContainer feature during container startup #742

Conversation

dharanikesav commented Jun 11, 2018

kiview 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

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

dharanikesav Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

bsideup Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dharanikesav commented Jun 20, 2018

Choose a reason for hiding this comment

dharanikesav commented Jun 20, 2018

Choose a reason for hiding this comment

kiview commented Jun 22, 2018

dharanikesav commented Jun 26, 2018

bsideup commented Jul 10, 2018

dharanikesav Jun 18, 2018 •

edited

Loading

bsideup Jun 18, 2018 •

edited

Loading