Merging Properties and POM based configuration #948

stromnet · 2018-02-19T13:09:35Z

(originally discussed in #938. Branch based on commit in #941 with run/buildEnv)

Edit: NOTE The following details are NOT what was finally implemented, instead a <mode> property is used. Please refer to the final docs rather than details in this body.

This adds supports to use both Properties based configuration and POM based configuration to configure the same image. This is done by adding the source configuration to the <external> section, which can have one of the following values:

properties - default, same as existing properties based configuration (ie. no merge)
pom, properties - merge pom configs and properties, with values in POM having priority
properties, pom - merge pom configs and properties, with values in properties having priority
(pom - pretty useless, same as not having at all)

Can also be enabled by pure properties (only makes sense for single image), by setting the property docker.externalPropertyConfiguration.source to a valid source value. This is useful if we want to add properties from site-wide config for example settings.xml, for example adding runEnv.http_proxy without touching every POM using the plugin.

Properties are loaded from Maven Project properties merged with System properties (Maven -D flags).

Merging on value level takes place for only a few specific properties by default, for most values it does not make sense to merge (i.e. runCmd, from or any other single values).

For Lists, merging happens by appending all values to one list.
For Maps, merging happens by adding all keys to one map. Source with higher priority overwrites keys with lower priority.

Which values to be merged/replaced are controlled by a new configuration on the ConfigKey enum. This can be overridden by setting the _combine property suffix.

h2. Examples

<properties>
   <docker.network.alias.1>mycontainer</docker.network.alias.1>
</properties>
....
<image>  
  <external>
    <type>properties</type>
    <source>properties,pom</source> <!-- This one is new -->
  </external>
  <name>myimage</name>
  <build> 
       ...
  </build>
   <run>
      ....
   </run>
</image>

Set some properties through -D flags (requires that external is activated in POM)

mvn docker:run -Ddocker.runEnv.http_proxy=http://my.runtime.specific.proxy.com:1234

Set some properties through -D flags, also activating external from -D flag:

mvn docker:run -Ddocker.externalPropertyConfiguration.source=properties,pom \
  -Ddocker.runEnv.http_proxy=http://my.runtime.specific.proxy.com:1234

Override merge policy for runEnv map (only use the one defined in properties, ignoring any in POM):

mvn docker:run -Ddocker.externalPropertyConfiguration.source=properties,pom \
  -Ddocker.runEnv.http_proxy._combine=replace \
  -Ddocker.runEnv.http_proxy=http://my.runtime.specific.proxy.com:1234

codecov · 2018-02-19T13:12:35Z

Codecov Report

Merging #948 into master will increase coverage by 0.25%.
The diff coverage is 77.28%.

@@             Coverage Diff              @@
##             master     #948      +/-   ##
============================================
+ Coverage      51.4%   51.65%   +0.25%     
- Complexity     1232     1350     +118     
============================================
  Files           144      147       +3     
  Lines          7276     7445     +169     
  Branches        985     1121     +136     
============================================
+ Hits           3740     3846     +106     
- Misses         3220     3232      +12     
- Partials        316      367      +51

Impacted Files	Coverage Δ	Complexity Δ
...a/io/fabric8/maven/docker/service/WaitService.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...va/io/fabric8/maven/docker/AbstractDockerMojo.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...ic8/maven/docker/config/AssemblyConfiguration.java	`72.3% <0%> (-4.75%)`	`14 <0> (ø)`
...8/maven/docker/config/BuildImageConfiguration.java	`82.2% <100%> (+0.17%)`	`60 <16> (+14)`	⬆️
.../io/fabric8/maven/docker/config/NetworkConfig.java	`92.06% <100%> (+0.26%)`	`29 <2> (+2)`	⬆️
...n/docker/config/handler/property/PropertyMode.java	`100% <100%> (ø)`	`5 <5> (?)`
...abric8/maven/docker/config/ImageConfiguration.java	`47.5% <100%> (+3.91%)`	`15 <1> (+3)`	⬆️
...fabric8/maven/docker/config/WaitConfiguration.java	`72.52% <100%> (+2.96%)`	`10 <4> (+1)`	⬆️
...a/io/fabric8/maven/docker/config/ConfigHelper.java	`91.66% <100%> (+1.66%)`	`19 <4> (+4)`	⬆️
...ain/java/io/fabric8/maven/docker/util/EnvUtil.java	`70.96% <100%> (+1.37%)`	`49 <1> (+3)`	⬆️
... and 15 more

stromnet · 2018-02-19T14:05:45Z

Ping @rhuss

rhuss · 2018-02-20T07:35:52Z

Awesome, thanks ! I'm on the road this week (and was on PTO last week), so I probably can review the PR only at the beginning of next week.

'hope this is ok ...

stromnet · 2018-02-20T07:46:54Z

Great, OK!
If you could take a quick glance at the above description of functionality (not review of actual implementation) it would be great, if acceptable I can start preparing our projects for using this.

rhuss · 2018-02-20T08:05:07Z

Looks great, thanks again ! I like the way how <source> is implemented.

Some minor nits about the naming: I would probably made it more explicit that it is about naming, and pom is probably a too generic name (as properties are also part of the pom and can e.g. definied in a <properties> section).

What do you thinks about a <mergePolicy> tag with the possible values:

none
properties (or properties,config, maybe we can support both. Or any other idea for a catchy name here ?)
config (or config,properties)
skip

?

Not 100% sure about the _combine suffix (and whether I understand it on a first read ;-). Do you have a use case handy where this required (and can't be solved otherwise) ?

stromnet · 2018-02-20T08:14:21Z

Hm yes I was going back and forth with pom vs config vs something else.. Agree that PM is a bit too ambigious. Other suggestions?

Are you suggesting having mergePolicy instead of source (to indicate which order to merge stuff)? I.e. to set <mergePolicy>properties</mergePolicy> in order to merge existing config with properties, and properties have priority?

The source as it is now is only there to dictate which source to prioritize, pom/config vs properties.
The _combine is different, and allows overriding of what I call CombinePolicy in the implementation, which dictates what to do with a ConfigKey when it sees multiple values.

Basically, each ConfigKey has a CombinePolicy assigned to it, for most it's Replace. That means the highest prioritised value will be used without any sort of merge.
But for some, i.e runEnv, it has CombinePolicy Merge, which allows those values to be merged. For runCmd it does not make sense to have Merge for example, but for runEnv it does, at least we could assume so generally. But if a user actually want's to replace their full runEnv, they can set a property docker.runEnv._combine=replace to force the Replace policy to be used on the runEnv. Same thing with labels etc.

The defaults for the different ConfigKeys are certainly up for discussion. For example, labels, ulimit, and a few others could very well make sense to use Merge by default.

rhuss · 2018-02-20T08:35:10Z

Are you suggesting having mergePolicy instead of source (to indicate which order to merge stuff)? I.e. to set properties in order to merge existing config with properties, and properties have priority?

yes, I think has its about merging configurations maybe this should be reflected in the name of the config option, too. But if you like <source> better, I'm fine with that, too. Could be also 'propertiesFirst' (but that sounds a bit weird these days, too ;-)

I see the point with specifying how to combine values when merging, not 100% sure about how we expose this to the user. Let's discuss this next week, but you can assume that the PR will get in dmp asap (except maybe for minor tweaks), so that you could start migrating your projects already.

stromnet · 2018-02-20T08:39:33Z

I'm thinking that first step is "sourcing values" i.e. from config and/or properties, in some order, and second step is "combining values" to get the final configuration, where "combine" is in some cases merge and in some cases replace.. Although replace might be bad word too, since "replace" indicates that we are replacing something (when all we do is really taking the value with highest priority).

One thing which is not covered is clearing values. For example if we have some property in config, which we want to unset with properties. In some cases a blank string might work but..

Anyway, great, looking forward to further discuss next week and merge :)

stromnet · 2018-02-28T09:07:35Z

@rhuss Hi, had any time to look at this yet? :)

rhuss · 2018-02-28T10:54:35Z

@stromnet yeah, sorry. Back from PTO and conference, and already kneedeep in work again ;-(

I thought a bit about the naming and what do you think about this suggestion:

 <external>
    <type>properties</type>
    <mode>only|override|fallback|skip</mode>
  </external>

The advantage would be

properties is mentioned only once (could otherwise lead to confusion between <type> and <source>
It's short (e.g. not commas) and hopefully intuitive. The <mode> describes the role of the handler its own POV with respect to the other configuration.
Avoids confusion between combining and merging.

I don't mind much that we don't have yet support for clearing values. We should tackle this when it becomes a requirement (i.e. a valid use case exists).

wdyt ? If we agree on the naming I would jump into a reviewer later today or tomorrow.

stromnet · 2018-02-28T14:07:30Z

@rhuss welcome back :)

Good point on properties vs <type> confusion!

So, <mode>only</mode> would be same as todays functionality (and default).
<mode>override</mode> would use both, and let properties override any image configuration set in POM.
<mode>fallback</mode> would use both, and only use properties if not set in image configuration set in POM.

What would skip do? But sounds good to me!

It doesn't address the override/fallback scenarios where we want to do a merge, for example a env map where we want to merge both properties from POM and add some from properties. Or are you happy with the "combine policy" proposal?

rhuss · 2018-02-28T15:45:30Z

What would skip do? But sounds good to me!

skip just would skip using properties (your original pom source). Might be useful to disable it temporarily of for certain profiles. People like to have skip hooks I found out ;-)

Or are you happy with the "combine policy" proposal?

Do you mean the ..._combine specification ? I'm fine with that (and dont have another idea).

stromnet · 2018-02-28T16:10:59Z

skip just would skip using properties (your original pom source). Might be useful to disable it temporarily of for certain profiles. People like to have skip hooks I found out ;-)

Ah, of course. Yes!

Or are you happy with the "combine policy" proposal?

Do you mean the ..._combine specification ? I'm fine with that (and dont have another idea).

Ok, great!

So, existing code should be changed a bit then, but mostly around the ValueSource class and how it is referenced & used I think. I'll have time to fix this on Friday, so if you want you can still review the other parts of the PR tomorrow, the ValueSource specific parts are quite small. Then I can give a updated PR on Friday?

rhuss · 2018-02-28T21:10:37Z

Sounds like a plan ;-) thanks !

rhuss

Hey, awesome work ! Quite a lot of changes required, especially for separating default values implicitly set in the configuration classes.

I'm not sure whether I really love all the raw classes or whether default value handling should be completely different. But that another problem, which we need to think about later (e.g. use 'naked classes' for the configuraton and add only default values when used, or an adapter with default values wrapped around the original configuration classes with null values). Let's check that later, shouldn't be a hurdle for this PR.

Please don't get overwhelmed by the amount of comments. (Sorry, but thats my usual style of review, so it takes a always a bit until I jump on one).

Just check whether the comments make sense to you, and whether you might want to change it yourself. If not, I can do it later, too. Of course, I'm open for discussions, too ;-)

Thanks again, I really like the feature, but we need also to be sure to not break stuff (too much ;-).

rhuss · 2018-03-02T07:59:50Z

src/main/asciidoc/inc/external/_property_configuration.adoc

-| Sets an environment variable. E.g. `<docker.env.JAVA_OPTS>-Xmx512m</docker.env.JAVA_OPTS>` sets the environment variable `JAVA_OPTS`. Multiple such entries can be provided. This environment is used both for building images and running containers. The value cannot be empty but can contain Maven property names which are resolved before the Dockerfile is created.
+| Sets an environment variable used in build and run. E.g. `<docker.env.JAVA_OPTS>-Xmx512m</docker.env.JAVA_OPTS>` sets the environment variable `JAVA_OPTS`. Multiple such entries can be provided. This environment is used both for building images and running containers. The value cannot be empty but can contain Maven property names which are resolved before the Dockerfile is created.
+
+| *docker.buildEnv.VARIABLE*


actually the table in this document is order alphabetically so that one can easier find the right properties. This should be kept this was, but I see that it makes also sense to keep the env vars together.

wdyt about renaming buildEnv to envBuild ? (same for runEnv)

Had the same thought. Problem is, IIRC, that the existing code looks for properties beginning withdocker.env prefix. Consequently, if I had a property docker.envRun.JAVA_OPTS=-Xmx512m it was adding Run.JAVA_OPTS=-Xmx512m in both run and build env.. Or at least something along those lines, don't recall exactly.
Could perhaps be fixed by altering property matching?

I did not recall correct. And luckily enough I had described it there: #941

I was trying to use env.run, which suffered this problem. But envRun should be fine. Let's change!

rhuss · 2018-03-02T08:01:31Z

src/main/java/io/fabric8/maven/docker/config/handler/property/ConfigKey.java

@@ -51,8 +51,11 @@
    DOCKER_FILE,
    DOCKER_FILE_DIR,
    ENTRYPOINT,
+    @Deprecated


Wouldn't call it deprecated, I still think for simplicities reason it makes sense to keep a single env, as some build only do build or runs (and not both).

rhuss · 2018-03-02T08:02:05Z

src/main/java/io/fabric8/maven/docker/config/handler/property/ConfigKey.java

    ENV,
    ENV_PROPERTY_FILE,
+    ENV_BUILD("buildEnv", ValueCombinePolicy.Merge),


again an argument to call it 'envBuild', to keep the mapping of key name to the actual value.

rhuss · 2018-03-02T08:06:43Z

src/main/java/io/fabric8/maven/docker/config/handler/property/ValueProvider.java

+            }
+
+            // Shortcircuit
+            if(values.isEmpty())


Please add braces, we do this even for single line ifs. (Also maybe adapt to the spacing, but I'm not so dogmatic here).

Also for the other 'ifs'

rhuss · 2018-03-02T08:15:42Z

src/main/java/io/fabric8/maven/docker/config/handler/property/ConfigKey.java

@@ -119,22 +123,34 @@
    WATCH_INTERVAL("watch.interval"),
    WATCH_MODE("watch.mode"),
    WATCH_POSTGOAL("watch.postGoal"),
+    WATCH_POSTEXEC("watch.postExec"),


good catch, thanks for adding!

All of these seems to lack documentation in _property_configuration.adoc but leaving that to someone else.. :)

rhuss · 2018-03-02T09:51:23Z

src/main/java/io/fabric8/maven/docker/service/ContainerTracker.java

@@ -199,8 +199,8 @@ private void clearAllMaps() {

            RunImageConfiguration runConfig = imageConfig.getRunConfiguration();
            WaitConfiguration waitConfig = runConfig != null ? runConfig.getWaitConfiguration() : null;
-            this.shutdownGracePeriod = waitConfig != null ? waitConfig.getShutdown() : 0;
-            this.killGracePeriod = waitConfig != null ? waitConfig.getKill() : 0;
+            this.shutdownGracePeriod = waitConfig != null ? waitConfig.shutdown() : 0;


Could we just keep getShutdown() and make the null handling here (to keep the number of different getters lower)

Makes sense, since that one is only used there.

rhuss · 2018-03-02T09:51:31Z

src/main/java/io/fabric8/maven/docker/service/ContainerTracker.java

-            this.shutdownGracePeriod = waitConfig != null ? waitConfig.getShutdown() : 0;
-            this.killGracePeriod = waitConfig != null ? waitConfig.getKill() : 0;
+            this.shutdownGracePeriod = waitConfig != null ? waitConfig.shutdown() : 0;
+            this.killGracePeriod = waitConfig != null ? waitConfig.kill() : 0;


dito (see above)

rhuss · 2018-03-02T09:51:46Z

src/main/java/io/fabric8/maven/docker/service/WaitService.java

@@ -69,7 +69,7 @@ public void wait(ImageConfiguration imageConfig, Properties projectProperties, S

    private int getTimeOut(ImageConfiguration imageConfig) {
        WaitConfiguration wait = getWaitConfiguration(imageConfig);
-        return wait != null ? wait.getTime() : 0;
+        return wait != null ? wait.time() : 0;


rhuss · 2018-03-02T09:51:53Z

src/main/java/io/fabric8/maven/docker/service/WaitService.java

@@ -107,7 +107,7 @@ private String extractCheckerLog(List<WaitChecker> checkers) {
            }
        }

-        if (wait.getHealthy()) {
+        if (wait.healthy()) {


rhuss · 2018-03-02T09:54:57Z

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

+     * @param project Project to extract Properties from
+     * @return
+     */
+    public static Properties getPropertiesWithSystemOverrides(MavenProject project) {


I'm pretty sure we use such a functionality already somewhere else, so would be awesome if we would have a common approach.

Found the following spots on a quick search

not scope of this PR of course, just a reminder to me for a later cleanup.

stromnet · 2018-03-02T12:51:01Z

Great review! Lots of good points, have commented a few of them. Will get on to fix the no-discussion ones in the meantime.

stromnet · 2018-03-02T15:52:35Z

Ok, so as always, lots of stuff came in the way today and I did not have the time to do as much as I wanted... But just pushed some minor fixes at least. May be short on time next week, so not sure how much I'm going to be able to finish..

rhuss · 2018-03-02T19:52:58Z

@stromnet thanks for the feedback ! No worries, I'm currently also very busy, but happy to pick up as soon as you're done.

thanks !

stromnet · 2018-03-09T16:29:23Z

Got some time over, pushed most (all?) minor changes. And one inline-comment on null vs Java 8 above.

rhuss · 2018-03-12T13:18:09Z

cool, thanks ! I'm just on the road for Javaland, but hopefully I find some train-time to make the final review. Let's get it merged asap ;-)

stromnet · 2018-03-12T13:19:26Z

@rhuss There, I think I've addressed all issues now I think, except alternative solution for repeated config == null checks, see that comment (unfortunately hidden as "outdated").

When updates have been reviewed, I suggest we squash everything to single commit.. :)

rhuss · 2018-03-18T09:59:52Z

just a heads up: I'm going to merge the PR this afternoon when I'm back

stromnet · 2018-03-18T10:39:27Z

@rhuss Great :) Any objections to me squashing everything to one commit and re-pushing it before you do that?

Addresses fabric8io#386. Note that it was not possible to use "env.run/build", which would have matched well with other properties, because we already have the "env" matcher (which would have matched env.run too, injecting bad properties). Signed-off-by: Johan Ström <johan@stromnet.se>

…ions Allows that an image is configured from both POM file and properties. Is activated by either setting <source>properties,pom</source> in <external>, or for single image projects by setting property "docker.externalPropertyConfiguration.source=properties,pom". Most properties will be fully replaced, but some are merged (see ConfigKey for list of default combination policies) Signed-off-by: Johan Ström <johan@stromnet.se>

stromnet · 2018-03-18T12:03:16Z

Rebased on master and re-pushed (same branch) with squashed commits.
The original branch if you want to compare it, is on https://github.com/stromnet/docker-maven-plugin/tree/property-merging-nosquash

rhuss · 2018-03-18T18:49:24Z

did a quick re-review of your changes and all looks good. Thanks a lot !

So, let's merge it now.

However, for the next steps I really would like to streamline the default handling (i.e. how to provide default values, maybe getting rid of the "raw" methods and null checks).
We should use the full Java 8 machinery for this for sure.

Not sure how to tackle this, but would be super happy if you could give it a spin.

Thanks again a ton, it's really a super valuable feature. It does not look big, but make this plugin much more flexible.

stromnet · 2018-03-19T07:33:53Z

Perfect, thanks! Eagerly waiting for the release then :)

One oops: noticed my squashed commit msg mentions the wrong property.

stromnet mentioned this pull request Feb 19, 2018

Add support for docker.runEnv properties and -D flags #938

Closed

rhuss requested changes Mar 2, 2018

View reviewed changes

stromnet mentioned this pull request Mar 2, 2018

Add separate properties for build.env.build and run.env #941

Closed

stromnet added 2 commits March 18, 2018 12:18

stromnet force-pushed the property-merging branch from 4948880 to ac1db28 Compare March 18, 2018 12:02

rhuss approved these changes Mar 18, 2018

View reviewed changes

rhuss merged commit 33f11d8 into fabric8io:master Mar 18, 2018

rhuss mentioned this pull request Mar 18, 2018

Different configuration for run and build via PropertyConfigHandler #386

Closed

stromnet deleted the property-merging branch March 19, 2018 07:35

stromnet mentioned this pull request Mar 26, 2018

Unbreak merge of LogConfiguration config #978

Merged

stromnet mentioned this pull request Apr 25, 2018

Unbreak use of build/dockerFile in properties mode #1012

Merged

stromnet mentioned this pull request May 9, 2018

Unbreak setting of entrypoint in exec form when property mode is enabled #1020

Merged

Merging Properties and POM based configuration #948

Merging Properties and POM based configuration #948

Conversation

stromnet commented Feb 19, 2018 • edited Loading

codecov bot commented Feb 19, 2018 • edited Loading

Codecov Report

stromnet commented Feb 19, 2018

rhuss commented Feb 20, 2018

stromnet commented Feb 20, 2018

rhuss commented Feb 20, 2018

stromnet commented Feb 20, 2018

rhuss commented Feb 20, 2018

stromnet commented Feb 20, 2018

stromnet commented Feb 28, 2018

rhuss commented Feb 28, 2018

stromnet commented Feb 28, 2018

rhuss commented Feb 28, 2018

stromnet commented Feb 28, 2018

rhuss commented Feb 28, 2018

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

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

stromnet Mar 12, 2018 • edited Loading

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

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

stromnet commented Mar 2, 2018

stromnet commented Mar 2, 2018

rhuss commented Mar 2, 2018

stromnet commented Mar 9, 2018

rhuss commented Mar 12, 2018

stromnet commented Mar 12, 2018 • edited Loading

rhuss commented Mar 18, 2018

stromnet commented Mar 18, 2018

stromnet commented Mar 18, 2018

rhuss commented Mar 18, 2018

stromnet commented Mar 19, 2018

stromnet commented Feb 19, 2018 •

edited

Loading

codecov bot commented Feb 19, 2018 •

edited

Loading

stromnet Mar 12, 2018 •

edited

Loading

stromnet commented Mar 12, 2018 •

edited

Loading