Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1372: Plugin now resolves ARGs provided in BuildImageConfiguration #1373

Merged
merged 2 commits into from
Sep 27, 2020

Conversation

rohanKanojia
Copy link
Member

Fix #1372

Refactored DockerFileUtil.extractBaseImages to accept a HashMap of
Build args which would be passed from BuildService in order to resolve
ARG values which are provided from plugin configuration

…nfiguration

Refactored `DockerFileUtil.extractBaseImages` to accept a HashMap of
Build args which would be passed from BuildService in order to resolve
ARG values which are provided from plugin configuration
Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Thanks @rohanKanojia ! I will have a look now, and let's get that fix in ASAP.

I'm planning a release this weekend to fix some of the more pressing issues.

static String resolveArgValueFromStrContainingArgKey(String argString, Map<String, String> args) {
if (argString.startsWith("$") && args.containsKey(argString.substring(1))) {
return args.get(argString.substring(1));
} else if (argString.startsWith("${") && argString.endsWith("}") && args.containsKey(argString.substring(2, argString.length() - 1))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think spaces are also allowed here like in ${ foobar }. So for extracting the variable name it might be to user a regular expression. you then could also collapse the if statements into a single regular expression.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I.e I think a REs like

^\$([^{}]+)$

^\$\{\s*([^{}\s]+)\s*\}$

would do the trick (not verified though)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm trying out an arg with spaces like this in this Dockerfile:

ARG busyboxVersion
FROM busybox:${ busyboxVersion }

When I trying to build it with docker, I'm getting an error:

docker : $ docker build -t test-dockerfile --build-arg busyboxVersion=latest .
Sending build context to Docker daemon 3.072 kB
Error response from daemon: Dockerfile parse error line 2: FROM requires either one or three arguments

@rhuss
Copy link
Collaborator

rhuss commented Sep 27, 2020

@rohanKanojia let's get it merged now to fix the issue at hand, but would love to allow also variables provided in an extended way (as mentioned in the pr comments)

@rhuss rhuss merged commit dae2839 into fabric8io:master Sep 27, 2020
rohanKanojia added a commit to rohanKanojia/docker-maven-plugin that referenced this pull request Sep 28, 2020
Use regex in order to resolve ARG value
rohanKanojia added a commit to rohanKanojia/docker-maven-plugin that referenced this pull request Sep 29, 2020
Use regex in order to resolve ARG value
rohanKanojia added a commit to rohanKanojia/docker-maven-plugin that referenced this pull request Oct 2, 2020
Use regex in order to resolve ARG value
rhuss pushed a commit that referenced this pull request Oct 10, 2020
Use regex in order to resolve ARG value
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ARG still not working properly in FROM
2 participants