Partially related: #386

Codecov Report

Merging #938 into master will increase coverage by 0.42%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##             master     #938      +/-   ##
============================================
+ Coverage     51.57%   51.99%   +0.42%     
- Complexity     1232     1270      +38     
============================================
  Files           144      144              
  Lines          7250     7301      +51     
  Branches        981      991      +10     
============================================
+ Hits           3739     3796      +57     
+ Misses         3195     3183      -12     
- Partials        316      322       +6

@stromnet thanks for the PR ! I see clearly your use case and think that is useful.

However, I would strive for another approach: We already have the possibility to make external configuration via properties by adding to an image configuration:

<image>
  <external>
     <type>properties</type>
     <prefix>docker</prefix> <!-- this is the default -->
  </external>
</image>

This means you can fully configure your application (build and runtime aspects) via system properties. As this functionality overlaps which your PR (having a docker.runEnv is clearly part of an image of an image configuration), I do not want to add this at a global level outside this handler.

Currently, the only problems we have that this not already works out of the box are:

• The property handler only support docker.env.... properties which are used for both, build and runtime. There should be dedicated runEnv and buildEnv kind of properties.
• When you configure external properties likes this, you cannot use the XML configuration (its an either-or thing). However, it would be easy to allow merging of configuration, that is if also XML config is provided, then it's used as the default, overwritable by properties.

So, ideally, the configuraton should look like:

<image>  
  <external>  <type>properties</type>  </external>
  <name>myimage</name>
  <build> 
       ...
  </build>
   <run>
      ....
   </run>
</image>

Then you can overwrite or add to the configuration by providing the corresponding properties. This also would fit nicely into our current configuration scheme.

If you feel fancy, it would be awesome if you could add this. (happy to help and give pointers). If not, I would tackle this but cannot promise any ETA (so busy until May right now ;-(

@rhuss, thanks for response!

We've looked at the externalised solution, but since the pom file needs total rewrite to use only params and no XML, we rather avoid that. With customers ~100 projects it would be quite messy (hm.. must have counted wrong somewhere, can't really be that many.. but close!)

A merging of XML parameters with properties would indeed be an useful solution, especially if it can be made without adding additional <external> configuration and touching any pom at all (besides bumping version)... ;) Actually while playing with properties it took a while to understand why setting docker.env. didn't have any effect, before I realised that docker.buildArg was a special case and all others required this externalised configuration. Having merged parameters would feel very natural.

The only gotcha is that we don't want to set http_proxy in <run><env>.. since that would be embedded in the image, so some kind of buildEnv vs runEnv would be required.

Feel free to give some pointers on what would have to be done, and depending on how much work it is I might be able to get some time to do it.

Cool. I think its not so hard:

• In the PropertyConfigHandler one would need to adapt https://github.com/rhuss/docker-maven-plugin/blob/4e338524dc89c636535c87f4fa71f2607b266835/src/main/java/io/fabric8/maven/docker/config/handler/property/PropertyConfigHandler.java#L125 to allow .env (for backwards compat) and .runEnv properties, which should be merged (with runEnv having prio). The same for .env and .buildEnv

• In https://github.com/rhuss/docker-maven-plugin/blob/4e338524dc89c636535c87f4fa71f2607b266835/src/main/java/io/fabric8/maven/docker/config/handler/property/PropertyConfigHandler.java#L50-L64 you would need to merge the given config object and the newly created config from the properties. This might become a bit tricky, a deep merged is required. We could use the constructor https://github.com/rhuss/docker-maven-plugin/blob/4b16f1243b77ce5e2c2883976feb17b6404996e2/src/main/java/io/fabric8/maven/docker/config/ImageConfiguration.java#L167-L173 which does a deep copy first, but when adding the property values we need to be careful to (a) not set null values because the property was not given and (b) do a deep merge in Run and Build configuraiton (and the complex structures within it). That's the most difficult task, but you get the idea.

Right, ok will try to take a look at it, probably Friday or next week.
We actually want this to set build/run pullPolicy as well, so multiple reasons to get this fixed.

Any preferences on priority, properties overrides XML I guess, and if external is configured we ignore any XML like is done today?

General maven question, not too sure about all the internals: Should we evaluate System properties too (-D flags), I don't think maven merges those into project properties, but is there some other source of those perhaps?

Actually, I really like to have this merging when <external> is configured, and no property handling if no <external> is given by default (as it is now, except with the additional possibility to merge in XML configuration). And right, properties should then take precedence.

I see your use case that you want to have the property provider enabled by default (to not touch existing configuration), but this would break backwards compatibility, and also, what would you do when multiple images are configured ? (therefor you have the possibility to provide some prefix which is Docker by default).

Maybe a good compromise would be to add a new global configuration option enablePropertyConfiguration or so (which you then also could set via a property) and which then leads to your desired behaviour. This config then could also take the global prefix.

Yeah, sorry for docker.buildArgs being handled differently as the other property based configuration. Actually, when we already had this generic merge, I think this wouldn't have been necessary. Probably I will reconsider this option when the merging is in place.

wdyt ?

btw, I on PTO next week, so don't be surprised if I answer only slowly (well, in the recent busy times my response time was already suboptimal already ... :)

A property such as docker.enablePropertyConfiguration to enable it seems like a good thing. Not adding much complexity to my use-case, and still maintaining the backwards compatibility. Enabling it by default is indeed an issue if you have multiple images in one project, which would all react to docker.xxx properties, so I see the need.

Will give it a try!

Awesome, thanks !

On good way today, created a separate #941 for run/build env, and started on looking into merging too.

Basically, as you mentioned, I start with RunImageConfiguration.Builder(base), and then for every property merge them somehow. Question is, what makes most sense?

For simple String property, this could work:

public Builder imagePullPolicy(String imagePullPolicy) {
            config.imagePullPolicy = firstNonNull(imagePullPolicy, config.imagePullPolicy);
            return this;
        }

I.e, if it is explicitly set by property (which comes as param here), we use that, else we use that from base (which is null if not set).
But then, there is no way to override XML and set null from property... Should a blank string null it out perhaps?

Then we have for example tags, which takes a List of strings. Should the properties override all tags, or extend the list? For tags both scenarios makes sense.
But for runCmds, you only want either base or properties, not something merged.

Same questions applies to Maps, such as buildOptions, args, env.

Thoughts?

Ok, got some more done now. Here is WIP work, no nice descriptive text here yet but if you have feedback please give it :)

stromnet@e5c137a

Opened new PR in #948