DockerCommitImage task should not fail when accessing container ID property value #718

rroques · 2018-12-13T03:20:21Z

Fixes Issue#716

rroques · 2018-12-13T03:24:50Z

Sorry, I missed my changes in the first commit. Happy to rejig the commits if the changes are accepted.

orzeh

LGTM 👍

bmuschko · 2018-12-14T04:36:40Z

...nctTest/groovy/com/bmuschko/gradle/docker/tasks/image/DockerCommitImageFunctionalTest.groovy

+                targetContainerId createContainer.getContainerId()
+            }
+
+            ${containerCommitImageExecutionTask}


This test case is missing a task that stops the container. If we don't stop the container then we'll end up with stranded containers whenever the functional test is run. Could you please add it as a finalizedBy task? You will find some examples in other test classes.

Thanks for the feedback. I added image and container clean up in the test. Let me know how it looks.

cdancy · 2018-12-14T14:48:35Z

Didn't even see there was a PR out for this and I put one together :) Lets go with this one as it provides a more complete solution.

cdancy · 2018-12-14T14:50:09Z

src/main/groovy/com/bmuschko/gradle/docker/tasks/image/DockerCommitImage.groovy

-        logger.quiet "Commiting image for container '${getContainerId()}'."
-        def commitCmd = dockerClient.commitCmd(getContainerId())
+        logger.quiet "Commiting image for container '${getContainerId().get()}'."
+        def commitCmd = dockerClient.commitCmd(getContainerId().get())


Can we go with what I did for #720 here? I think it would be a good idea to start a new standard in that we need to check that the containerId was set and fail accordingly.

The get() method will actually throw an exception if no value was set.

getOrNull()?

I only say to use that as it may not be exactly obvious when buried within the gradle stacktrace at least in this case we can control the message.

I updated the code with the implementation from #720

@cdancy The method get() will throw an exception if no value was set. The method getOrNull() will not throw an exception but will return null if no value is net. I'd try to create a test and see how the error message looks like. Otherwise, we are likely just duplicating code.

I added a couple of functional tests for the cases where the container is missing, or is non existing.

In the case of the missing container, the test does not show case the getOrNull() behaviour because the task validation kicks in first, and since DockerExistingContainer#containerId input is not optional, the validation fails the build with error No value has been specified for property 'containerId'.

In the case of the non existing container, then we get the error message from the docker daemon.

…nd safer retrieval of containerId in DockerCommitImage

bmuschko · 2018-12-16T18:55:46Z

...nctTest/groovy/com/bmuschko/gradle/docker/tasks/image/DockerCommitImageFunctionalTest.groovy

+
+            ${containerCommitImageExecutionTask}
+
+            task removeImage(type: DockerRemoveImage) {


You won't need to remove the image as it is being reused for other functional tests. We'll only want to download that once.

Sorry I meant removing the image that has just been committed, myimage:latest. Happy to remove and only stop the container if you think that is not necessary.

Oh, I see. Yeah, let's remove the committed image. That makes sense.

bmuschko · 2018-12-16T18:56:38Z

...nctTest/groovy/com/bmuschko/gradle/docker/tasks/image/DockerCommitImageFunctionalTest.groovy

+        given:
+        String commitImage = """
+            task commitImage(type: DockerCommitImage) {
+                dependsOn startContainer


This should say finalizedBy removeContainer.

bmuschko · 2018-12-16T22:19:21Z

...nctTest/groovy/com/bmuschko/gradle/docker/tasks/image/DockerCommitImageFunctionalTest.groovy

+        result.output.contains("Commiting image for container")
+    }
+
+    def "cannot commit image without container"() {


I'd say this test case is not necessary as we are basically testing that the Gradle Provider API works as expected. You can remove this test case.

The intention was to verify that the DockerExistingContainer#containerId is properly annotated, and that the Gradle input validation kicks in.

Happy to remove though.

bmuschko · 2018-12-16T22:20:04Z

...nctTest/groovy/com/bmuschko/gradle/docker/tasks/image/DockerCommitImageFunctionalTest.groovy

+        buildFile << containerStart(commitImage)
+
+        when:
+        BuildResult result = build('commitImage')


As a side note: You'd use buildAndFail in the case that you expect the build to run and fail. In those cases you won't need to catch an exception.

Great! I did not know about that method.

bmuschko · 2018-12-16T22:20:23Z

...nctTest/groovy/com/bmuschko/gradle/docker/tasks/image/DockerCommitImageFunctionalTest.groovy

+        buildFile << containerStart(commitImage)
+
+        when:
+        BuildResult result = build('commitImage')


Same here: This should be buildAndFail.

bmuschko · 2018-12-16T22:21:41Z

src/main/groovy/com/bmuschko/gradle/docker/tasks/image/DockerCommitImage.groovy

@@ -67,8 +67,10 @@ class DockerCommitImage extends DockerExistingContainer {

    @Override
    void runRemoteCommand(dockerClient) {
-        logger.quiet "Commiting image for container '${getContainerId()}'."
-        def commitCmd = dockerClient.commitCmd(getContainerId())
+        def localId = Objects.requireNonNull(getContainerId().getOrNull(), 'containerId must be set')


IMHO I don't think this handling is necessary. If we expect the container ID property to always container a value then we can just let Gradle fail the build with its own error message.

bmuschko · 2018-12-16T23:19:06Z

Thanks for the clarification. I am going to merge and clean it up in a couple of minutes so we can get you the fix.

rroques · 2018-12-16T23:23:19Z

No problem, I have committed the last changes I had prepared following your comments. Feel free to take over the PR and make all the changes you see fit.

Thanks for the help! I learned a lot playing with the function tests using testkit, they are really great!

bmuschko · 2018-12-16T23:37:23Z

@rroques Thanks again for your work! We appreciated it.

Remi Roques added 2 commits December 13, 2018 16:07

Issue#716 Fix DockerCommitImage task and functional test

fa03057

Issue#716 Fix DockerCommitImage task and functional test

c858d93

orzeh approved these changes Dec 13, 2018

View reviewed changes

bmuschko requested changes Dec 14, 2018

View reviewed changes

cdancy assigned bmuschko and cdancy Dec 14, 2018

cdancy added bug enhancement labels Dec 14, 2018

cdancy reviewed Dec 14, 2018

View reviewed changes

cdancy mentioned this pull request Dec 14, 2018

Fix for ISSUE-716 #720

Closed

Add image and container cleanup to DockerCommitImageFunctionalTest, a…

bb37b4a

…nd safer retrieval of containerId in DockerCommitImage

bmuschko requested changes Dec 16, 2018

View reviewed changes

Functional test for Docker commit image when container is invalid

3469466

bmuschko added this to the v4.2.0 milestone Dec 16, 2018

bmuschko requested changes Dec 16, 2018

View reviewed changes

bmuschko merged commit 3469466 into bmuschko:master Dec 16, 2018

bmuschko changed the title ~~Issue#716 Fix DockerCommitImage task and functional test~~ DockerCommitImage task should not fail when accessing container ID property value Dec 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DockerCommitImage task should not fail when accessing container ID property value #718

DockerCommitImage task should not fail when accessing container ID property value #718

rroques commented Dec 13, 2018

rroques commented Dec 13, 2018

orzeh left a comment

bmuschko Dec 14, 2018

rroques Dec 14, 2018

cdancy commented Dec 14, 2018

cdancy Dec 14, 2018

bmuschko Dec 14, 2018

cdancy Dec 14, 2018

cdancy Dec 14, 2018

rroques Dec 14, 2018

bmuschko Dec 15, 2018

rroques Dec 16, 2018

bmuschko Dec 16, 2018

rroques Dec 16, 2018

bmuschko Dec 16, 2018

bmuschko Dec 16, 2018

bmuschko Dec 16, 2018

rroques Dec 16, 2018

bmuschko Dec 16, 2018

rroques Dec 16, 2018

bmuschko Dec 16, 2018

bmuschko Dec 16, 2018

bmuschko commented Dec 16, 2018

rroques commented Dec 16, 2018

bmuschko commented Dec 16, 2018


		${containerCommitImageExecutionTask}

		task removeImage(type: DockerRemoveImage) {

DockerCommitImage task should not fail when accessing container ID property value #718

DockerCommitImage task should not fail when accessing container ID property value #718

Conversation

rroques commented Dec 13, 2018

rroques commented Dec 13, 2018

orzeh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdancy commented Dec 14, 2018

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

bmuschko commented Dec 16, 2018

rroques commented Dec 16, 2018

bmuschko commented Dec 16, 2018