-
Notifications
You must be signed in to change notification settings - Fork 9
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
#199 fix environment variable substitution issue #201
Conversation
</build> | ||
</image> | ||
</images> | ||
</configuration> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I: clean up the duplicated execution configuration by moving the configuration out of plugin execution
</build> | ||
</image> | ||
</images> | ||
</configuration> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I: clean up the duplicated execution configuration by moving the configuration out of plugin execution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a thorough search on the project and cannot find any additional places the curly braces need to be removed. LGTM.
ARG JARS_DIR | ||
ADD ${JARS_DIR}/* ${SPARK_HOME}/jars/ | ||
ADD ${JARS_DIR}/* $SPARK_HOME/jars/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about ${JARS_DIR}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build argument can be replaced with ${}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this any different then the $JARS_DIR
in extensions/extensions-docker/aissemble-hive/aissemble-hive-service/src/main/resources/docker/Dockerfile
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah good catch.. I think that's been replaced by accident since the ARG can be replaced with the curl parenthesis. Both ways work for ARG type but I can replace it back for consistent but I think it's minor.
458b0f3
to
638d5ab
Compare
We use curly braces for some of the ENV substitutions in our Dockerfiles, but because fabric8 does Maven property expansion, and by default looks for ${} to do that substitution, we were getting injection of host machine values instead of properly using the VM’s env. This PR is to change the environment variable substitution from ${X} to $X