Support multiple tags in DockerJavaApplicationPlugin #505
Conversation
|
@orzeh thanks for the feedback. I decided against the Plus, |
|
@aax |
|
By taking in this PR we'd essentially be breaking existing code if I'm not mistaken? Is there room for having both approaches for the moment with the existing being in a deprecated state until some future release to allow developers time to migrate? |
| @@ -104,7 +105,7 @@ class DockerJavaApplicationPlugin implements Plugin<Project> { | |||
| description = 'Builds the Docker image for the Java application.' | |||
| dependsOn createDockerfileTask | |||
| conventionMapping.inputDir = { createDockerfileTask.destFile.parentFile } | |||
| conventionMapping.tag = { determineImageTag(project, dockerJavaApplication) } | |||
| conventionMapping.tags = { [determineImageTag(project, dockerJavaApplication)] as Set } | |||
There was a problem hiding this comment.
Using the tags property of DockerBuildImage instead of (deprecated) property tag allows overriding the default config of javaApplication. In DockerBuildImage, tag has preference over tags, which made it effectless to set tags before.
I don't consider this change to be breaking, because the behavior is only changed if someone had the aforementioned effectless property tags set.
| @@ -122,7 +123,15 @@ class DockerJavaApplicationPlugin implements Plugin<Project> { | |||
| project.task(PUSH_IMAGE_TASK_NAME, type: DockerPushImage) { | |||
| description = 'Pushes created Docker image to the repository.' | |||
| dependsOn dockerBuildImageTask | |||
| conventionMapping.imageName = { dockerBuildImageTask.getTag() } | |||
| conventionMapping.imageName = { | |||
| if (dockerBuildImageTask.getTags().size() > 1) { | |||
There was a problem hiding this comment.
This check forces you to override settings for dockerPushImage iff you specified multiple tags in dockerBuildImage.
This is somewhat ugly, but it's just here because DockerPushImage doesn't support multiple tags to be pushed. Otherwise, the default behavior could be to push all built tags.
The change is not breaking because it behaves exactly the same in the default case of just 1 tag.
|
@orzeh I'm happy to provide a solution within javaApplication, but I'm unsure how to deal with the fact, that DockerPushImage does not support this. If you could sketch how you see this working, it would be a great help. Apart from that, I consider my approach as "handling it with low level tasks", because I let javaApplication untouched, but give the user the opportunity to go a level down and achieve the goal with the dockerBuildImage task. @cdancy I added some comments to the changes in DockerJavaApplicationPlugin.groovy explaining why I think the changes are not breaking. If I'm wrong, please drop me a hint. |
|
@aax fair enough. Feels like then we could simplify things if we added the necessary functionality to |
|
@aax sorry for my late response.
We have some misunderstanding here. What I was trying to say is that if you use Coming back to your PR, IMO it would be better to add |
|
@orzeh feels like we should just solve this problem within DockerPullImage and let multiple tags be used which in turn would make this PR much easier to code. |
|
@cdancy you mean |
|
@orzeh yeah ... I wonder if it would make more sense to use |
|
I suggest that we clean up all deprecated methods first. That will 1) force us to use the non-deprecated methods and 2) we'll have to add a I am going to close this PR for now. It has gotten quite outdated. Thanks for your work! @aax Would you like to help out with removing the deprecations? We can remove them one by one with individual pull requests if needed. |
Issue #461 asked for support for multiple tags in DockerJavaApplicationPlugin. This PR gives a way to achieve the same goal not by directly supporting multiple tags in the configuration of javaApplication. But it allows to override the tags configuration of the DockerBuildImage task, which was not possible before.