Add possiblity to use docker.imagePropertyConfiguration with multipe images #1001

stromnet · 2018-04-16T12:02:01Z

By default it works as previous, but if explicit is added to each image, and i.e. setting a custom prefix, it work.

This is useful when i.e. env has docker.imagePropertyConfiguration set globally (i.e. settings.xml), and a Maven project happens to have multiple images.

Also add support for docker.imagePropertyConfiguration=skip, for i.e. overriding parents config and disable it (note that setting this in pom will not override a value in settings.xml, mostly useful to override parent pom).

… images By default it will not work, but if explicit <external> is added to each image, it work. Also add support for docker.imagePropertyConfiguration=skip, for i.e. overriding parents config and disable it.

codecov · 2018-04-16T12:05:14Z

Codecov Report

Merging #1001 into master will increase coverage by 0.08%.
The diff coverage is 82.92%.

@@             Coverage Diff              @@
##             master    #1001      +/-   ##
============================================
+ Coverage      51.8%   51.89%   +0.08%     
- Complexity     1365     1379      +14     
============================================
  Files           147      147              
  Lines          7460     7503      +43     
  Branches       1132     1141       +9     
============================================
+ Hits           3865     3894      +29     
- Misses         3229     3243      +14     
  Partials        366      366

Impacted Files	Coverage Δ	Complexity Δ
...va/io/fabric8/maven/docker/AbstractDockerMojo.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...a/io/fabric8/maven/docker/config/UlimitConfig.java	`71.42% <0%> (ø)`	`9 <1> (ø)`	⬇️
...n/docker/config/handler/property/PropertyMode.java	`100% <100%> (ø)`	`5 <5> (ø)`	⬇️
...a/io/fabric8/maven/docker/config/ConfigHelper.java	`93.22% <100%> (+1.55%)`	`23 <0> (+4)`	⬆️
.../docker/config/handler/property/ValueProvider.java	`88.88% <100%> (ø)`	`10 <0> (ø)`	⬇️
...ven/docker/config/handler/ImageConfigResolver.java	`93.54% <75%> (-2.75%)`	`11 <1> (+1)`
...config/handler/property/PropertyConfigHandler.java	`72.47% <81.81%> (+0.7%)`	`114 <6> (+7)`	⬆️
...aven/docker/config/DockerMachineConfiguration.java	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
...abric8/maven/docker/assembly/DockerFileOption.java	`100% <0%> (ø)`	`4% <0%> (ø)`	⬇️
...aven/docker/config/handler/property/ConfigKey.java	`100% <0%> (ø)`	`10% <0%> (ø)`	⬇️
... and 9 more

stromnet · 2018-04-25T07:38:32Z

@rhuss Follow up for my previous work, any chance of merge & release? :)

rhuss

Technically the PR look good, thanks ! (have some minor comments, though).

However I'm afraid that it makes things even more complex as they are already and its also hard to explain and understand whats going on.

Do you have a use case in hand where you want to mix explicit configured <external> property handlers and implicit added via the global property ?

Actually, I would prefer to ignore the external property docker.imagePropertyConfiguration when one image used an explicit external property handler (because then you already "in this business").

Everything makes things so much harder to understand and debug, especially when combined with other external handlers like docker-compose.

But maybe you have a convincing use case at hand where this would make sense ? ;)

rhuss · 2018-04-25T15:19:49Z

src/test/java/io/fabric8/maven/docker/config/ConfigHelperTest.java

@@ -71,11 +69,41 @@ public void externalPropertyActivation() throws MojoFailureException {
            ConfigHelper.validateExternalPropertyActivation(project, images);
            fail();
        }catch(MojoFailureException ex) {
-            assertTrue(ex.getMessage().contains("Configuration error: Cannot use property"));
+            assertEquals("Configuration error: Cannot use property " + ConfigHelper.EXTERNALCONFIG_ACTIVATION_PROPERTY + " on projects with multiple images without explicit image external configuration.",


I would only check on parts with a essential to provide a meaningful context in the error message. That way the check is more robust on changes wrt rewording of the message. So, I always prefer contains on the essential parts (like contains(ConfigHelper.EXTERNALCONFIG_ACTIVATION_PROPERTY))

rhuss · 2018-04-25T15:21:22Z

src/main/java/io/fabric8/maven/docker/config/ConfigHelper.java

+        int imagesWithoutExternalConfig = 0;
+        for (ImageConfiguration image : images) {
+            if(PropertyConfigHandler.canCoexistWithOtherPropertyConfiguredImages(image.getExternalConfig()))
+                continue;


can we please agree on adding brackets even around single line conditionals ? This is according to the existing style and less error-prone (believe, I hit that when adding a second line).

Also, would prefer to have spaces between if( but I'm less dogmatic here ;-)

Gah, I thought I configured IntelliJ to do that...

rhuss · 2018-04-25T15:24:10Z

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

+If set globally (i.e. parent POM), but not wanted locally, the property could be overriden to `skip` to disabled it.
+Note that settings.xml properties take precedence over any properties defined in pom.xml.
+
+For configurations with multiple images, using this property will produce an error. All images would use the docker prefix,


maybe highlight "docker" as this is a literal.

maybe: "All images would use the same docker prefix ..." ?

rhuss · 2018-04-25T15:24:14Z

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

+global settings.xml).
+
+If set globally (i.e. parent POM), but not wanted locally, the property could be overriden to `skip` to disabled it.
+Note that settings.xml properties take precedence over any properties defined in pom.xml.


How would one then could add 'skip' in the pom.xml when the properties in settings.xml has higher prio ? It probably only makes sense when used in pom.xml inheritance.

That comment is more in general for all properties, inherently by maven any properties in settings.xml seems to override my projects properties.

rhuss · 2018-04-25T15:24:45Z

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

+[[combining-property-config-externally]]
+.Activating property configuration externally
+It also possible to activate property configuration by setting the property `docker.imagePropertyConfiguration` to a
+valid `property mode`, without adding an `<external>` section.


would add here, that the configuration then looks for the default prefix of docker.

rhuss · 2018-04-25T15:38:20Z

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

+Note that settings.xml properties take precedence over any properties defined in pom.xml.
+
+For configurations with multiple images, using this property will produce an error. All images would use the docker prefix,
+they would all be given the same configuration, and that is probably not intended.


"probably" is a bit weak.

Maybe: "All images would then use the same docker property prefix, resulting in multiple identical configurations"

stromnet · 2018-04-25T15:58:01Z

The use case is actually not mixing and docker.imagePropertyConfiguration.

The use case here was that I globally (through settings.xml) set docker.imagePropertyConfiguration=override and docker.runEnv.http_proxy=xxxx. But that effectively breaks any of our projects (although very few) with two images in the same d-m-p config.

Thus I don't really have a need to add to either of the images, that was more of a way to work around it.. since both images cannot use the same prefix :)

The opposite solution would be, if more than one image exists, we ignore any docker.imagePropertyConfiguration, and enforce the caller to explicitly set sections. in case they actually want any merging to happen.. But that hides logic (docker.imagePropertyConfiguration suddenly not working if >1 image), so I don't really like that.

stromnet · 2018-04-27T12:39:41Z

All code comments fixed. Also adjusted the external activation & multi-image check a bit: if prefix is explicitly set (to anything), allow any number of images with same prefix. It is useful if most of config is set in POM, but still want to use override for some properties such as docker.envRun.http_proxy=... which should apply to all images.

stromnet · 2018-05-08T16:41:29Z

@rhuss any additional feedback? :)

rhuss · 2018-05-15T02:15:52Z

@stromnet just back from Red Hat Summit, tomorrow is docker-maven-plugin day ;-) 'hope to work on as many PRs as possible, including this one.

rhuss

hey. Still not 100% convinced, let me sleep one night over it :) maybe some good example in the doc would help.

My point is, that you can quickly run into situation which are not easily understandable whats happening if the configuration comes from different places (like mixing property and XML handling).

At least we should add some more diagnostic message on verbose level so that people can understand whats going on.

rhuss · 2018-05-16T01:05:27Z

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

+images (or at least all but one). Normally you'd want to use different prefix for each image, but if explicitly set
+it does allow you to use the same prefix (even `docker`) on all images. This is useful in case you just want to share
+a few properties. This only makes sense when `property mode` is _override_ or _fallback_ and image-specific configuration
+are defined in the POM configuration.


Can we add an example here please, e.g. the example for setting the http proxy for all configured images with a single property (i.e. you use case) ? I think this would help to understand people why it would make sense sometimes to use one property for all configured images.

Good point. An example has been added for http_proxy and a single image, as well as an example with two images (using the same prefix).

stromnet · 2018-05-16T08:48:04Z

Improved docs, and added some verbose logging when the external activation property is handled. I was thinking of adding something like "for image ", but that is tricky to fetch since it does not necessarily have a name at that time (we don't yet know if it is from config or from properties).
But at least it hints that a global property is present!

Not sure what more verbose log to add. Perhaps dump the whole image configuration after they have been resolved, or is that more suitable for debug perhaps?

rhuss · 2018-05-16T18:39:33Z

Looks good to me, thanks for the docs and examples. Let's get that merged, also to gather some feedback.

thanks !

stromnet · 2018-05-16T20:42:55Z

Great, thanks! Hoping for a 0.25.3 soon then (and hopefully no more bugs...) :)

Add possiblity to use docker.imagePropertyConfiguration with multiple…

72f81f3

… images By default it will not work, but if explicit <external> is added to each image, it work. Also add support for docker.imagePropertyConfiguration=skip, for i.e. overriding parents config and disable it.

stromnet force-pushed the multiple-images-with-external-property-config branch from d51a1ac to 72f81f3 Compare April 16, 2018 12:03

rhuss requested changes Apr 25, 2018

View reviewed changes

stromnet added 4 commits April 27, 2018 13:35

Use braces on if

532a869

Less strict exception test

7bbdb9a

Documentation fixes

ca66d37

Allow same prefix on multiple images, if explicitly set

c7e258d

rhuss reviewed May 16, 2018

View reviewed changes

stromnet added 2 commits May 16, 2018 10:31

Improved documentation with examples of external property mode enabling

84116ab

Add verbose log if externalconfig property is set

48a5598

rhuss approved these changes May 16, 2018

View reviewed changes

rhuss merged commit 1a6113f into fabric8io:master May 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add possiblity to use docker.imagePropertyConfiguration with multipe images #1001

Add possiblity to use docker.imagePropertyConfiguration with multipe images #1001

stromnet commented Apr 16, 2018

codecov bot commented Apr 16, 2018 •

edited

Loading

stromnet commented Apr 25, 2018

rhuss left a comment

rhuss Apr 25, 2018

rhuss Apr 25, 2018

stromnet Apr 25, 2018

rhuss Apr 25, 2018

rhuss Apr 25, 2018

rhuss Apr 25, 2018

stromnet Apr 25, 2018

rhuss Apr 25, 2018

rhuss Apr 25, 2018

stromnet commented Apr 25, 2018

stromnet commented Apr 27, 2018

stromnet commented May 8, 2018

rhuss commented May 15, 2018

rhuss left a comment

rhuss May 16, 2018

stromnet May 16, 2018

stromnet commented May 16, 2018

rhuss commented May 16, 2018

stromnet commented May 16, 2018

Add possiblity to use docker.imagePropertyConfiguration with multipe images #1001

Add possiblity to use docker.imagePropertyConfiguration with multipe images #1001

Conversation

stromnet commented Apr 16, 2018

codecov bot commented Apr 16, 2018 • edited Loading

Codecov Report

stromnet commented Apr 25, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stromnet commented Apr 25, 2018

stromnet commented Apr 27, 2018

stromnet commented May 8, 2018

rhuss commented May 15, 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

stromnet commented May 16, 2018

rhuss commented May 16, 2018

stromnet commented May 16, 2018

codecov bot commented Apr 16, 2018 •

edited

Loading