feat: added support for overriding Build Args appearing in multiple s…

…ources (3637) feat: added support for overriding build args from multiple sources Signed-off-by: l3002 <[email protected]> --- chore(test): updated unit tests for build arg override Signed-off-by: l3002 <[email protected]> --- doc: updated docs for merging build args from multiple sources Signed-off-by: l3002 <[email protected]> --- doc: updated user facing docs with order of precedence Signed-off-by: l3002 <[email protected]> --- chore: updated docs & comments with mentioned suggestions Signed-off-by: l3002 <[email protected]>
eclipse-jkube · Feb 4, 2025 · 954b72a · 954b72a
1 parent eaca6c7
commit 954b72a
Show file tree

Hide file tree

Showing 7 changed files with 138 additions and 60 deletions.
diff --git a/.../build/api/src/main/java/org/eclipse/jkube/kit/build/api/helper/BuildArgResolverUtil.java b/.../build/api/src/main/java/org/eclipse/jkube/kit/build/api/helper/BuildArgResolverUtil.java
@@ -15,72 +15,93 @@
 
 import org.apache.commons.lang3.StringUtils;
 import org.eclipse.jkube.kit.common.JKubeConfiguration;
-import org.eclipse.jkube.kit.common.JKubeException;
+import org.eclipse.jkube.kit.common.KitLogger;
 import org.eclipse.jkube.kit.config.image.ImageConfiguration;
 
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Properties;
-import java.util.stream.Stream;
 
 public class BuildArgResolverUtil {
 
   private static final String ARG_PREFIX = "docker.buildArg.";
 
-  private BuildArgResolverUtil() { }
+  private BuildArgResolverUtil() {
+  }
 
   /**
-   * Merges Docker Build Args from the following sources:
+   * Merges Docker Build Args from the following source in the mentioned order of precedence (moving from higher to lower precedence):
    * <ul>
    *   <li>Build Args specified directly in ImageConfiguration</li>
    *   <li>Build Args specified via System Properties</li>
    *   <li>Build Args specified via Project Properties</li>
    *   <li>Build Args specified via Plugin configuration</li>
    *   <li>Docker Proxy Build Args detected from ~/.docker/config.json</li>
    * </ul>
+   *
+   * <i><b>Note:</b> When identical Build Args are specified in multiple sources, their values are overridden according to the established order of precedence. A warning is also logged to highlight the Build Arg and the updated value.</i>
+   *
    * @param imageConfig ImageConfiguration where to get the Build Args from.
    * @param configuration {@link JKubeConfiguration}.
    * @return a Map containing merged Build Args from all sources.
    */
-  public static Map<String, String> mergeBuildArgsIncludingLocalDockerConfigProxySettings(ImageConfiguration imageConfig, JKubeConfiguration configuration) {
-    return mergeBuildArgsFrom(imageConfig.getBuild().getArgs(),
-            buildArgsFromProperties(System.getProperties()),
-            buildArgsFromProperties(configuration.getProject().getProperties()),
-            configuration.getBuildArgs(),
-            buildArgsFromDockerConfig());
+  public static Map<String, String> mergeBuildArgsIncludingLocalDockerConfigProxySettings(ImageConfiguration imageConfig,
+      JKubeConfiguration configuration, KitLogger logger) {
+    List<Map<String, String>> buildArgSources = new ArrayList<>();
+
+    // Add build arg sources following order of precedence
+    buildArgSources.add(buildArgsFromDockerConfig());
+    buildArgSources.add(configuration.getBuildArgs());
+    buildArgSources.add(buildArgsFromProperties(configuration.getProject().getProperties()));
+    buildArgSources.add(buildArgsFromProperties(System.getProperties()));
+    buildArgSources.add(imageConfig.getBuild().getArgs());
+    return mergeBuildArgsFrom(buildArgSources, logger);
   }
 
   /**
-   * Merges Docker Build Args from the following sources:
+   * Merges Docker Build Args from the following source in the mentioned order of precedence (moving from higher to lower precedence):
    * <ul>
    *   <li>Build Args specified directly in ImageConfiguration</li>
    *   <li>Build Args specified via System Properties</li>
    *   <li>Build Args specified via Project Properties</li>
    *   <li>Build Args specified via Plugin configuration</li>
    * </ul>
+   *
+   * <i><b>Note:</b> When identical Build Args are specified in multiple sources, their values are overridden according to the established order of precedence. A warning is also logged to highlight the Build Arg and the updated value.</i>
+   *
    * @param imageConfig ImageConfiguration where to get the Build Args from.
    * @param configuration {@link JKubeConfiguration}.
    * @return a Map containing merged Build Args from all sources.
    */
-  public static Map<String, String> mergeBuildArgsWithoutLocalDockerConfigProxySettings(ImageConfiguration imageConfig, JKubeConfiguration configuration) {
-    return mergeBuildArgsFrom(imageConfig.getBuild().getArgs(),
-        buildArgsFromProperties(System.getProperties()),
-        buildArgsFromProperties(configuration.getProject().getProperties()),
-        configuration.getBuildArgs());
+  public static Map<String, String> mergeBuildArgsWithoutLocalDockerConfigProxySettings(ImageConfiguration imageConfig,
+      JKubeConfiguration configuration, KitLogger logger) {
+
+    List<Map<String, String>> buildArgSources = new ArrayList<>();
+
+    // Add build arg sources following increasing order of precedence
+    buildArgSources.add(configuration.getBuildArgs());
+    buildArgSources.add(buildArgsFromProperties(configuration.getProject().getProperties()));
+    buildArgSources.add(buildArgsFromProperties(System.getProperties()));
+    buildArgSources.add(imageConfig.getBuild().getArgs());
+    return mergeBuildArgsFrom(buildArgSources, logger);
   }
 
-  @SafeVarargs
-  private static Map<String, String> mergeBuildArgsFrom(Map<String, String>... buildArgSources) {
+  private static Map<String, String> mergeBuildArgsFrom(List<Map<String, String>> buildArgSources, KitLogger logger) {
     final Map<String, String> buildArgs = new HashMap<>();
-    Stream.of(buildArgSources)
+    buildArgSources.stream()
         .filter(Objects::nonNull)
         .flatMap(map -> map.entrySet().stream())
         .forEach(entry -> {
           if (buildArgs.containsKey(entry.getKey())) {
-            throw new JKubeException(String.format("Multiple Build Args with the same key: %s=%s and %s=%s",
-                entry.getKey(), buildArgs.get(entry.getKey()), entry.getKey(), entry.getValue()));
+            logger.warn(
+                String.format("Multiple Build Args with the same key: %s=%s and %s=%s, overriding value of key to %s=%s",
+                    entry.getKey(), buildArgs.get(entry.getKey()), entry.getKey(), entry.getValue(), entry.getKey(),
+                    entry.getValue())
+            );
           }
           buildArgs.put(entry.getKey(), entry.getValue());
         });
@@ -122,9 +143,9 @@ private static Map<String, String> buildArgsFromDockerConfig() {
             "ftpProxy", "ftp_proxy"
         };
 
-        for(int index = 0; index < proxyMapping.length; index += 2) {
+        for (int index = 0; index < proxyMapping.length; index += 2) {
           if (defaultProxyObj.containsKey(proxyMapping[index])) {
-            buildArgs.put(ARG_PREFIX + proxyMapping[index+1], defaultProxyObj.get(proxyMapping[index]));
+            buildArgs.put(ARG_PREFIX + proxyMapping[index + 1], defaultProxyObj.get(proxyMapping[index]));
           }
         }
       }

diff --git a/...t/java/org/eclipse/jkube/kit/build/api/helper/BuildArgResolverUtilMergeBuildArgsTest.java b/...t/java/org/eclipse/jkube/kit/build/api/helper/BuildArgResolverUtilMergeBuildArgsTest.java
@@ -14,8 +14,8 @@
 package org.eclipse.jkube.kit.build.api.helper;
 
 import org.eclipse.jkube.kit.common.JKubeConfiguration;
-import org.eclipse.jkube.kit.common.JKubeException;
 import org.eclipse.jkube.kit.common.JavaProject;
+import org.eclipse.jkube.kit.common.KitLogger;
 import org.eclipse.jkube.kit.common.util.EnvUtil;
 import org.eclipse.jkube.kit.config.image.ImageConfiguration;
 import org.eclipse.jkube.kit.config.image.build.BuildConfiguration;
@@ -36,13 +36,16 @@
 import java.util.Properties;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 
 class BuildArgResolverUtilMergeBuildArgsTest {
   private ImageConfiguration imageConfiguration;
   private JKubeConfiguration jKubeConfiguration;
   private Properties projectProperties;
   private Map<String, String> buildArgFromPluginConfiguration;
+  private KitLogger kitLogger;
 
   @BeforeEach
   void setUp() {
@@ -59,6 +62,7 @@ void setUp() {
         .build(BuildConfiguration.builder()
             .build())
         .build();
+    kitLogger = spy(new KitLogger.SilentLogger());
   }
 
   @Test
@@ -75,7 +79,8 @@ void whenBuildArgsFromImageConfigAndFromProjectProperties_shouldMergeBuildArgs()
         .build();
 
     // When
-    Map<String, String> mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(imageConfiguration, jKubeConfiguration);
+    Map<String, String> mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(
+        imageConfiguration, jKubeConfiguration, kitLogger);
 
     // Then
     assertThat(mergedBuildArgs)
@@ -95,7 +100,8 @@ void fromAllSourcesWithDifferentKeys_shouldMergeBuildArgs() {
     givenBuildArgsFromJKubeConfiguration("FULL_IMAGE", "busybox:latest");
 
     // When
-    Map<String, String> mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(imageConfiguration, jKubeConfiguration);
+    Map<String, String> mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(
+        imageConfiguration, jKubeConfiguration, kitLogger);
 
     // Then
     assertThat(mergedBuildArgs)
@@ -106,42 +112,81 @@ void fromAllSourcesWithDifferentKeys_shouldMergeBuildArgs() {
   }
 
   @Test
-  @DisplayName("build args in image config and system properties with same key, should throw exception")
-  void fromBuildConfigurationAndSystemPropertiesWithSameKey_shouldNotMergeBuildArgs() {
+  @DisplayName("build args in image config, system properties, project properties should merge for same keys following the order of precedence")
+  void fromAllSourcesWithSameKeys_shouldMergeBuildArgs_withLoggedWarnings() {
     // Given
     givenBuildArgsFromImageConfiguration("VERSION", "latest");
     System.setProperty("docker.buildArg.VERSION", "1.0.0");
+    projectProperties.setProperty("docker.buildArg.VERSION", "1.1.0");
+    givenBuildArgsFromJKubeConfiguration("VERSION", "1.1.1");
 
-    // When & Then
-    assertThatExceptionOfType(JKubeException.class)
-        .isThrownBy(() -> BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(imageConfiguration, jKubeConfiguration))
-        .withMessage("Multiple Build Args with the same key: VERSION=latest and VERSION=1.0.0");
+    // When
+    Map<String, String> mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(
+        imageConfiguration, jKubeConfiguration, kitLogger);
+
+    // Then
+    assertThat(mergedBuildArgs)
+        .containsEntry("VERSION", "latest");
+    verify(kitLogger, times(1)).warn(
+        "Multiple Build Args with the same key: VERSION=1.1.1 and VERSION=1.1.0, overriding value of key to VERSION=1.1.0");
+    verify(kitLogger, times(1)).warn(
+        "Multiple Build Args with the same key: VERSION=1.1.0 and VERSION=1.0.0, overriding value of key to VERSION=1.0.0");
+    verify(kitLogger, times(1)).warn(
+        "Multiple Build Args with the same key: VERSION=1.0.0 and VERSION=latest, overriding value of key to VERSION=latest");
   }
 
   @Test
-  @DisplayName("build args in image config and project properties with same key, should throw exception")
-  void fromBuildConfigurationAndProjectPropertiesWithSameKey_shouldNotMergeBuildArgs() {
+  @DisplayName("build args in image config and system properties with same key, should log a warning and override the value of key in system properties")
+  void fromBuildConfigurationAndSystemPropertiesWithSameKey_shouldMergeBuildArgs() {
+    // Given
+    givenBuildArgsFromImageConfiguration("VERSION", "latest");
+    System.setProperty("docker.buildArg.VERSION", "1.0.0");
+
+    // When
+    Map<String, String> mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(
+        imageConfiguration, jKubeConfiguration, kitLogger);
+
+    // Then
+    assertThat(mergedBuildArgs)
+        .containsEntry("VERSION", "latest");
+    verify(kitLogger, times(1)).warn(
+        "Multiple Build Args with the same key: VERSION=1.0.0 and VERSION=latest, overriding value of key to VERSION=latest");
+  }
+
+  @Test
+  @DisplayName("build args in image config and project properties with same key, should log a warning and override the value of key in project properties")
+  void fromBuildConfigurationAndProjectPropertiesWithSameKey_shouldMergeBuildArgs() {
     // Given
     givenBuildArgsFromImageConfiguration("VERSION", "latest");
     projectProperties.setProperty("docker.buildArg.VERSION", "1.0.0");
 
-    // When & Then
-    assertThatExceptionOfType(JKubeException.class)
-        .isThrownBy(() -> BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(imageConfiguration, jKubeConfiguration))
-        .withMessage("Multiple Build Args with the same key: VERSION=latest and VERSION=1.0.0");
+    // When
+    Map<String, String> mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(
+        imageConfiguration, jKubeConfiguration, kitLogger);
+
+    // Then
+    assertThat(mergedBuildArgs)
+        .containsEntry("VERSION", "latest");
+    verify(kitLogger, times(1)).warn(
+        "Multiple Build Args with the same key: VERSION=1.0.0 and VERSION=latest, overriding value of key to VERSION=latest");
   }
 
   @Test
-  @DisplayName("build args in image config and plugin config with same key, should throw exception")
-  void fromBuildConfigurationAndJKubeConfigurationWithSameKey_shouldNotMergeBuildArgs() {
+  @DisplayName("build args in image config and plugin config with same key, should log a warning and override the value of key in plugin config")
+  void fromBuildConfigurationAndJKubeConfigurationWithSameKey_shouldMergeBuildArgs() {
     // Given
     givenBuildArgsFromImageConfiguration("VERSION", "latest");
     givenBuildArgsFromJKubeConfiguration("VERSION", "1.0.0");
 
-    // When & Then
-    assertThatExceptionOfType(JKubeException.class)
-        .isThrownBy(() -> BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(imageConfiguration, jKubeConfiguration))
-        .withMessage("Multiple Build Args with the same key: VERSION=latest and VERSION=1.0.0");
+    // When
+    Map<String, String> mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(
+        imageConfiguration, jKubeConfiguration, kitLogger);
+
+    // Then
+    assertThat(mergedBuildArgs)
+        .containsEntry("VERSION", "latest");
+    verify(kitLogger, times(1)).warn(
+        "Multiple Build Args with the same key: VERSION=1.0.0 and VERSION=latest, overriding value of key to VERSION=latest");
   }
 
   @Nested
@@ -167,7 +212,8 @@ void setUp() throws IOException {
     @DisplayName("mergeBuildArgsIncludingLocalDockerConfigProxySettings, should add proxy build args for docker build strategy")
     void shouldAddBuildArgsFromDockerConfigInDockerBuild() {
       // When
-      final Map<String, String> mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(imageConfiguration, jKubeConfiguration);
+      final Map<String, String> mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(
+          imageConfiguration, jKubeConfiguration, kitLogger);
       // Then
       assertThat(mergedBuildArgs)
           .containsEntry("docker.buildArg.http_proxy", "http://proxy.example.com:3128")
@@ -179,7 +225,8 @@ void shouldAddBuildArgsFromDockerConfigInDockerBuild() {
     @DisplayName("mergeBuildArgsWithoutIncludingLocalDockerConfigProxySettings, should not add proxy build args for OpenShift build strategy")
     void shouldNotAddBuildArgsFromDockerConfig() {
       // When
-      final Map<String, String> mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgsWithoutLocalDockerConfigProxySettings(imageConfiguration, jKubeConfiguration);
+      final Map<String, String> mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgsWithoutLocalDockerConfigProxySettings(
+          imageConfiguration, jKubeConfiguration, kitLogger);
       // Then
       assertThat(mergedBuildArgs)
           .doesNotContainEntry("docker.buildArg.http_proxy", "http://proxy.example.com:3128")

diff --git a/...service/docker/src/main/java/org/eclipse/jkube/kit/build/service/docker/BuildService.java b/...service/docker/src/main/java/org/eclipse/jkube/kit/build/service/docker/BuildService.java
@@ -64,7 +64,7 @@ public class BuildService {
     public void buildImage(ImageConfiguration imageConfig, ImagePullManager imagePullManager, JKubeConfiguration configuration)
             throws IOException {
 
-        Map<String, String> mergedBuildArgs = mergeBuildArgsIncludingLocalDockerConfigProxySettings(imageConfig, configuration);
+        Map<String, String> mergedBuildArgs = mergeBuildArgsIncludingLocalDockerConfigProxySettings(imageConfig, configuration,this.log);
 
         if (imagePullManager != null) {
             autoPullBaseImage(imageConfig, imagePullManager, configuration, mergedBuildArgs);

diff --git a/.../main/java/org/eclipse/jkube/kit/config/service/openshift/OpenShiftBuildServiceUtils.java b/.../main/java/org/eclipse/jkube/kit/config/service/openshift/OpenShiftBuildServiceUtils.java
@@ -26,6 +26,7 @@
 import io.fabric8.openshift.api.model.ImageStreamTagBuilder;
 import org.apache.commons.lang3.StringUtils;
 import org.eclipse.jkube.kit.build.api.assembly.ArchiverCustomizer;
+import org.eclipse.jkube.kit.common.KitLogger;
 import org.eclipse.jkube.kit.common.util.IoUtil;
 import org.eclipse.jkube.kit.common.util.KubernetesHelper;
 import org.eclipse.jkube.kit.config.image.ImageConfiguration;
@@ -110,7 +111,7 @@ private static ArchiverCustomizer createS2IArchiveCustomizer(
   }
 
   protected static BuildStrategy createBuildStrategy(
-      JKubeServiceHub jKubeServiceHub, ImageConfiguration imageConfig, String openshiftPullSecret) {
+      JKubeServiceHub jKubeServiceHub, ImageConfiguration imageConfig, String openshiftPullSecret, KitLogger logger) {
     final BuildServiceConfig config = jKubeServiceHub.getBuildServiceConfig();
     final JKubeBuildStrategy osBuildStrategy = config.getJKubeBuildStrategy();
     final BuildConfiguration buildConfig = imageConfig.getBuildConfiguration();
@@ -134,7 +135,7 @@ protected static BuildStrategy createBuildStrategy(
               .withNamespace(StringUtils.isEmpty(fromNamespace) ? null : fromNamespace)
             .endFrom()
             .withEnv(checkForEnv(imageConfig))
-            .withBuildArgs(mergeBuildArgsWithoutLocalDockerConfigProxySettings(imageConfig, jKubeServiceHub.getConfiguration())
+            .withBuildArgs(mergeBuildArgsWithoutLocalDockerConfigProxySettings(imageConfig, jKubeServiceHub.getConfiguration(),logger)
                 .entrySet()
                 .stream()
                 .map(bcArg -> new EnvVarBuilder()

diff --git a/...e/src/main/java/org/eclipse/jkube/kit/config/service/openshift/OpenshiftBuildService.java b/...e/src/main/java/org/eclipse/jkube/kit/config/service/openshift/OpenshiftBuildService.java
@@ -203,7 +203,7 @@ protected String updateOrCreateBuildConfig(BuildServiceConfig config, OpenShiftC
         ImageName imageName = new ImageName(imageConfig.getName());
         String buildName = computeS2IBuildName(imageConfig, config, imageName);
 
-        BuildStrategy buildStrategyResource = createBuildStrategy(jKubeServiceHub, imageConfig, openshiftPullSecret);
+        BuildStrategy buildStrategyResource = createBuildStrategy(jKubeServiceHub, imageConfig, openshiftPullSecret, this.log);
         BuildOutput buildOutput = createBuildOutput(imageConfig, imageName);
 
         // Fetch existing build config