fix (jkube-kit/config) : docker.buildArg.* properties not taken int…

…o account in OpenShift plugins Added call to resolve build args from all sources in OpenShiftBuildServiceUtils so that build args get resolved from all the available sources (compared to just ImageConfig) Signed-off-by: Rohan Kumar <[email protected]>
eclipse-jkube · Apr 30, 2024 · 02a912d · 02a912d
1 parent 65db45e
commit 02a912d
Show file tree

Hide file tree

Showing 4 changed files with 141 additions and 75 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -29,6 +29,7 @@ Usage:
 * Fix #2860: Correctly pass Docker build-arg from the build configuration to the Openshift build strategy
 * Fix #2885: Provide a way to set labels on images defined by Generators
 * Fix #2901: Ensure Docker build arguments from properties are used during images pre-pulling
+* Fix #2904: `docker.buildArg.*` properties not taken into account in OpenShift plugins
 
 ### 1.16.2 (2024-03-27)
 * Fix #2461: `k8s:watch`/`k8sWatch` should throw error in `buildpacks` build strategy

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
@@ -45,23 +45,45 @@ private BuildArgResolverUtil() { }
    * @return a Map containing merged Build Args from all sources.
    */
   public static Map<String, String> mergeBuildArgs(ImageConfiguration imageConfig, JKubeConfiguration configuration) {
+    return mergeBuildArgsFrom(imageConfig.getBuild().getArgs(),
+            buildArgsFromProperties(System.getProperties()),
+            buildArgsFromProperties(configuration.getProject().getProperties()),
+            configuration.getBuildArgs(),
+            buildArgsFromDockerConfig());
+  }
+
+  /**
+   * Merges Docker Build Args from the following sources:
+   * <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>
+   * @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> mergeBuildArgsOpenShift(ImageConfiguration imageConfig, JKubeConfiguration configuration) {
+    return mergeBuildArgsFrom(imageConfig.getBuild().getArgs(),
+        buildArgsFromProperties(System.getProperties()),
+        buildArgsFromProperties(configuration.getProject().getProperties()),
+        configuration.getBuildArgs());
+  }
+
+  @SafeVarargs
+  private static Map<String, String> mergeBuildArgsFrom(Map<String, String>... buildArgSources) {
     final Map<String, String> buildArgs = new HashMap<>();
-    Stream.of(
-      imageConfig.getBuild().getArgs(),
-      buildArgsFromProperties(System.getProperties()),
-      buildArgsFromProperties(configuration.getProject().getProperties()),
-      configuration.getBuildArgs(),
-      buildArgsFromDockerConfig()
-    )
-      .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()));
-        }
-        buildArgs.put(entry.getKey(), entry.getValue());
-      });
+    Stream.of(buildArgSources)
+        .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()));
+          }
+          buildArgs.put(entry.getKey(), entry.getValue());
+        });
     return buildArgs;
   }
 

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
@@ -47,6 +47,7 @@
 import java.util.Optional;
 import java.util.stream.Collectors;
 
+import static org.eclipse.jkube.kit.build.api.helper.BuildArgResolverUtil.mergeBuildArgsOpenShift;
 import static org.eclipse.jkube.kit.build.api.helper.BuildUtil.extractBaseFromDockerfile;
 import static org.eclipse.jkube.kit.config.service.openshift.ImageStreamService.resolveImageStreamName;
 import static org.eclipse.jkube.kit.config.service.openshift.OpenshiftBuildService.DEFAULT_BUILD_OUTPUT_KIND;
@@ -133,7 +134,9 @@ protected static BuildStrategy createBuildStrategy(
               .withNamespace(StringUtils.isEmpty(fromNamespace) ? null : fromNamespace)
             .endFrom()
             .withEnv(checkForEnv(imageConfig))
-            .withBuildArgs(Optional.ofNullable(buildConfig.getArgs()).orElse(Collections.emptyMap()).entrySet().stream()
+            .withBuildArgs(mergeBuildArgsOpenShift(imageConfig, jKubeServiceHub.getConfiguration())
+                .entrySet()
+                .stream()
                 .map(bcArg -> new EnvVarBuilder()
                     .withName(bcArg.getKey())
                     .withValue(bcArg.getValue()).build())

diff --git a/...t/java/org/eclipse/jkube/kit/config/service/openshift/OpenShiftBuildServiceUtilsTest.java b/...t/java/org/eclipse/jkube/kit/config/service/openshift/OpenShiftBuildServiceUtilsTest.java
@@ -18,12 +18,15 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.Properties;
 
 import io.fabric8.openshift.api.model.ImageStreamTag;
 import io.fabric8.openshift.api.model.ImageStreamTagBuilder;
+import org.assertj.core.api.InstanceOfAssertFactories;
 import org.eclipse.jkube.kit.build.api.assembly.ArchiverCustomizer;
 import org.eclipse.jkube.kit.build.api.assembly.JKubeBuildTarArchiver;
 import org.eclipse.jkube.kit.common.JKubeConfiguration;
+import org.eclipse.jkube.kit.common.JavaProject;
 import org.eclipse.jkube.kit.config.image.ImageConfiguration;
 import org.eclipse.jkube.kit.config.image.ImageName;
 import org.eclipse.jkube.kit.config.image.build.BuildConfiguration;
@@ -40,6 +43,8 @@
 import io.fabric8.openshift.api.model.BuildStrategy;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
 import org.mockito.ArgumentCaptor;
@@ -75,6 +80,10 @@ void setUp() {
     jKubeServiceHub = mock(JKubeServiceHub.class, RETURNS_DEEP_STUBS);
     when(jKubeServiceHub.getBuildServiceConfig().getBuildDirectory())
         .thenReturn(temporaryFolder.getAbsolutePath());
+    when(jKubeServiceHub.getConfiguration()).thenReturn(JKubeConfiguration.builder()
+            .project(JavaProject.builder().build())
+            .buildArgs(Collections.emptyMap())
+        .build());
     imageConfiguration = ImageConfiguration.builder()
         .name("myapp")
         .build(BuildConfiguration.builder()
@@ -183,64 +192,95 @@ void createBuildStrategy_withS2iBuildStrategyAndPullSecret_shouldReturnValidBuil
         .hasFieldOrPropertyWithValue("pullSecret.name", "my-secret-for-pull");
   }
 
-  @Test
-  void createBuildStrategy_withDockerBuildStrategyAndNoPullSecret_shouldReturnValidBuildStrategy() {
-    // Given
-    when(jKubeServiceHub.getBuildServiceConfig().getJKubeBuildStrategy())
-        .thenReturn(JKubeBuildStrategy.docker);
-    // When
-    final BuildStrategy result = createBuildStrategy(jKubeServiceHub, imageConfiguration, null);
-    // Then
-    assertThat(result)
-        .hasFieldOrPropertyWithValue("type", "Docker")
-        .extracting(BuildStrategy::getDockerStrategy)
-        .hasFieldOrPropertyWithValue("from.kind", "DockerImage")
-        .hasFieldOrPropertyWithValue("from.name", "ubi8/s2i-base");
-  }
-
-  @Test
-  void createBuildStrategy_withDockerBuildArgs_shouldReturnValidBuildStrategyWithBuildArgs() {
-    // Given
-    when(jKubeServiceHub.getBuildServiceConfig().getJKubeBuildStrategy())
-        .thenReturn(JKubeBuildStrategy.docker);
-    ImageConfiguration imageConfig = ImageConfiguration.builder()
-        .name("myapp")
-        .build(BuildConfiguration.builder()
-            .from("ubi8/s2i-base")
-            .args(Collections.singletonMap("BUILD_ARGS_KEY", "build-args-value"))
-            .build())
-        .build();
-    // When
-    final BuildStrategy result = createBuildStrategy(jKubeServiceHub, imageConfig, "my-secret-for-pull");
-    // Then
-    assertThat(result)
-        .hasFieldOrPropertyWithValue("type", "Docker")
-        .extracting(BuildStrategy::getDockerStrategy)
-        .hasFieldOrPropertyWithValue("from.kind", "DockerImage")
-        .hasFieldOrPropertyWithValue("from.name", "ubi8/s2i-base")
-        .extracting("buildArgs").asList().hasSize(1).first().satisfies(
-            envVar -> {
-              assertThat(envVar)
-                  .hasFieldOrPropertyWithValue("name", "BUILD_ARGS_KEY")
-                  .hasFieldOrPropertyWithValue("value", "build-args-value");
-            });
-  }
-
-  @Test
-  void createBuildStrategy_withDockerBuildStrategyAndPullSecret_shouldReturnValidBuildStrategy() {
-    // Given
-    when(jKubeServiceHub.getBuildServiceConfig().getJKubeBuildStrategy())
-        .thenReturn(JKubeBuildStrategy.docker);
-    // When
-    final BuildStrategy result = createBuildStrategy(jKubeServiceHub, imageConfiguration, "my-secret-for-pull");
-    // Then
-    assertThat(result)
-        .hasFieldOrPropertyWithValue("type", "Docker")
-        .extracting(BuildStrategy::getDockerStrategy)
-        .hasFieldOrPropertyWithValue("from.kind", "DockerImage")
-        .hasFieldOrPropertyWithValue("from.name", "ubi8/s2i-base")
-        .hasFieldOrPropertyWithValue("noCache", false)
-        .hasFieldOrPropertyWithValue("pullSecret.name", "my-secret-for-pull");
+  @Nested
+  @DisplayName("docker BuildStrategy")
+  class DockerBuildStrategy {
+    @BeforeEach
+    void setUp() {
+      when(jKubeServiceHub.getBuildServiceConfig().getJKubeBuildStrategy())
+          .thenReturn(JKubeBuildStrategy.docker);
+    }
+
+    @Test
+    @DisplayName("no pull secret specified")
+    void withDockerBuildStrategyAndNoPullSecret_shouldReturnValidBuildStrategy() {
+      // When
+      final BuildStrategy result = createBuildStrategy(jKubeServiceHub, imageConfiguration, null);
+      // Then
+      assertThat(result)
+          .hasFieldOrPropertyWithValue("type", "Docker")
+          .extracting(BuildStrategy::getDockerStrategy)
+          .hasFieldOrPropertyWithValue("from.kind", "DockerImage")
+          .hasFieldOrPropertyWithValue("from.name", "ubi8/s2i-base");
+    }
+
+    @Test
+    @DisplayName("pull secret specified, should reflect in Build Strategy")
+    void withPullSecret_shouldReturnValidBuildStrategy() {
+      // When
+      final BuildStrategy result = createBuildStrategy(jKubeServiceHub, imageConfiguration, "my-secret-for-pull");
+      // Then
+      assertThat(result)
+          .hasFieldOrPropertyWithValue("type", "Docker")
+          .extracting(BuildStrategy::getDockerStrategy)
+          .hasFieldOrPropertyWithValue("from.kind", "DockerImage")
+          .hasFieldOrPropertyWithValue("from.name", "ubi8/s2i-base")
+          .hasFieldOrPropertyWithValue("noCache", false)
+          .hasFieldOrPropertyWithValue("pullSecret.name", "my-secret-for-pull");
+    }
+
+    @Nested
+    @DisplayName("with build args")
+    class WithDockerBuildArgs {
+      @Test
+      @DisplayName("when docker build args in ImageConfig, then add them to BuildStrategy")
+      void withDockerBuildArgsInImageConfiguration_shouldReturnValidBuildStrategyWithBuildArgs() {
+        // Given
+        imageConfiguration = imageConfiguration.toBuilder()
+            .build(imageConfiguration.getBuild().toBuilder()
+                .args(Collections.singletonMap("BUILD_ARGS_KEY", "build-args-value"))
+                .build())
+            .build();
+        // When
+        final BuildStrategy result = createBuildStrategy(jKubeServiceHub, imageConfiguration, "my-secret-for-pull");
+        // Then
+        assertBuildArgPresentInBuildStrategy(result);
+      }
+
+      @Test
+      @DisplayName("when docker build args in project properties, then add them to BuildStrategy")
+      void withDockerBuildArgsInProjectProperties_shouldReturnValidBuildStrategyWithBuildArgs() {
+        // Given
+        Properties properties = new Properties();
+        properties.put("docker.buildArg.BUILD_ARGS_KEY", "build-args-value");
+        when(jKubeServiceHub.getConfiguration()).thenReturn(JKubeConfiguration.builder()
+            .project(JavaProject.builder()
+                .properties(properties)
+                .build())
+            .build());
+        // When
+        final BuildStrategy result = createBuildStrategy(jKubeServiceHub, imageConfiguration, "my-secret-for-pull");
+        // Then
+        assertBuildArgPresentInBuildStrategy(result);
+      }
+
+      void assertBuildArgPresentInBuildStrategy(BuildStrategy result) {
+        assertThat(result)
+            .hasFieldOrPropertyWithValue("type", "Docker")
+            .extracting(BuildStrategy::getDockerStrategy)
+            .hasFieldOrPropertyWithValue("from.kind", "DockerImage")
+            .hasFieldOrPropertyWithValue("from.name", "ubi8/s2i-base")
+            .extracting("buildArgs")
+            .asInstanceOf(InstanceOfAssertFactories.LIST)
+            .singleElement()
+            .satisfies(
+                envVar -> {
+                  assertThat(envVar)
+                      .hasFieldOrPropertyWithValue("name", "BUILD_ARGS_KEY")
+                      .hasFieldOrPropertyWithValue("value", "build-args-value");
+                });
+      }
+    }
   }
 
   @Test