Add support for supplying build-args, fixes #22 #41
Conversation
There was a problem hiding this comment.
LGTM overall. Thanks for the contribution! I like the format for the pom configuration, and the way the test checks that the ARG is replaced is pretty smart 👍
Before merging though I'd be curious to hear what @dflemstr thinks of this functionality?
| @@ -93,7 +103,7 @@ public void execute(DockerClient dockerClient) | |||
| } | |||
|
|
|||
| final String imageId = buildImage( | |||
| dockerClient, log, verbose, contextDirectory, repository, tag, pullNewerImage, noCache); | |||
| dockerClient, log, verbose, contextDirectory, repository, tag, pullNewerImage, noCache, buildArgs); | |||
There was a problem hiding this comment.
buildArgs is now the seventh argument passed to this method that is an instance variable, perhaps it is time to make buildImage non-static so we don't have to have so many parameters passed to a method in the same class (which is never used by outside callers).
There was a problem hiding this comment.
Sounds like a good idea. If you want I could give it a go, unless you think it's outside the PR scope.
| @@ -145,6 +156,15 @@ static String buildImage(@Nonnull DockerClient dockerClient, | |||
| buildParameters.add(DockerClient.BuildParam.noCache()); | |||
| } | |||
|
|
|||
| if (buildArgs != null && !buildArgs.isEmpty()) { | |||
| try { | |||
| final String encodedBuildArgs = URLEncoder.encode(new Gson().toJson(buildArgs), "utf-8"); | |||
There was a problem hiding this comment.
I could be wrong but I think Jersey (used by docker-client) encodes the name/value of the BuildParam as query params before they are passed to the Docker Remote API. Are you sure the encoding is needed here too?
There was a problem hiding this comment.
I was surprised by this as well but without the encoding it complains about " in the request:
[INFO] [ERROR] Failed to execute goal com.spotify:dockerfile-maven-plugin:1.3.2-ooyala-build-args:build (default) on project basic-with-build-args: Execution default of goal com.spotify:dockerfile-maven-plugin:1.3.2-ooyala-build-args:build failed: Illegal character """ at position 21 is not allowed as a start of a name in a path template "pull=true&buildargs={"IMAGE_VERSION":"0.0.1"}". -> [Help 1]
| <dependencies> | ||
| <dependency> | ||
| <groupId>com.spotify</groupId> | ||
| <artifactId>docker-client</artifactId> |
There was a problem hiding this comment.
why is this needed as a dependency of maven-invoker-plugin?
There was a problem hiding this comment.
The invoker-plugin executes the verify.groovy scripts which don't seem to inherit the dependencies in the pom. I had to add it here to be able to use the DefaultDockerClient in the test.
There was a problem hiding this comment.
Ah that makes sense, thanks for the clarification
|
|
||
| ARG IMAGE_VERSION | ||
|
|
||
| LABEL version=${IMAGE_VERSION} |
There was a problem hiding this comment.
Minor: missing trailing newline
There was a problem hiding this comment.
I've amended the commit and added a newline
|
I released version 1.3.3 of the plugin a few minutes ago with this change in it. |
|
Awesome, thanks! 👍 |
This fixes issue #22. I took inspiration from the comments there and used the format below, let me know if you prefer another format.