Merge pull request #978 from stromnet/propmerge-unbreak-logconfiguration

Unbreak merge of LogConfiguration config
fabric8io · Apr 4, 2018 · 0673619 · 0673619
2 parents 2b0aa69 + c8df0c0
commit 0673619
Show file tree

Hide file tree

Showing 5 changed files with 209 additions and 18 deletions.
diff --git a/doc/changelog.md b/doc/changelog.md
@@ -3,6 +3,7 @@
 * **0.24-SNAPSHOT**
   - Fix possible NPE when logging to a file and the parent directory does not exist yet (#911) (#940)
   - Change content type to "application/json" when talking to the Docker daemon (#945)
+  - Add support for merging properties and POM configuration (#948)
   - PostStart exec breakOnError now fails fast (#970)
   - Use docker.skip.tag property on push and remove (#954) ([#869](https://github.com/fabric8io/docker-maven-plugin/issues/869))
   - Property placeholders are not interpolated when they are the only thing in the XML element value (#960)

diff --git a/src/main/java/io/fabric8/maven/docker/StartMojo.java b/src/main/java/io/fabric8/maven/docker/StartMojo.java
@@ -370,7 +370,7 @@ protected boolean showLogs(ImageConfiguration imageConfig) {
         if (runConfig != null) {
             LogConfiguration logConfig = runConfig.getLogConfiguration();
             if (logConfig != null) {
-                return logConfig.isEnabled();
+                return Boolean.TRUE == logConfig.isEnabled();
             } else {
                 // Default is to show logs if "follow" is true
                 return follow;

diff --git a/src/main/java/io/fabric8/maven/docker/config/LogConfiguration.java b/src/main/java/io/fabric8/maven/docker/config/LogConfiguration.java
@@ -14,7 +14,7 @@ public class LogConfiguration implements Serializable {
     public static final LogConfiguration DEFAULT = new LogConfiguration(false, null, null, null, null, null);
 
     @Parameter(defaultValue = "true")
-    private boolean enabled = true;
+    private Boolean enabled;
 
     @Parameter
     private String prefix;
@@ -33,7 +33,7 @@ public class LogConfiguration implements Serializable {
 
     public LogConfiguration() {}
 
-    private LogConfiguration(boolean enabled, String prefix, String color, String date, String file, LogDriver driver) {
+    private LogConfiguration(Boolean enabled, String prefix, String color, String date, String file, LogDriver driver) {
         this.enabled = enabled;
         this.prefix = prefix;
         this.date = date;
@@ -54,7 +54,7 @@ public String getColor() {
         return color;
     }
 
-    public boolean isEnabled() {
+    public Boolean isEnabled() {
         return enabled;
     }
 
@@ -95,7 +95,7 @@ public Map<String, String> getOpts() {
     // =============================================================================
 
     public static class Builder {
-        private boolean enabled = true;
+        private Boolean enabled = true;
         private String prefix, date, color, file;
         private Map<String, String> driverOpts;
         private String driverName;
@@ -134,6 +134,14 @@ public Builder logDriverOpts(Map<String, String> logOpts) {
             return this;
         }
 
+        /**
+         * Returns true if all options (except enabled) are null, used to decide value of enabled.
+         *
+         * @return
+         */
+        public boolean isBlank() {
+            return prefix == null && date == null && color == null && file == null && driverName == null && driverOpts == null;
+        }
 
         public LogConfiguration build() {
             return new LogConfiguration(enabled, prefix, color, date, file,

diff --git a/src/main/java/io/fabric8/maven/docker/config/handler/property/PropertyConfigHandler.java b/src/main/java/io/fabric8/maven/docker/config/handler/property/PropertyConfigHandler.java
@@ -54,9 +54,10 @@ public List<ImageConfiguration> resolve(ImageConfiguration fromConfig, MavenProj
         String name = valueProvider.getString(NAME, fromConfig.getName());
         String alias = valueProvider.getString(ALIAS, fromConfig.getAlias());
 
-        if (name == null)
+        if (name == null) {
             throw new IllegalArgumentException(String.format("Mandatory property [%s] is not defined", NAME));
-
+        }
+
         return Collections.singletonList(
                 new ImageConfiguration.Builder()
                         .name(name)
@@ -118,9 +119,10 @@ private BuildImageConfiguration extractBuildConfiguration(ImageConfiguration fro
 
     private RunImageConfiguration extractRunConfiguration(ImageConfiguration fromConfig, ValueProvider valueProvider) {
         RunImageConfiguration config = fromConfig.getRunConfiguration();
-        if(config.isDefault())
+        if (config.isDefault()) {
             config = null;
-
+        }
+
         return new RunImageConfiguration.Builder()
                 .capAdd(valueProvider.getList(CAP_ADD, config == null ? null : config.getCapAdd()))
                 .capDrop(valueProvider.getList(CAP_DROP, config == null ? null : config.getCapDrop()))
@@ -197,8 +199,9 @@ private HealthCheckConfiguration extractHealthCheck(HealthCheckConfiguration con
                     .mode(valueProvider.getString(HEALTHCHECK_MODE, config == null || config.getMode() == null ? null : config.getMode().name()))
                     .cmd(extractArguments(valueProvider, HEALTHCHECK_CMD, config == null ? null : config.getCmd()))
                     .build();
-        }else
+        } else {
             return config;
+        }
     }
 
     // Extract only the values of the port mapping
@@ -218,8 +221,9 @@ private List<String> extractPortValues(List<String> config, ValueProvider valueP
 
     private String extractArguments(ValueProvider valueProvider, ConfigKey configKey, Arguments alternative) {
         String rawAlternative = null;
-        if(alternative != null)
+        if (alternative != null) {
             rawAlternative = alternative.getShell();
+        }
 
         return valueProvider.getString(configKey, rawAlternative);
     }
@@ -238,10 +242,15 @@ private LogConfiguration extractLogConfig(LogConfiguration config, ValueProvider
             .prefix(valueProvider.getString(LOG_PREFIX, config == null ? null : config.getPrefix()))
             .logDriverName(valueProvider.getString(LOG_DRIVER_NAME, config == null || config.getDriver() == null ? null : config.getDriver().getName()))
             .logDriverOpts(valueProvider.getMap(LOG_DRIVER_OPTS, config == null || config.getDriver() == null ? null : config.getDriver().getOpts()));
-        Boolean enabled = valueProvider.getBoolean(LOG_ENABLED, config == null ? null : config.isEnabled());
-        if (enabled != null) {
-            builder.enabled(enabled);
+
+        Boolean configEnabled = config != null ? config.isEnabled() : null;
+        Boolean enabled = valueProvider.getBoolean(LOG_ENABLED, configEnabled);
+
+        if (enabled == null) {
+            enabled = configEnabled == Boolean.TRUE || !builder.isBlank();
         }
+
+        builder.enabled(enabled);
         return builder.build();
     }
 
@@ -287,7 +296,7 @@ private WatchImageConfiguration extractWatchConfig(ImageConfiguration fromConfig
 
     private List<UlimitConfig> extractUlimits(List<UlimitConfig> config, ValueProvider valueProvider) {
         List<String> other = null;
-        if(config != null) {
+        if (config != null) {
             other = new ArrayList<>();
             // Convert back to string for potential merge
             for (UlimitConfig ulimitConfig : config) {

diff --git a/src/test/java/io/fabric8/maven/docker/config/handler/property/PropertyConfigHandlerTest.java b/src/test/java/io/fabric8/maven/docker/config/handler/property/PropertyConfigHandlerTest.java
@@ -194,15 +194,13 @@ public void testRunCommandsFromPropertiesAndConfig() {
     @Test
     public void testRunCommandsFromConfigAndProperties() {
         imageConfiguration = new ImageConfiguration.Builder()
-                .externalConfig(new HashMap<String, String>())
+                .externalConfig(externalConfigMode(PropertyMode.Fallback))
                 .buildConfig(new BuildImageConfiguration.Builder()
                         .runCmds(Arrays.asList("some","configured","value"))
                         .build()
                 )
                 .build();
 
-        makeExternalConfigUse(PropertyMode.Fallback);
-
         List<ImageConfiguration> configs = resolveImage(
                 imageConfiguration,props(
                         "docker.from", "base",
@@ -219,6 +217,173 @@ public void testRunCommandsFromConfigAndProperties() {
         assertArrayEquals(new String[]{"some", "configured", "value"}, runCommands);
     }
 
+    @Test
+    public void testDefaultLogEnabledConfiguration() {
+        imageConfiguration = new ImageConfiguration.Builder()
+                .externalConfig(externalConfigMode(PropertyMode.Override))
+                .buildConfig(new BuildImageConfiguration.Builder()
+                        .build()
+                )
+                .build();
+
+        List<ImageConfiguration> configs = resolveImage(
+                imageConfiguration, props(
+                        "docker.from", "base",
+                        "docker.name", "demo")
+        );
+
+        assertEquals(1, configs.size());
+
+        RunImageConfiguration runConfiguration = configs.get(0).getRunConfiguration();
+        assertFalse(runConfiguration.getLogConfiguration().isEnabled());
+
+        // If any log property is set, enabled shall be true by default
+        configs = resolveImage(
+                imageConfiguration, props(
+                        "docker.from", "base",
+                        "docker.name", "demo",
+                        "docker.log.color", "green")
+        );
+
+        runConfiguration = getRunImageConfiguration(configs);
+        assertTrue(runConfiguration.getLogConfiguration().isEnabled());
+        assertEquals("green", runConfiguration.getLogConfiguration().getColor());
+
+
+        // If image configuration has non-blank log configuration, it should become enabled
+        imageConfiguration = new ImageConfiguration.Builder()
+                .externalConfig(externalConfigMode(PropertyMode.Override))
+                .runConfig(new RunImageConfiguration.Builder()
+                        .log(new LogConfiguration.Builder().color("red").build())
+                        .build()
+                )
+                .build();
+
+        configs = resolveImage(
+                imageConfiguration, props(
+                        "docker.from", "base",
+                        "docker.name", "demo")
+        );
+
+        runConfiguration = getRunImageConfiguration(configs);
+        assertTrue(runConfiguration.getLogConfiguration().isEnabled());
+        assertEquals("red", runConfiguration.getLogConfiguration().getColor());
+
+
+        // and if set by property, still enabled but overrides
+        configs = resolveImage(
+                imageConfiguration, props(
+                        "docker.from", "base",
+                        "docker.name", "demo",
+                        "docker.log.color", "yellow")
+        );
+
+        runConfiguration = getRunImageConfiguration(configs);
+        assertTrue(runConfiguration.getLogConfiguration().isEnabled());
+        assertEquals("yellow", runConfiguration.getLogConfiguration().getColor());
+
+
+        // Fallback works as well
+        makeExternalConfigUse(PropertyMode.Fallback);
+        configs = resolveImage(
+                imageConfiguration, props(
+                        "docker.from", "base",
+                        "docker.name", "demo",
+                        "docker.log.color", "yellow")
+        );
+
+        runConfiguration = getRunImageConfiguration(configs);
+        assertTrue(runConfiguration.getLogConfiguration().isEnabled());
+        assertEquals("red", runConfiguration.getLogConfiguration().getColor());
+    }
+
+    @Test
+    public void testExplicitLogEnabledConfiguration() {
+        imageConfiguration = new ImageConfiguration.Builder()
+                .externalConfig(externalConfigMode(PropertyMode.Override))
+                .runConfig(new RunImageConfiguration.Builder()
+                        .log(new LogConfiguration.Builder().color("red").build())
+                        .build()
+                )
+                .build();
+
+        // Explicitly enabled
+        List<ImageConfiguration> configs = resolveImage(
+                imageConfiguration, props(
+                        "docker.from", "base",
+                        "docker.name", "demo",
+                        "docker.log.enabled", "true")
+        );
+
+        RunImageConfiguration runConfiguration = getRunImageConfiguration(configs);
+        assertTrue(runConfiguration.getLogConfiguration().isEnabled());
+        assertEquals("red", runConfiguration.getLogConfiguration().getColor());
+
+        // Explicitly disabled
+        makeExternalConfigUse(PropertyMode.Override);
+        configs = resolveImage(
+                imageConfiguration,props(
+                        "docker.from", "base",
+                        "docker.name","demo",
+                        "docker.log.color", "yellow",
+                        "docker.log.enabled", "false")
+        );
+
+        runConfiguration = getRunImageConfiguration(configs);
+        assertFalse(runConfiguration.getLogConfiguration().isEnabled());
+        assertEquals("yellow", runConfiguration.getLogConfiguration().getColor());
+
+
+        // Disabled by config
+        imageConfiguration = new ImageConfiguration.Builder()
+                .externalConfig(externalConfigMode(PropertyMode.Fallback))
+                .runConfig(new RunImageConfiguration.Builder()
+                        .log(new LogConfiguration.Builder().enabled(false).color("red").build())
+                        .build()
+                )
+                .build();
+
+        configs = resolveImage(
+                imageConfiguration,props(
+                        "docker.from", "base",
+                        "docker.name","demo")
+        );
+
+        runConfiguration = getRunImageConfiguration(configs);
+        assertFalse(runConfiguration.getLogConfiguration().isEnabled());
+        assertEquals("red", runConfiguration.getLogConfiguration().getColor());
+
+        // Enabled by property, with override
+        makeExternalConfigUse(PropertyMode.Override);
+        configs = resolveImage(
+                imageConfiguration,props(
+                        "docker.from", "base",
+                        "docker.name","demo",
+                        "docker.log.enabled", "true")
+        );
+
+        runConfiguration = getRunImageConfiguration(configs);
+        assertTrue(runConfiguration.getLogConfiguration().isEnabled());
+        assertEquals("red", runConfiguration.getLogConfiguration().getColor());
+
+        // Disabled with property too
+        configs = resolveImage(
+                imageConfiguration,props(
+                        "docker.from", "base",
+                        "docker.name","demo",
+                        "docker.log.enabled", "false")
+        );
+
+        runConfiguration = getRunImageConfiguration(configs);
+        assertFalse(runConfiguration.getLogConfiguration().isEnabled());
+        assertEquals("red", runConfiguration.getLogConfiguration().getColor());
+    }
+
+    private RunImageConfiguration getRunImageConfiguration(List<ImageConfiguration> configs) {
+        assertEquals(1, configs.size());
+        return configs.get(0).getRunConfiguration();
+    }
+
     @Test
     public void testEnvAndLabels() throws Exception {
         List<ImageConfiguration> configs = resolveImage(
@@ -513,6 +678,14 @@ private ImageConfiguration buildAnUnresolvedImage() {
                 .build();
     }
 
+    private Map<String, String> externalConfigMode(PropertyMode mode) {
+        Map<String, String> externalConfig = new HashMap<>();
+        if(mode != null) {
+            externalConfig.put("type", "properties");
+            externalConfig.put("mode", mode.name());
+        }
+        return externalConfig;
+    }
 
     private void makeExternalConfigUse(PropertyMode mode) {
         Map<String, String> externalConfig = imageConfiguration.getExternalConfig();