Fix #986 : Create volumes with proper configuration during "docker:start" #1007

rohanKanojia · 2018-04-23T07:09:38Z

codecov · 2018-04-23T07:21:50Z

Codecov Report

Merging #1007 into master will increase coverage by 0.51%.
The diff coverage is 65%.

@@             Coverage Diff              @@
##             master    #1007      +/-   ##
============================================
+ Coverage      51.3%   51.81%   +0.51%     
- Complexity     1429     1441      +12     
============================================
  Files           150      150              
  Lines          7755     7775      +20     
  Branches       1151     1156       +5     
============================================
+ Hits           3979     4029      +50     
+ Misses         3384     3350      -34     
- Partials        392      396       +4

Impacted Files	Coverage Δ	Complexity Δ
...c/main/java/io/fabric8/maven/docker/StartMojo.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...va/io/fabric8/maven/docker/service/RunService.java	`51.07% <81.25%> (+2.22%)`	`25 <3> (+3)`	⬆️
.../io/fabric8/maven/docker/service/WatchService.java	`3.79% <0%> (+3.79%)`	`1% <0%> (+1%)`	⬆️
...a/io/fabric8/maven/docker/service/WaitService.java	`4.42% <0%> (+4.42%)`	`1% <0%> (+1%)`	⬆️
...o/fabric8/maven/docker/service/ArchiveService.java	`26.66% <0%> (+26.66%)`	`1% <0%> (+1%)`	⬆️
...va/io/fabric8/maven/docker/service/ServiceHub.java	`36.58% <0%> (+36.58%)`	`3% <0%> (+3%)`	⬆️
...abric8/maven/docker/service/ServiceHubFactory.java	`80% <0%> (+80%)`	`2% <0%> (+2%)`	⬆️

rhuss

Looks good to me, except that we should move the method to the RunService (and some comments on the style, too).

Some test would be awesome, too (which is easier todo when specified in the RunService).

This PR is still marked as WIP. Is this still true ?

rhuss · 2018-05-16T01:14:26Z

src/main/java/io/fabric8/maven/docker/StartMojo.java

@@ -331,6 +336,26 @@ public StartedContainer call() throws Exception {
        return imagesWaitingToStart;
    }

+    private void createVolumesAsPerVolumeBinds(ServiceHub hub, List<String> volumeBinds, List<VolumeConfiguration> volumesConfigs)


can we move this out of the StartMojo into the RunService ? The Mojo should stay as slim as possible.

rhuss · 2018-05-16T01:14:54Z

src/main/java/io/fabric8/maven/docker/StartMojo.java

+    private void createVolumesAsPerVolumeBinds(ServiceHub hub, List<String> volumeBinds, List<VolumeConfiguration> volumesConfigs)
+        throws DockerAccessException {
+        Map<String, Integer> aIndexMap = new HashMap();
+        for(Integer aIndex = 0; aIndex < volumesConfigs.size(); aIndex++)


please add braces

rhuss · 2018-05-16T01:18:11Z

src/main/java/io/fabric8/maven/docker/StartMojo.java

+            if(aVolumeBind.contains(":")) {
+                aVolumeBind = aVolumeBind.substring(0, aVolumeBind.indexOf(":"));
+            }
+            Integer nVolumeConfigIndex = aIndexMap.get(aVolumeBind);


What's the difference between an n and a a prefix for your variables names ? tbh, I would avoid that kind of prefixing as we don't do it nowhere else in this code base. It's always good to adapt to a given codestyle instead of introducing ones own (like avoid braces or spaces after for or if). This helps to keep the code consistently readable.

rohanKanojia · 2018-07-30T17:00:49Z

@rhuss : Could you please take a look at it again? I've modified it according to your comments :)

rhuss · 2018-07-31T05:56:35Z

thanks, will do ASAP !

…docker:start"

rhuss · 2018-09-25T12:25:22Z

@rohanKanojia Thanks ! Going to merge now ...

jmayday · 2018-09-28T10:14:02Z

I'm afraid it caused regression:
#1093

rohanKanojia · 2018-09-28T10:15:43Z

@jmayday @rhuss : Sorry for causing regression 😞

jmayday · 2018-09-28T10:28:10Z

It's OK, I'm fine with downgrading for now. It just took time to analyze what's going on.

rohanKanojia · 2018-09-28T10:29:42Z

@jmayday : bdw, Roland fixed this in #1092 . It would be released soon :-)

rhuss · 2018-09-28T10:40:13Z

np, its was a bad reviewer ;-) 'going to release 0.27.1 this evening ...

rohanKanojia added the WIP Work in Progress label Apr 23, 2018

rhuss requested changes May 16, 2018

View reviewed changes

rhuss added the changes requested label May 16, 2018

rohanKanojia removed the WIP Work in Progress label May 16, 2018

rohanKanojia force-pushed the issue986 branch 2 times, most recently from 0e45351 to a770153 Compare May 16, 2018 18:57

rohanKanojia added waiting on feedback and removed changes requested labels May 16, 2018

rohanKanojia force-pushed the issue986 branch 2 times, most recently from d86b78d to 67563cb Compare July 6, 2018 11:43

rohanKanojia force-pushed the issue986 branch from 67563cb to ca2a8f0 Compare July 30, 2018 17:00

rhuss force-pushed the issue986 branch 2 times, most recently from 187acea to ca2a8f0 Compare September 25, 2018 12:07

Fix fabric8io#986 : Create volumes with proper configuration during "…

c1c7d35

…docker:start"

rhuss force-pushed the issue986 branch from ca2a8f0 to c1c7d35 Compare September 25, 2018 12:08

rhuss removed the waiting on feedback label Sep 25, 2018

rhuss added 3 commits September 25, 2018 14:10

fix: Missing Import

f1ca5e0

fix: Missing imports

c210d99

chore: Update documentation for implicit volume creation

1e5b6ab

rhuss approved these changes Sep 25, 2018

View reviewed changes

rhuss merged commit 4481c2f into fabric8io:master Sep 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #986 : Create volumes with proper configuration during "docker:start" #1007

Fix #986 : Create volumes with proper configuration during "docker:start" #1007

rohanKanojia commented Apr 23, 2018

codecov bot commented Apr 23, 2018 •

edited

Loading

rhuss left a comment

rhuss May 16, 2018

rhuss May 16, 2018

rhuss May 16, 2018

rohanKanojia commented Jul 30, 2018

rhuss commented Jul 31, 2018

rhuss commented Sep 25, 2018

jmayday commented Sep 28, 2018

rohanKanojia commented Sep 28, 2018 •

edited

Loading

jmayday commented Sep 28, 2018

rohanKanojia commented Sep 28, 2018

rhuss commented Sep 28, 2018

Fix #986 : Create volumes with proper configuration during "docker:start" #1007

Fix #986 : Create volumes with proper configuration during "docker:start" #1007

Conversation

rohanKanojia commented Apr 23, 2018

codecov bot commented Apr 23, 2018 • edited Loading

Codecov Report

rhuss left a comment

Choose a reason for hiding this comment

rhuss May 16, 2018

Choose a reason for hiding this comment

rhuss May 16, 2018

Choose a reason for hiding this comment

rhuss May 16, 2018

Choose a reason for hiding this comment

rohanKanojia commented Jul 30, 2018

rhuss commented Jul 31, 2018

rhuss commented Sep 25, 2018

jmayday commented Sep 28, 2018

rohanKanojia commented Sep 28, 2018 • edited Loading

jmayday commented Sep 28, 2018

rohanKanojia commented Sep 28, 2018

rhuss commented Sep 28, 2018

codecov bot commented Apr 23, 2018 •

edited

Loading

rohanKanojia commented Sep 28, 2018 •

edited

Loading