Fixes #960 Property placeholders are not interpolated when they are only thing in XML #964

rohanKanojia · 2018-03-06T14:11:40Z

Fixes #960

Somehow these ${...} parameters are not being picked up by maven
properly when used solely without any suffix string literal, the maven
parameter string comes off as NULL. So adding a workaround for this.
We'll use +${...} as a parameter which would be handled by dmp afterwards

codecov · 2018-03-06T14:14:48Z

Codecov Report

Merging #964 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #964      +/-   ##
============================================
+ Coverage     51.65%   51.67%   +0.01%     
- Complexity     1350     1351       +1     
============================================
  Files           147      147              
  Lines          7445     7447       +2     
  Branches       1121     1122       +1     
============================================
+ Hits           3846     3848       +2     
  Misses         3232     3232              
  Partials        367      367

Impacted Files	Coverage Δ	Complexity Δ
...ic8/maven/docker/access/ContainerCreateConfig.java	`77.21% <100%> (+0.59%)`	`26 <0> (+1)`	⬆️

rhuss · 2018-03-12T14:20:35Z

@rohanKanojia thanks for picking that up ! Gimme a bit, just on my way to a conference, so hope to review it soonish.

rhuss

Thanks, looks good !

Howver, I would harden the detection of the + a bit and move the docs to the place where the implicit variables are explained. See comments inline.

rhuss · 2018-03-26T12:07:40Z

src/main/asciidoc/inc/misc/_env.adoc

-When creating a container one or more environment variables can be set via configuration with the `env` parameter
+When creating a container one or more environment variables can be set via configuration with the `env` parameter. Somehow maven is showing weird behavior in case of interpolating single ${..} parameter; so we have provided a workaround for that. An `env` can take the following forms:
+
+.Env format


Thanks for caring about the documentation, too ;-)

However I would describe it a bit shorter. Maybe just:

If the only value of the env parameter is a docker-maven-plugin internal property which has been set implicitely you have to prefix the property with a single + like in +${docker.container.test.ip}. This is necessary due to some Maven limitations which simply interpolates a lone, non defined property, to an empty string which can't then be replaced by this plugin after the initial interpolation phase.

Maybe we can list the affected properties here. Or we should put the whole documentation to the place, where the implicit properties are explained (i think this is even better)

rhuss · 2018-03-26T12:09:43Z

src/main/java/io/fabric8/maven/docker/access/ContainerCreateConfig.java

@@ -57,6 +57,12 @@ public ContainerCreateConfig environment(String envPropsFile, Map<String, String
                String value = entry.getValue();
                if (value == null) {
                    value = "";
+                } else if(value.contains("+")) {


wold prefer a startsWith("+$") here because otherwise its far too unspecific. (Think about values which can validly contain + characters)

Even better, use a regexp ^\+\$\{.*\}$. This would really only match our use case.

rhuss · 2018-03-26T12:11:01Z

src/test/java/io/fabric8/maven/docker/access/ContainerCreateConfigTest.java

@@ -122,6 +123,7 @@ private String copyPropsToFile() throws IOException {
        Map<String,String> envMap = new HashMap<>();
        envMap.put("JAVA_OPTS", "-Xmx512m");
        envMap.put("TEST_SERVICE", "LOGGING");
+        envMap.put("TEST_HTTP_ADDR", "+${docker.container.consul.ip}");


Please add also tests where the + sign is part of a value or when there is something in addition like in +${docker.container.consul.ip}additional_data (here, there should be no removal of the +)

rhuss · 2018-03-26T12:11:54Z

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

+    }
+
+    @Test
+    public void testSerialization() {


what is this test about ?

Sorry, I thought I should add a test to check whether these RunImageConfiguration objects are properly serialized into objects or not. If this doesn't look like a sane to you, I would remove it.

Also, Can there be other tests related to this class?

well, don't know exactly where this class should be serialized.

Adding tests are very valuable, but:

they add maintenance costs, which is ok if the test has some value and tests the right thing. E.g they need to be updated if ImageConfiguration changes

should not test trivial things which is out of scope of the project. E.g. testing Java serialization (which is the task of the JVM) is not in scope of this project. The same btw for testing setter and getter.

So, testing simple, dump configuration classes does not make sense normally, except when it add some extra logic.

What should be tested for sure are classes which use these configuration.

E.g. the latest PR about allowing to merge property based configuration with XML based configuration introduces some kind of logic to the configuration classes which could (and probably should) be tested. However, the plan is to move these classes to a Optional to handle the situation better to distinguish between default values and values set by the user when it comes to mergign (where default values should be ignored).

tl;dr - yes, please remove this test ;-) (but thanks for caring about testing, not every contribution does this)

Sorry for my naive mistakes. I'll update it

hey @rohanKanojia no need to apologize ! it's awesome that you picked up the story and did the implementation. Highly appreciated !

…hey are only thing in XML Somehow these ${...} parameters are not being picked up by maven properly when used solely without any suffix string literal, the maven parameter string comes off as NULL. So adding a workaround for this. We'll use +${...} as a parameter which would be handled by dmp afterwards

rhuss

lgtm, thanks !

rohanKanojia requested a review from rhuss March 6, 2018 14:11

rhuss requested changes Mar 26, 2018

View reviewed changes

rhuss added the changes requested label Mar 26, 2018

rohanKanojia force-pushed the issue960 branch from 992ed24 to 765be06 Compare March 28, 2018 18:24

rohanKanojia force-pushed the issue960 branch from 765be06 to 2839cd5 Compare March 29, 2018 06:22

rohanKanojia added waiting on feedback and removed changes requested labels Apr 3, 2018

rhuss approved these changes Apr 3, 2018

View reviewed changes

rhuss merged commit 4c0fbcd into fabric8io:master Apr 3, 2018

rohanKanojia removed the waiting on feedback label Apr 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #960 Property placeholders are not interpolated when they are only thing in XML #964

Fixes #960 Property placeholders are not interpolated when they are only thing in XML #964

rohanKanojia commented Mar 6, 2018 •

edited

Loading

codecov bot commented Mar 6, 2018 •

edited

Loading

rhuss commented Mar 12, 2018

rhuss left a comment

rhuss Mar 26, 2018

rhuss Mar 26, 2018

rhuss Mar 26, 2018

rhuss Mar 26, 2018

rohanKanojia Mar 27, 2018

rhuss Mar 28, 2018

rohanKanojia Mar 28, 2018

rhuss Mar 28, 2018

rhuss left a comment

Fixes #960 Property placeholders are not interpolated when they are only thing in XML #964

Fixes #960 Property placeholders are not interpolated when they are only thing in XML #964

Conversation

rohanKanojia commented Mar 6, 2018 • edited Loading

codecov bot commented Mar 6, 2018 • edited Loading

Codecov Report

rhuss commented Mar 12, 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

rhuss left a comment

Choose a reason for hiding this comment

rohanKanojia commented Mar 6, 2018 •

edited

Loading

codecov bot commented Mar 6, 2018 •

edited

Loading