Fix #859: ARG not support in FROM #1299

rohanKanojia · 2019-11-22T13:55:23Z

rhuss

Thanks a lot for the PR, but I think its not complete. Its not only that you can use args in a tag but as anything behind FROM (also the image name).

Not sure though how to actually tackle this.

rhuss · 2019-11-22T14:46:12Z

doc/changelog.md

@@ -8,6 +8,7 @@
  - Add docker:build support for 'network' option #1030
  - Failure referencing a previous staged image in FROM clause #1264
  - Treat bridged and default network mode the same (#1234)
+  - ARG not support in FROM (#859)


Suggested change

- ARG not support in FROM (#859)

- Support `ARG ` in `FROM` (#859)

rhuss · 2019-11-22T14:49:53Z

src/main/java/io/fabric8/maven/docker/util/DockerFileUtil.java

+        for (String[] argLine : argLines) {
+            if (argLine.length > 1) {
+                if (argLine[1].contains(":")) {
+                    result.put(argLine[1].split(":")[0], argLine[1].split(":")[1]);


You should only split once and reuse the result

You should split into exactly two parts, with the second parts potentially having :, too. I.e. use the split function which takes the max. nr of resulting split elements as argument.

rhuss · 2019-11-22T14:52:07Z

src/main/java/io/fabric8/maven/docker/util/DockerFileUtil.java

+                if (argLine[1].contains(":")) {
+                    result.put(argLine[1].split(":")[0], argLine[1].split(":")[1]);
+                } else if (argLine[1].contains("=")) {
+                    result.put(argLine[1].split("=")[0], argLine[1].split("=")[1]);


You can put the split on ':' and '=' into a function for reuse, e.g. addArgWitValue(result, "=", argLine[1])

rohanKanojia · 2019-11-22T17:23:55Z

I was trying to do the changes but I noticed one thing, I'm testing using this Dockerfile(to test FROM $IMAGENAME scenario):

FROM openjdk:jre

ARG version=0.31-SNAPSHOT
ARG jar_file=target/demp-sample-zero-config-$version.jar
ARG FULLIMAGENAME=busybox

ADD $jar_file /tmp/zero-config.jar
CMD java -cp /tmp/zero-config.jar HelloWorld
FROM $FULLIMAGENAME

But docker doesn't seem to be able to process this:

~/work/repos/docker-maven-plugin/samples/zero-config : $ docker build .
Sending build context to Docker daemon  45.06kB
Step 1/7 : FROM openjdk:jre
 ---> 6b23d41384f9
Step 2/7 : ARG version=0.31-SNAPSHOT
 ---> Using cache
 ---> de78be110994
Step 3/7 : ARG jar_file=target/demp-sample-zero-config-$version.jar
 ---> Using cache
 ---> a8268726a072
Step 4/7 : ARG FULLIMAGENAME=busybox
 ---> Using cache
 ---> f802a9b7666c
Step 5/7 : ADD $jar_file /tmp/zero-config.jar
 ---> Using cache
 ---> 15a65328a8aa
Step 6/7 : CMD java -cp /tmp/zero-config.jar HelloWorld
 ---> Using cache
 ---> dd5d7d159fea
Step 7/7 : FROM $FULLIMAGENAME
base name ($FULLIMAGENAME) should not be blank

Were you talking about this case? I'm not sure whether I've understood your comment correctly or not. I'm on 1.40 API version at the moment.

~/work/repos/docker-maven-plugin/samples/zero-config : $ docker version
Client: Docker Engine - Community
 Version:           19.03.4
 API version:       1.40
 Go version:        go1.12.10
 Git commit:        9013bf583a
 Built:             Fri Oct 18 15:52:46 2019
 OS/Arch:           linux/amd64
 Experimental:      false

Server: Docker Engine - Community
 Engine:
  Version:          19.03.4
  API version:      1.40 (minimum version 1.12)
  Go version:       go1.12.10
  Git commit:       9013bf583a
  Built:            Fri Oct 18 15:51:19 2019
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          1.2.10
  GitCommit:        b34a5c8af56e510852c35414db4c1f4fa6172339
 runc:
  Version:          1.0.0-rc8+dev
  GitCommit:        3e425f80a8c931f88e6d94a8c831b9d5aa481657
 docker-init:
  Version:          0.18.0

rohanKanojia · 2019-11-22T17:49:48Z

Sorry, it seems to be working in this case:

ARG FULLIMAGENAME=busybox:latest
FROM $FULLIMAGENAME

rhuss · 2019-12-19T15:01:54Z

src/main/java/io/fabric8/maven/docker/util/DockerFileUtil.java

+    private static AbstractMap.SimpleEntry<String, String> resolveArgsFromArgString(String argString) {
+        for (int index = 0; index < argString.length(); index++) {
+            if (argString.charAt(index) == ':' || argString.charAt(index) == '=') {
+                return new AbstractMap.SimpleEntry<>(argString.substring(0, index), argString.substring(index+1));


~~Could we add a trim() to allow spaces around : or = ?~~

well thats not possible as you send only the first argument of the line (and arguments are delmited by spaces).

I'm really not sure that simplistic parsing of values is correct. What happens with arg values that carry spaces ?

What if index is the lasst char in the string ? Wouldn't it be better to use String.split() with a regexp like :|= ?

rhuss

Thanks a lot for the PR but I'm afraid the parsing is not robust enough and only works for simple cases. That's just my feeling, but we need a better test coverage for the corner case and error cases, too.

rhuss · 2020-01-08T20:01:57Z

src/main/java/io/fabric8/maven/docker/util/DockerFileUtil.java

+    private static AbstractMap.SimpleEntry<String, String> resolveArgsFromArgString(String argString) {
+        for (int index = 0; index < argString.length(); index++) {
+            if (argString.charAt(index) == ':' || argString.charAt(index) == '=') {
+                return new AbstractMap.SimpleEntry<>(argString.substring(0, index), argString.substring(index+1));


What if index is the lasst char in the string ? Wouldn't it be better to use String.split() with a regexp like :|= ?

rhuss · 2020-01-08T20:06:21Z

src/main/java/io/fabric8/maven/docker/util/DockerFileUtil.java

+        Map<String, String> result = new HashMap<>();
+        for (String[] argLine : argLines) {
+            if (argLine.length > 1) {
+                AbstractMap.SimpleEntry<String, String> resolvedArg = resolveArgsFromArgString(argLine[1]);


I would call the method updateMapWithArgValue() and feed in the map as argument. That allows you to avoid to generate temporary, short-lived obkects like resolvedArg

rhuss · 2020-01-08T20:13:04Z

src/test/java/io/fabric8/maven/docker/util/DockerFileUtilTest.java

+        assertEquals("busybox:latest", fromClauses.next());
+        assertEquals(false, fromClauses.hasNext());
+    }
+


I would love to see more tests directly for the parsing methods with many corner cases. No need to create test Dockerfiles, but calling extractArgs() and resolveImageTagFromArgs() in a loop with different inputs (you could make the methods package private). But we need much more tests about different cases (like with ARG values, containing spaces, if this is allowed. Also check the Docker spec for the possible ARG variations).

rohanKanojia · 2020-01-20T18:07:46Z

@rhuss : Hi, Sorry I forgot to update this. I will update this around coming weekend

rhuss

Looks good to me with one minor suggestion.

But we should merge this sonnish ...

rhuss · 2020-06-04T09:56:16Z

src/main/java/io/fabric8/maven/docker/util/DockerFileUtil.java

+     * @param argLines list of lines containing arguments
+     * @return HashMap of arguments or empty collection if none is found
+     */
+    public static Map<String, String> extractArgs(List<String[]> argLines) {


Maybe you could inline the usage of extractLines ? (e..g change the signature to extractArgs(String dockerfile, FIxedStringSearchInterpolator interpolator) and call extract Lines internally)

rhuss · 2020-06-04T09:57:19Z

src/test/java/io/fabric8/maven/docker/util/DockerFileUtilTest.java

+    }
+
+    @Test
+    public void testExtractArgsFromDockerfile() {


dsmiley · 2020-08-12T20:59:31Z

This issue that this PR fixes is important; I hope the PR can be merged and released.

rhuss · 2020-08-29T10:27:06Z

Thanks, let's merge that now.

…r arguments in FROM port of fabric8io/docker-maven-plugin#1299

…r arguments in FROM Port of fabric8io/docker-maven-plugin#1299

…n FROM Port of fabric8io/docker-maven-plugin#1299

rohanKanojia requested a review from rhuss November 22, 2019 14:00

rhuss requested changes Nov 22, 2019

View reviewed changes

rohanKanojia force-pushed the pr/issue859 branch 2 times, most recently from fc6fde3 to d19be15 Compare November 22, 2019 18:01

rhuss reviewed Jan 8, 2020

View reviewed changes

rhuss requested changes Jan 8, 2020

View reviewed changes

rohanKanojia force-pushed the pr/issue859 branch from d063d8a to e84d0c8 Compare May 2, 2020 12:38

rohanKanojia mentioned this pull request May 4, 2020

Fix #174: Build failure when trying to use Dockerfile for arguments in FROM eclipse-jkube/jkube#175

Merged

rhuss requested changes Jun 4, 2020

View reviewed changes

Fix fabric8io#859: ARG not support in FROM

22f51d6

rohanKanojia force-pushed the pr/issue859 branch from 6bf635e to 22f51d6 Compare June 27, 2020 06:22

Merge branch 'master' into pr/issue859

731f0ac

rhuss merged commit 02822a7 into fabric8io:master Aug 29, 2020

rohanKanojia added a commit to rohanKanojia/jkube that referenced this pull request Sep 11, 2020

Fix eclipse-jkube#174: Build failure when trying to use Dockerfile fo…

724e8f2

…r arguments in FROM port of fabric8io/docker-maven-plugin#1299

ThatDockerUser mentioned this pull request Sep 14, 2020

ARG still not working properly in FROM #1372

Closed

rohanKanojia added a commit to rohanKanojia/jkube that referenced this pull request Oct 27, 2020

Fix eclipse-jkube#174: Build failure when trying to use Dockerfile fo…

1c94589

…r arguments in FROM Port of fabric8io/docker-maven-plugin#1299

rohanKanojia added a commit to rohanKanojia/jkube that referenced this pull request Oct 30, 2020

Fix eclipse-jkube#174: Build failure when trying to use Dockerfile fo…

98e9148

…r arguments in FROM Port of fabric8io/docker-maven-plugin#1299

rohanKanojia added a commit to rohanKanojia/jkube that referenced this pull request Oct 30, 2020

Fix eclipse-jkube#174: Build failure when trying to use Dockerfile fo…

7355a25

…r arguments in FROM Port of fabric8io/docker-maven-plugin#1299

rohanKanojia added a commit to rohanKanojia/jkube that referenced this pull request Oct 30, 2020

Fix eclipse-jkube#174: Build failure when trying to use Dockerfile fo…

50ec7f0

…r arguments in FROM Port of fabric8io/docker-maven-plugin#1299

rohanKanojia added a commit to rohanKanojia/jkube that referenced this pull request Oct 30, 2020

Fix eclipse-jkube#174: Build failure when trying to use Dockerfile fo…

2b693e6

…r arguments in FROM Port of fabric8io/docker-maven-plugin#1299

manusa pushed a commit to eclipse-jkube/jkube that referenced this pull request Oct 30, 2020

Fix #174: Build failure when trying to use Dockerfile for arguments i…

72188e1

…n FROM Port of fabric8io/docker-maven-plugin#1299

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #859: ARG not support in FROM #1299

Fix #859: ARG not support in FROM #1299

rohanKanojia commented Nov 22, 2019

rhuss left a comment

rhuss Nov 22, 2019

rhuss Nov 22, 2019

rhuss Nov 22, 2019

rohanKanojia commented Nov 22, 2019

rohanKanojia commented Nov 22, 2019

rhuss Dec 19, 2019 •

edited

Loading

rhuss Jan 8, 2020

rhuss left a comment

rhuss Jan 8, 2020

rhuss Jan 8, 2020

rhuss Jan 8, 2020

rohanKanojia commented Jan 20, 2020

rhuss left a comment

rhuss Jun 4, 2020

rhuss Jun 4, 2020

dsmiley commented Aug 12, 2020

rhuss commented Aug 29, 2020

	- ARG not support in FROM (#859)
	- Support `ARG ` in `FROM` (#859)

Fix #859: ARG not support in FROM #1299

Fix #859: ARG not support in FROM #1299

Conversation

rohanKanojia commented Nov 22, 2019

rhuss 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

rohanKanojia commented Nov 22, 2019

rohanKanojia commented Nov 22, 2019

rhuss Dec 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss 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

rohanKanojia commented Jan 20, 2020

rhuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsmiley commented Aug 12, 2020

rhuss commented Aug 29, 2020

rhuss Dec 19, 2019 •

edited

Loading