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

added build-args support #409

Merged
merged 3 commits into from
Apr 4, 2016

Conversation

balazsmaria
Copy link
Contributor

Signed-off-by: Balazs Nemeth balazs.nemeth@edudoo.com


branch [[support-build-args]]

Signed-off-by: Balazs Nemeth <balazs.nemeth@edudoo.com>

----------------------
branch [[support-build-args]]
@balazsmaria
Copy link
Contributor Author

Addresses this issue: #334

@elkdisk
Copy link

elkdisk commented Mar 16, 2016

Awesome! This will fix my problem that my build VM is behind a proxy and the container doesn't know of it at build time :D

@rhuss
Copy link
Collaborator

rhuss commented Mar 16, 2016

Thanks a lot ! 'will have a look ASAP and integrate it.


private Map<String, String> addBuildArgs() {
HashMap<String, String> args = new HashMap<>();
String[] argPairs = buildArgs.split(",");
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 that a bit too simplistic because of missing escape hanlding (think about values containing ,).

What do you think about the idea to allow properties of the form -Ddocker.build.arg.<variable_1>=<value_1> -Ddocker.build.arg.<variable_2>=<value_2> .... Thats how we model maps in the property handler anyway.

Of course then simple injection wouldn't work here but one would need to look it up actively from the project as properties when looking up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also wonder whether we need a distinction for individual images here since a build can contain many images (e.g. docker.build.<image|alias>.arg.<varname>

To make thinks shorter what about:

  • docker.arg.<varname> --> Used for all images
  • docker.iarg.<image>.<varname> --> Used for image <image>.

@rhuss
Copy link
Collaborator

rhuss commented Mar 22, 2016

Thanks again for the PR. Maybe you could have a look in the way how to provide build args from the outside (see above) ?

On a second thought, I don't see so much value for Docker ARGs when internally creating the Dockerfile from the build information (in contrast of using an external Dockerfile) since everything could be filled in with Maven properties, too. Also, the resulting Dockerfile is 'fixed' so that it can be better versioned (using the Dockerfile in multiple build will allways result in the same image because no external vars can be provided). Thats also important for 'docker:source' which is supposed to create the same image regardless how often used.

So maybe we can change the handling to use <build><arg>... only for filling in values for external Dockerfiles, not adding it to an internally created Dockerfile which doesnt make much sense because Maven properties can be used here anyway. Its then all about setting up the JSON query parameter when calling the docker daemon: Use the configuration specified here, overwritable via external properties.

It might make more sense when using an external Dockerfile (however we will get problems when using Maven property filtering because of the same variable syntax with ${ ... } but see #334 , too).

@rhuss rhuss added the feature label Mar 22, 2016
@balazsmaria
Copy link
Contributor Author

Thanks for having a look at it.
I think I understand the first comment, I can do that. I only needed to provide the proxy in my case, so that's why the simplistic approach for the start.

But I'd need some more clarification on the second one. How can you fill in the ARGs into the Dockerfile with Maven properties? I didn't really get this when I had looked at #334 for the first time and it's still unclear.

@rhuss
Copy link
Collaborator

rhuss commented Mar 22, 2016

@balazsmaria currently there are two canonical ways to build images:

Building up an own Dockerfile with information given in the in plugin configuration (thats what the DockerfileBuilder is for). Since you configure this completely in the pom.xml you can use Maven properties in the config values, too. E.g.

<build>
    <from>${baseImage}</from>
    <runCmds>
        <run>${proxyEnv} apt-get update</run>
    </runCmds>
    ....
</build>
mvn -DproxyEnv="HTTP_PROXY=http://myproxy:3231" docker:build

This properties can be configured the Maven way: In <properties>, with -D ... or in profiles. For this use case we don't need any Docker ARG support. The problem is then also, that the generated Dockerfile (which can be deployed and distributed with docker:source) is then not fixed (but depends on the args given when building), so the build is not fully specified with this Dockerfile + assembly.

Hence i think for this case we don't need support for adding ARGS instructions to the Dockerfile ...


Or there is the build mode with an external Dockerfile when the property dockerFileDir is set in the build's assembly configuration. In that case all Dockerfile features can be used, potentially also ARG. So for this case it would be good to allow to provide the args when doing the build (i.e. the JSON object sent to the Docker daemon as query parameter). This can be done as properties (as in your PR) or also as parat of the <build> configuration. E.g. the <args> you just introduced, so instead of using this for adding them to the Dockerfile in the use case above it could be used to provide the build time value to be filled in.

However one should be aware, if one used external Dockerfiles and ARG then one could have a problem when the standard Maven property filtering is used (which use the same variable syntax as ARG with ${...}). See also how we want to enhance this Dockerfile support in #205


Unfortunately the build configuration for both quite distinct use cases is interwoven, but I plan to detangle this to make it more clear what to use (i.e. to use complete separate config syntax format when using either of these configuration modes).

'hope that makes my thought a bit clearer ....

@balazsmaria
Copy link
Contributor Author

Yes, it's much clearer now. But:

"The problem is then also, that the generated Dockerfile (which can be deployed and distributed with docker:source) is then not fixed (but depends on the args given when building)". Why is that a problem? It depends on the args given when building, this is by design so. If an ARG has no default value, it has to be specified during building. This way you can have very different images only by changing an ARG at build time. I'm not sure I like this, but that's how it is. If you can do this with a vanilla Dockerfile, you should be able to do that with this plugin as well.

What am I missing here?

@rhuss
Copy link
Collaborator

rhuss commented Mar 22, 2016

@balazsmaria you are right, that with vanilla Docker the same Dockerfile can result in different images, but that doesn't necessarily mean thats a good thing when it comes to versioning ;-) (e.g. wonder how Docker Hub deals with build args)

With docker:source you use Maven coordinates including version number so that you can easily reference this from a Maven build, too (see #414). Ok, even when not using ARGs then there is no guarantee that the same Dockerfile leads to the same image (different docker engines use for building, 'RUN apt-get update' etc.), to its probably not that big issue here. Its fuzzy anyway.

However I'm still not fully convinced that we need ARGs for the internal Dockerfile use case:

  • It adds extra complexity albeit plain Maven properties could do everything ARGs can do.
  • More important: How to distinguish the ARG specification in the Dockerfile and the value to fill in for the build ? Not sure that always putting in values as defaults as in ARG var=default would work in any case (thinking of Maven profiles which only partially wants to override argument values. Don't know how good the Maven config merging / overwritting as described here will works in this case.

Not that I'm completely against it, just looking for some reasons ;)

@balazsmaria
Copy link
Contributor Author

I don't really have any reasons at the moment, it just seemed natural to include something that I could write to an external Dockerfile.
For the time being I only add the support for the build-args providing from the outside. Are you okay with that?

@rhuss
Copy link
Collaborator

rhuss commented Mar 22, 2016

@balazsmaria yes, perfectly fine for me.

Balazs Nemeth added 2 commits March 22, 2016 13:32
…outside

----------------------
branch [[support-build-args]]
----------------------
branch [[support-build-args]]
@balazsmaria
Copy link
Contributor Author

Have you had a chance to look at the latest commits?

@rhuss
Copy link
Collaborator

rhuss commented Apr 1, 2016

will do it today

@rhuss
Copy link
Collaborator

rhuss commented Apr 2, 2016

I looked at it briefly, and will comment on it with some suggestions soon (maybe over the weekend)

@rhuss rhuss merged commit 6951923 into fabric8io:integration Apr 4, 2016
rhuss added a commit that referenced this pull request Apr 4, 2016
This is based on the work done in #409 (thanks @	balazsmaria !) and allows the usage of ARG in external Dockerfiles.
Documentation and external property config hanlder has been updated, too.
@rhuss
Copy link
Collaborator

rhuss commented Apr 4, 2016

Thanks a lot again ! I made things a bit simpler by moving the args configuration to the plugin configuration (in order to avoid support for external properties in the main configuration section.

You can always do a

<image>
   <build>
     <args>
        <HTTP_PROXY>${myHttpProxy}</HTTP_PROXY>
     <args>
   </build>
</imag>

and then run Maven with

 mvn -DmyHttpProxy=http://proxy:8001 .....

Or alternatively you could also use the external property config handler.

'hope this still fits your use case.

@balazsmaria
Copy link
Contributor Author

Thanks for the merge!

So previously we said this should be possible:

mvn package docker:build -Ddocker.arg.http_proxy=http://myproxy:1234

But now how do I do that without modifying the plugin configuration?

The problem is that these are predefined ARG variables:

  • HTTP_PROXY
  • http_proxy
  • HTTPS_PROXY
  • https_proxy
  • FTP_PROXY
  • ftp_proxy
  • NO_PROXY
  • no_proxy

so you don't want to re-define them.

@rhuss
Copy link
Collaborator

rhuss commented Apr 4, 2016

This could be done as described above, however with an update to the configuration (which make explicit which ARGs are used). Maybe that was a misunderstanding on my side. We intentionally don't want to expose image specific configuration via properties to the outside because as experience told us this leads easily to nasty hidden dependencies (e.g. the d-m-p and the fabric8-maven-plugin are coupled with the docker.image property). So when there is a solution without properties which has intrinsic semantic meaning to the plugin I tend to favour this.

Said that, there is a way to configure the build via properties alone. You can use an external property configuration handler as described in http://fabric8io.github.io/docker-maven-plugin/external-configuration.html which probably comes close to the usage scenario you describe above (but you have to decide which configuration style you want to chose, my recommendation is the standard one :)

I wonder whether you could live this extra step of mentioning the args in the plugin configuration. If not, I would consider to introduce a global configuration option docker.buildArg.MY_ARG which is not image specific but adds to the build-args for all images. This could be a good compromise.

wdyt ?

@balazsmaria
Copy link
Contributor Author

The problem is that I don't want to change anything at all in order to use a proxy. The proxy has nothing to do with the business goal, and in my case it has to be there in one office location and in others not, especially not in a cloud environment. So it's not OK to change the pom just because one location uses the proxy. (The bad thing about it is that currently I have to put the proxy there manually and remove it before commit. Very fishy!)

I think this is a very stupid but valid scenario.

@rhuss
Copy link
Collaborator

rhuss commented Apr 4, 2016

But you need to craft the Dockerfile accordingly for using the ARGs, aren't you ?

With the setup i described above, you also don't have to change the pom when you are changing the environment:

<project>
...
<properties>
   <!-- Empty by default -->
   <my.http.proxy></my.http.proxy>
</properties>
....
....
<plugin><configuration><images><image><build>
  <args>
    <HTTP_PROXY_ENV>${my.http.proxy}</HTTP_PROXY_ENV>
  </args>
....

with a Dockerfile

FROM ...
ARG HTTP_PROXY_ENV
RUN ${HTTP_PROXY_ENV} apt-get ....

and then run either mvn clean install (without a proxy environment) or mvn -Dmy.http.proxy="http_proxy=http://company-proxy:8001" for setting the proxy.

So you can use the same pom.xml in all environments (if this is your requirement). + you can infer from reading the pom.xml what to set for enabling the proxy (its explicit, that's my point).

@balazsmaria
Copy link
Contributor Author

No, you don't need to alter your Dockerfile, this is all implicit. That's the point of the predefined ARG values. So the simple

FROM ...
RUN apt-get ....

works if you just do
docker build --build-arg HTTP_PROXY=http://10.20.30.2:1234 .

@rhuss
Copy link
Collaborator

rhuss commented Apr 4, 2016

Sorry, then I misunderstood the Docker documentation which states that a certain set of predefined args need not to be declared with ARG but you still need to use them in the Dockerfile.

Or do they automatically end up as environment variables (thats new to me, but tbh I never used this feature) ?

@balazsmaria
Copy link
Contributor Author

Yes, this is how they work. During build they act as if they were environmental variables.

@rhuss
Copy link
Collaborator

rhuss commented Apr 4, 2016

Then it makes definitively sense to provide suche props from the outside. I wonder though whether it would make sense to add a dedicated, global proxy option like '-Ddocker.buildProxy(and maybe-Ddocker.noProxy`) which would set all environment variable in all variations (similar to what is described in moby/moby#14634 (comment)).

@balazsmaria
Copy link
Contributor Author

I'd prefer our original approach with docker.buildArg.myVar, so -Ddocker.buildArg.http_proxy=http://proxy, which is close to the vanilla Docker way. I don't know if it is necessary to provide support explicitly for these vars, I'd let the users decide what and how to set and not give more than docker gives.

@rhuss
Copy link
Collaborator

rhuss commented Apr 4, 2016

Agreed. we shouldn't be too clever (always in danger of premature featuritis ;).

I will then add the support as described above, e.g. a global configuration option for setting build args for all configured images at once.

Thanks for the discussion and the heads wrt to predefined args (tbh didn't really know that). I'm currently on another PR but will add this afterwards.

@balazsmaria
Copy link
Contributor Author

Could you please let me know how is that different from what I implemented? In case it's similar and you are not happy with something, I could fix that.

@rhuss
Copy link
Collaborator

rhuss commented Apr 4, 2016

The difference is that its not specific to an image (name or alias), but globally applicable for every image. not a biggie, if you like you can pick that up from branch integration.

@balazsmaria
Copy link
Contributor Author

Does this go to BuildService or AbstractBuildSupportMojo? Also, do global build-args come from project.properties as well, or only system.properties? In the first case AbstractBuildSupportMojo is the choice I guess.

@rhuss
Copy link
Collaborator

rhuss commented Apr 5, 2016

It should go into AbstractBuildSupportMojo since the BuildService is agnostic to a MavenProject and its properties. Also, I would lookup in Project properties (<properties>...</properties>) and in System.getProperties() (-D...) (but could be that system properties are already included in MavenProject properties)

balazsmaria pushed a commit to balazsmaria/docker-maven-plugin that referenced this pull request Apr 5, 2016
…rom project and system properties

----------------------
branch [[build-args-from-outside]]
@balazsmaria
Copy link
Contributor Author

I checked and System.properties are not in Project properties, so I added both.

rhuss added a commit that referenced this pull request Apr 5, 2016
#409 - docker.buildArg.myBuildArg to docker build-arg both from proje…
rhuss added a commit that referenced this pull request Apr 5, 2016
Also related to #409, only minor updates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants