Skip to content

Commit

Permalink
review: build args helper
Browse files Browse the repository at this point in the history 
Simplify logic to allow for easier future developments and improvements.

Signed-off-by: Marc Nuri <[email protected]>
  • Loading branch information
@manusa
manusa committed Apr 30, 2024
1 parent 315ad28 commit 65db45e
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 112 deletions.
44 changes: 27 additions & 17 deletions .../build/api/src/main/java/org/eclipse/jkube/kit/build/api/helper/BuildArgResolverUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,47 +15,57 @@

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.config.image.ImageConfiguration;

import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import java.util.Objects;
import java.util.Properties;

import static org.eclipse.jkube.kit.common.util.MapUtil.mergeMapsImmutable;
import java.util.stream.Stream;

public class BuildArgResolverUtil {

private static final String ARG_PREFIX = "docker.buildArg.";

private BuildArgResolverUtil() { }

/**
* Merges Docker Build Args in the following order (in decreasing order of precedence):
* 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>
* <li>Docker Proxy Build Args detected from ~/.docker/config.json</li>
* </ul>
* @param imageConfig ImageConfiguration for which Build Args would be resolved
* @param configuration {@link JKubeConfiguration} JKubeConfiguration
* @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> mergeBuildArgs(ImageConfiguration imageConfig, JKubeConfiguration configuration) {
Map<String, String> buildArgsFromProjectProperties = addBuildArgsFromProperties(configuration.getProject().getProperties());
Map<String, String> buildArgsFromSystemProperties = addBuildArgsFromProperties(System.getProperties());
Map<String, String> buildArgsFromDockerConfig = addBuildArgsFromDockerConfig();

return mergeMapsImmutable(imageConfig.getBuild().getArgs(),
buildArgsFromSystemProperties,
buildArgsFromProjectProperties,
Optional.ofNullable(configuration.getBuildArgs()).orElse(Collections.emptyMap()),
buildArgsFromDockerConfig);
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());
});
return buildArgs;
}

private static Map<String, String> addBuildArgsFromProperties(Properties properties) {
private static Map<String, String> buildArgsFromProperties(Properties properties) {
Map<String, String> buildArgs = new HashMap<>();
for (Object keyObj : properties.keySet()) {
String key = (String) keyObj;
Expand All @@ -71,7 +81,7 @@ private static Map<String, String> addBuildArgsFromProperties(Properties propert
return buildArgs;
}

private static Map<String, String> addBuildArgsFromDockerConfig() {
private static Map<String, String> buildArgsFromDockerConfig() {
final Map<String, Object> dockerConfig = DockerFileUtil.readDockerConfig();
if (dockerConfig == null) {
return Collections.emptyMap();
Expand Down
15 changes: 8 additions & 7 deletions ...t/java/org/eclipse/jkube/kit/build/api/helper/BuildArgResolverUtilMergeBuildArgsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
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.util.EnvUtil;
import org.eclipse.jkube.kit.config.image.ImageConfiguration;
Expand All @@ -34,7 +35,7 @@
import java.util.Properties;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;

class BuildArgResolverUtilMergeBuildArgsTest {
private ImageConfiguration imageConfiguration;
Expand Down Expand Up @@ -111,9 +112,9 @@ void fromBuildConfigurationAndSystemPropertiesWithSameKey_shouldNotMergeBuildArg
System.setProperty("docker.buildArg.VERSION", "1.0.0");

// When & Then
assertThatIllegalArgumentException()
assertThatExceptionOfType(JKubeException.class)
.isThrownBy(() -> BuildArgResolverUtil.mergeBuildArgs(imageConfiguration, jKubeConfiguration))
.withMessage("Multiple entries with same key: VERSION=latest and VERSION=1.0.0");
.withMessage("Multiple Build Args with the same key: VERSION=latest and VERSION=1.0.0");
}

@Test
Expand All @@ -124,9 +125,9 @@ void fromBuildConfigurationAndProjectPropertiesWithSameKey_shouldNotMergeBuildAr
projectProperties.setProperty("docker.buildArg.VERSION", "1.0.0");

// When & Then
assertThatIllegalArgumentException()
assertThatExceptionOfType(JKubeException.class)
.isThrownBy(() -> BuildArgResolverUtil.mergeBuildArgs(imageConfiguration, jKubeConfiguration))
.withMessage("Multiple entries with same key: VERSION=latest and VERSION=1.0.0");
.withMessage("Multiple Build Args with the same key: VERSION=latest and VERSION=1.0.0");
}

@Test
Expand All @@ -137,9 +138,9 @@ void fromBuildConfigurationAndJKubeConfigurationWithSameKey_shouldNotMergeBuildA
givenBuildArgsFromJKubeConfiguration("VERSION", "1.0.0");

// When & Then
assertThatIllegalArgumentException()
assertThatExceptionOfType(JKubeException.class)
.isThrownBy(() -> BuildArgResolverUtil.mergeBuildArgs(imageConfiguration, jKubeConfiguration))
.withMessage("Multiple entries with same key: VERSION=latest and VERSION=1.0.0");
.withMessage("Multiple Build Args with the same key: VERSION=latest and VERSION=1.0.0");
}

@Test
Expand Down
53 changes: 28 additions & 25 deletions ...ice/docker/src/test/java/org/eclipse/jkube/kit/build/service/docker/BuildServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,11 @@
import org.eclipse.jkube.kit.build.service.docker.access.DockerAccess;
import org.eclipse.jkube.kit.build.service.docker.access.DockerAccessException;
import org.eclipse.jkube.kit.common.JKubeConfiguration;
import org.eclipse.jkube.kit.common.JavaProject;
import org.eclipse.jkube.kit.common.KitLogger;
import org.eclipse.jkube.kit.config.image.ImageConfiguration;
import org.eclipse.jkube.kit.config.image.build.BuildConfiguration;
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;

Expand All @@ -32,12 +30,8 @@
import java.io.OutputStream;
import java.nio.file.Files;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.Properties;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
Expand All @@ -48,23 +42,39 @@
import static org.mockito.Mockito.when;

class BuildServiceTest {

@TempDir
private File tempDir;
private DockerAccess mockedDockerAccess;
private BuildService buildService;
private ImageConfiguration imageConfiguration;
private ImagePullManager mockedImagePullManager;
private JKubeConfiguration mockedJKubeConfiguration;
private Properties projectProperties;
private JKubeConfiguration jKubeConfiguration;
private RegistryService mockedRegistryService;

@BeforeEach
void setUp() {
mockedDockerAccess = mock(DockerAccess.class, RETURNS_DEEP_STUBS);
ArchiveService mockedArchiveService = mock(ArchiveService.class, RETURNS_DEEP_STUBS);
mockedRegistryService = mock(RegistryService.class, RETURNS_DEEP_STUBS);
KitLogger mockedLog = mock(KitLogger.SilentLogger.class, RETURNS_DEEP_STUBS);
mockedImagePullManager = mock(ImagePullManager.class, RETURNS_DEEP_STUBS);
mockedJKubeConfiguration = mock(JKubeConfiguration.class, RETURNS_DEEP_STUBS);
projectProperties = new Properties();
jKubeConfiguration = JKubeConfiguration.builder()
.project(JavaProject.builder()
.properties(projectProperties)
.baseDirectory(tempDir)
.build())
.sourceDirectory(tempDir.getAbsolutePath())
.build();
QueryService mockedQueryService = new QueryService(mockedDockerAccess);
buildService = new BuildService(mockedDockerAccess, mockedQueryService, mockedRegistryService, mockedArchiveService, mockedLog);
buildService = new BuildService(
mockedDockerAccess,
mockedQueryService,
mockedRegistryService,
mockedArchiveService,
new KitLogger.SilentLogger()
);
imageConfiguration = ImageConfiguration.builder()
.name("image-name")
.build(BuildConfiguration.builder()
Expand All @@ -80,7 +90,7 @@ void buildImage_whenValidImageConfigurationProvidedAndDockerDaemonReturnsValidId
when(mockedDockerAccess.getImageId("image-name")).thenReturn("c8003cb6f5db");

// When
buildService.buildImage(imageConfiguration, mockedImagePullManager, mockedJKubeConfiguration);
buildService.buildImage(imageConfiguration, mockedImagePullManager, jKubeConfiguration);

// Then
verify(mockedDockerAccess, times(1))
Expand All @@ -93,7 +103,7 @@ void buildImage_whenValidImageConfigurationProvidedAndDockerDaemonReturnsNull_sh
when(mockedDockerAccess.getImageId("image-name")).thenReturn(null);
// When & Then
assertThatIllegalStateException()
.isThrownBy(() -> buildService.buildImage(imageConfiguration, mockedImagePullManager, mockedJKubeConfiguration))
.isThrownBy(() -> buildService.buildImage(imageConfiguration, mockedImagePullManager, jKubeConfiguration))
.withMessage("Failure in building image, unable to find image built with name image-name");
}

Expand All @@ -111,8 +121,6 @@ void tagImage_whenValidImageConfigurationProvided_shouldTagImage() throws Docker
void buildImage_whenMultiStageDockerfileWithBuildArgs_shouldPrepullImages() throws IOException {
// Given
when(mockedDockerAccess.getImageId("image-name")).thenReturn("c8003cb6f5db");
when(mockedJKubeConfiguration.getSourceDirectory()).thenReturn(tempDir.getAbsolutePath());
when(mockedJKubeConfiguration.getProject().getBaseDirectory()).thenReturn(tempDir);
File multistageDockerfile = copyToTempDir("Dockerfile_multi_stage_with_args_no_default");
imageConfiguration = ImageConfiguration.builder()
.name("image-name")
Expand All @@ -122,15 +130,13 @@ void buildImage_whenMultiStageDockerfileWithBuildArgs_shouldPrepullImages() thro
.build())
.build();

Properties props = new Properties();
props.setProperty("docker.buildArg.VERSION", "latest");
props.setProperty("docker.buildArg.FULL_IMAGE", "busybox:latest");
props.setProperty("docker.buildArg.REPO_1", "docker.io/library");
props.setProperty("docker.buildArg.IMAGE-1", "openjdk");
when(mockedJKubeConfiguration.getProject().getProperties()).thenReturn(props);
projectProperties.setProperty("docker.buildArg.VERSION", "latest");
projectProperties.setProperty("docker.buildArg.FULL_IMAGE", "busybox:latest");
projectProperties.setProperty("docker.buildArg.REPO_1", "docker.io/library");
projectProperties.setProperty("docker.buildArg.IMAGE-1", "openjdk");

// When
buildService.buildImage(imageConfiguration, mockedImagePullManager, mockedJKubeConfiguration);
buildService.buildImage(imageConfiguration, mockedImagePullManager, jKubeConfiguration);

// Then
verify(mockedRegistryService, times(1)).pullImageWithPolicy(eq("fabric8/s2i-java:latest"), any(), any(), any());
Expand All @@ -139,9 +145,6 @@ void buildImage_whenMultiStageDockerfileWithBuildArgs_shouldPrepullImages() thro
any());
}

@TempDir
private File tempDir;

private File copyToTempDir(String resource) throws IOException {
File ret = new File(tempDir, "Dockerfile");
try (OutputStream os = Files.newOutputStream(ret.toPath())) {
Expand Down
26 changes: 0 additions & 26 deletions jkube-kit/common/src/main/java/org/eclipse/jkube/kit/common/util/MapUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,32 +42,6 @@ public static void mergeIfAbsent(Map<String, String> map, Map<String, String> to
}
}

/**
* Returns a new map with all the entries of first map with rest map entries. It throws IllegalArgumentException
* when it encounters conflicting keys
*
* @param maps var arg parameter of maps to be merged
* @return merged map
* @param <K> key type
* @param <V> value type
* @throws IllegalArgumentException when multiple entries with same key are found
*/
@SafeVarargs
public static <K,V> Map<K,V> mergeMapsImmutable(Map<K, V>... maps) {
Map<K, V> answer = new HashMap<>();
for (int i = maps.length-1; i >= 0; i--) {
if (maps[i] != null) {
for (Map.Entry<K, V> e : maps[i].entrySet()) {
if (answer.containsKey(e.getKey())) {
throw new IllegalArgumentException(String.format("Multiple entries with same key: %s=%s and %s=%s", e.getKey(), e.getValue(), e.getKey(), answer.get(e.getKey())));
}
answer.put(e.getKey(), e.getValue());
}
}
}
return answer;
}

/**
* Returns a new map with all the entries of first map with rest map entries which don't override map1.
*
Expand Down
37 changes: 0 additions & 37 deletions jkube-kit/common/src/test/java/org/eclipse/jkube/kit/common/util/MapUtilTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,12 @@
import java.util.Map;

import org.assertj.core.api.InstanceOfAssertFactories;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
import static org.assertj.core.api.Assertions.entry;
import static org.eclipse.jkube.kit.common.util.MapUtil.getFlattenedMap;
import static org.eclipse.jkube.kit.common.util.MapUtil.getNestedMap;
import static org.eclipse.jkube.kit.common.util.MapUtil.mergeMapsImmutable;
import static org.junit.jupiter.api.Assertions.assertThrows;

/**
Expand Down Expand Up @@ -140,39 +136,6 @@ void getNestedMap_withInvalidFlattenedMap_shouldThrowNodeOverlapsException() {
assertThat(result).hasMessage("The provided input Map is invalid (node <second> overlaps with key)");
}

@Nested
class MergeMapsImmutable {
@Test
@DisplayName("different keys present in maps, then merge maps")
void whenNoConflictingKeysProvided_thenMergeMaps() {
// Given
Map<String, String> m1 = createMap("foo", "bar");
Map<String, String> m2 = createMap("BAR", "FOO");

// When
Map<String, String> result = mergeMapsImmutable(m1, m2);

// Then
assertThat(result)
.containsEntry("foo", "bar")
.containsEntry("BAR", "FOO");
}

@Test
@DisplayName("same keys present in maps, then throw exception")
void whenConflictingKeysProvided_thenThrowException() {
// Given
Map<String, String> m1 = createMap("foo", "bar");
Map<String, String> m2 = createMap("foo", "FOO");

// When + Then
assertThatIllegalArgumentException()
.isThrownBy(() -> MapUtil.mergeMapsImmutable(m1, m2, null))
.withMessage("Multiple entries with same key: foo=bar and foo=FOO");
}
}


private Map<String, String> createMap(String ... args) {
Map<String, String> ret = new LinkedHashMap<>();
for (int i = 0; i < args.length; i+=2) {
Expand Down

0 comments on commit 65db45e

Please sign in to comment.