Skip to content

Commit

Permalink
review: ResourceConfig refactor
Browse files Browse the repository at this point in the history 
Signed-off-by: Marc Nuri <marc@marcnuri.com>
  • Loading branch information
@manusa
manusa committed Feb 8, 2023
1 parent 0b743bb commit 45727b3
Show file tree
Hide file tree
Showing 7 changed files with 157 additions and 159 deletions.
63 changes: 28 additions & 35 deletions ...t/config/resource/src/main/java/org/eclipse/jkube/kit/config/resource/ResourceConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,52 +114,48 @@ public class ResourceConfig {
private String restartPolicy;
private ControllerResourceConfig controller;

public static ResourceConfigBuilder toBuilder(ResourceConfig original) {
return Optional.ofNullable(original).orElse(new ResourceConfig()).toBuilder();
}

public static ControllerResourceConfig resolveControllerConfig(ResourceConfig config) {
ControllerResourceConfig controller = config.getController();
if (config.getController() != null && config.isAnyControllerLegacyConfigFieldSet()) {
public final ControllerResourceConfig getController() {
if (controller != null && isAnyControllerLegacyConfigFieldSet()) {
throw new IllegalArgumentException("Can't use both controller and resource level controller configuration fields." +
"Please migrate to controller configuration");
}
if (controller == null) {
controller = createNewControllerConfigWithLegacyMerged(config);
" Please migrate to controller configuration");
}
return controller;
return controller == null ? controllerResourceConfigFromLegacyFields() : controller;
}

private static ControllerResourceConfig createNewControllerConfigWithLegacyMerged(ResourceConfig config) {
public static ResourceConfigBuilder toBuilder(ResourceConfig original) {
return Optional.ofNullable(original).orElse(new ResourceConfig()).toBuilder();
}

private ControllerResourceConfig controllerResourceConfigFromLegacyFields() {
ControllerResourceConfig.ControllerResourceConfigBuilder builder = ControllerResourceConfig.builder();
if (config.env != null && !config.env.isEmpty()) {
builder.env(config.env);
if (env != null && !env.isEmpty()) {
builder.env(env);
}
if (config.volumes != null) {
builder.volumes(config.volumes);
if (volumes != null) {
builder.volumes(volumes);
}
if (StringUtils.isNotBlank(config.controllerName)) {
builder.controllerName(config.controllerName);
if (StringUtils.isNotBlank(controllerName)) {
builder.controllerName(controllerName);
}
if (config.liveness != null) {
builder.liveness(config.liveness);
if (liveness != null) {
builder.liveness(liveness);
}
if (config.readiness != null) {
builder.readiness(config.readiness);
if (readiness != null) {
builder.readiness(readiness);
}
if (config.startup != null) {
builder.startup(config.startup);
if (startup != null) {
builder.startup(startup);
}
if (config.imagePullPolicy != null) {
builder.imagePullPolicy(config.imagePullPolicy);
if (imagePullPolicy != null) {
builder.imagePullPolicy(imagePullPolicy);
}
if (config.replicas != null) {
builder.replicas(config.replicas);
if (replicas != null) {
builder.replicas(replicas);
}
if (StringUtils.isNotBlank(config.restartPolicy)) {
builder.restartPolicy(config.restartPolicy);
if (StringUtils.isNotBlank(restartPolicy)) {
builder.restartPolicy(restartPolicy);
}
builder.containerPrivileged(config.containerPrivileged);
builder.containerPrivileged(containerPrivileged);
return builder.build();
}

Expand All @@ -176,9 +172,6 @@ public boolean isAnyControllerLegacyConfigFieldSet() {
(containerPrivileged);
}


// TODO: SCC

// ===============================
// TODO:
// jkube.extended.environment.metadata
Expand Down
11 changes: 5 additions & 6 deletions ...nfig/resource/src/test/java/org/eclipse/jkube/kit/config/resource/ResourceConfigTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
import static org.eclipse.jkube.kit.config.resource.ResourceConfig.resolveControllerConfig;

@SuppressWarnings("deprecation")
class ResourceConfigTest {
@Test
void resolveControllerConfig_whenInvoked_shouldMergeLegacyOptionsWithController() {
Expand All @@ -42,7 +42,7 @@ void resolveControllerConfig_whenInvoked_shouldMergeLegacyOptionsWithController(
.build();

// When
ControllerResourceConfig controllerResourceConfig = resolveControllerConfig(resourceConfig);
ControllerResourceConfig controllerResourceConfig = resourceConfig.getController();

// Then
assertThat(controllerResourceConfig)
Expand Down Expand Up @@ -70,7 +70,7 @@ void resolveControllerConfig_whenInvoked_shouldMergeSingleControllerConfig() {
.build();

// When
ControllerResourceConfig controllerConfigList = resolveControllerConfig(resourceConfig);
ControllerResourceConfig controllerConfigList = resourceConfig.getController();

// Then
assertThat(controllerConfigList)
Expand All @@ -92,9 +92,8 @@ void resolveControllerConfig_whenInvokedWithBothResourceAndControllerConfig_shou

// When + Then
assertThatIllegalArgumentException()
.isThrownBy(() -> resolveControllerConfig(resourceConfig))
.withMessage("Can't use both controller and resource level controller configuration fields." +
"Please migrate to controller configuration");
.isThrownBy(resourceConfig::getController)
.withMessage("Can't use both controller and resource level controller configuration fields. Please migrate to controller configuration");
}

@Test
Expand Down
44 changes: 19 additions & 25 deletions jkube-kit/enricher/api/src/main/java/org/eclipse/jkube/kit/enricher/api/BaseEnricher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,6 @@
import java.util.Optional;
import java.util.Properties;

import static org.eclipse.jkube.kit.config.resource.ResourceConfig.resolveControllerConfig;

/**
* @author roland
*/
Expand Down Expand Up @@ -151,22 +149,22 @@ protected Long getOpenshiftDeployTimeoutInSeconds(Long defaultValue) {
/**
* This method overrides the controller name value by the value provided in XML config.
*
* @param controllerResourceConfig Controller Resource config from plugin configuration
* @param defaultValue default value
* @return string as controller name
*/
protected String getControllerName(ControllerResourceConfig controllerResourceConfig, String defaultValue) {
return Optional.ofNullable(controllerResourceConfig).map(ControllerResourceConfig::getControllerName).orElse(defaultValue);
protected String getControllerName(String defaultValue) {
return StringUtils.isNotBlank(getControllerResourceConfig().getControllerName()) ?
getControllerResourceConfig().getControllerName() : defaultValue;
}

protected ControllerResourceConfig getControllerResourceConfig() {
ResourceConfig resourceConfig = getConfiguration().getResource();
if (resourceConfig != null) {
if (resourceConfig.isAnyControllerLegacyConfigFieldSet()) {
log.debug("Controller configuration fields in resource are deprecated." +
"Please use nested field controller for specifying controller configuration");
" Please use nested field controller for specifying controller configuration");
}
return resolveControllerConfig(resourceConfig);
return resourceConfig.getController();
}
return ControllerResourceConfig.builder().build();
}
Expand All @@ -175,17 +173,16 @@ protected ControllerResourceConfig getControllerResourceConfig() {
* This method overrides the ImagePullPolicy value by the value provided in
* XML config.
*
* @param controllerResourceConfig controller resource config from plugin configuration
* @param enricherConfig Enricher specific configuration for ImagePullPolicy
* @return string as image pull policy
*/
protected String getImagePullPolicy(ControllerResourceConfig controllerResourceConfig, Configs.Config enricherConfig) {
protected String getImagePullPolicy(Configs.Config enricherConfig) {
String imagePullPolicyFromProperty = getValueFromConfig(JKUBE_ENFORCED_IMAGE_PULL_POLICY, null);
if (StringUtils.isNotBlank(imagePullPolicyFromProperty)) {
return imagePullPolicyFromProperty;
}
if (controllerResourceConfig != null && StringUtils.isNotBlank(controllerResourceConfig.getImagePullPolicy())) {
return controllerResourceConfig.getImagePullPolicy();
if (StringUtils.isNotBlank(getControllerResourceConfig().getImagePullPolicy())) {
return getControllerResourceConfig().getImagePullPolicy();
}
final String imagePullPolicyFromEnricherConfig = enricherConfig != null ? getConfig(enricherConfig) : null;
if (StringUtils.isNotBlank(imagePullPolicyFromEnricherConfig)) {
Expand All @@ -211,25 +208,22 @@ protected boolean getCreateExternalUrls() {
* topmost priority.
*
* @param builder kubernetes list builder containing objects
* @param controllerResourceConfig xml resource config from plugin configuration
* @param defaultValue default value
* @return resolved replica count
*/
protected static int getReplicaCount(KubernetesListBuilder builder, ControllerResourceConfig controllerResourceConfig, int defaultValue) {
if (controllerResourceConfig != null) {
final List<HasMetadata> items = Optional.ofNullable(builder)
.map(KubernetesListBuilder::buildItems).orElse(Collections.emptyList());
for (HasMetadata item : items) {
if (item instanceof Deployment && ((Deployment)item).getSpec().getReplicas() != null) {
return ((Deployment)item).getSpec().getReplicas();
}
if (item instanceof DeploymentConfig && ((DeploymentConfig)item).getSpec().getReplicas() != null) {
return ((DeploymentConfig)item).getSpec().getReplicas();
}
protected int getReplicaCount(KubernetesListBuilder builder, int defaultValue) {
final List<HasMetadata> items = Optional.ofNullable(builder)
.map(KubernetesListBuilder::buildItems).orElse(Collections.emptyList());
for (HasMetadata item : items) {
if (item instanceof Deployment && ((Deployment)item).getSpec().getReplicas() != null) {
return ((Deployment)item).getSpec().getReplicas();
}
if (item instanceof DeploymentConfig && ((DeploymentConfig)item).getSpec().getReplicas() != null) {
return ((DeploymentConfig)item).getSpec().getReplicas();
}
return controllerResourceConfig.getReplicas() != null ? controllerResourceConfig.getReplicas() : defaultValue;
}
return defaultValue;
return getControllerResourceConfig().getReplicas() != null ?
getControllerResourceConfig().getReplicas() : defaultValue;
}

public static String getNamespace(ResourceConfig resourceConfig, String defaultValue) {
Expand Down
68 changes: 55 additions & 13 deletions ...api/src/test/java/org/eclipse/jkube/kit/enricher/api/BaseEnricherGetReplicaCountTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,13 @@

import io.fabric8.kubernetes.api.model.apps.DeploymentBuilder;
import io.fabric8.openshift.api.model.DeploymentConfigBuilder;
import org.eclipse.jkube.kit.common.JavaProject;
import org.eclipse.jkube.kit.common.KitLogger;
import org.eclipse.jkube.kit.config.resource.ControllerResourceConfig;

import io.fabric8.kubernetes.api.model.ConfigMapBuilder;
import io.fabric8.kubernetes.api.model.KubernetesListBuilder;
import org.eclipse.jkube.kit.config.resource.ResourceConfig;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
Expand All @@ -30,7 +33,6 @@
import java.util.stream.Stream;

import static org.assertj.core.api.Assertions.assertThat;
import static org.eclipse.jkube.kit.enricher.api.BaseEnricher.getReplicaCount;
import static org.junit.jupiter.params.provider.Arguments.arguments;

class BaseEnricherGetReplicaCountTest {
Expand All @@ -43,8 +45,10 @@ class NoListBuilder {
@Test
@DisplayName("no controller resource config, should return default value")
void nullControllerResourceConfig() {
// Given
final BaseEnricher enricher = withEnricher(JKubeEnricherContext.builder());
// When
int result = getReplicaCount(null, null, 42);
int result = enricher.getReplicaCount(null, 42);
// Then
assertThat(result).isEqualTo(42);
}
Expand All @@ -54,9 +58,11 @@ void nullControllerResourceConfig() {
@MethodSource("getReplicaCountsData")
void resourceConfig(Integer replicas, int expectedReplicas) {
// Given
final ControllerResourceConfig resourceConfig = ControllerResourceConfig.builder().replicas(replicas).build();
final ControllerResourceConfig controller = ControllerResourceConfig.builder().replicas(replicas).build();
final BaseEnricher enricher = withEnricher(
JKubeEnricherContext.builder().resources(ResourceConfig.builder().controller(controller).build()));
// When
int result = getReplicaCount(null, resourceConfig, 42);
int result = enricher.getReplicaCount(null, 42);
// Then
assertThat(result).isEqualTo(expectedReplicas);
}
Expand All @@ -69,37 +75,73 @@ Stream<Arguments> getReplicaCountsData() {
}
}


@Test
void withEmptyListBuilderAndEmptyControllerResourceConfig_shouldReturnDefault() {
void withEmptyListBuilderAndEmptyResourceConfig_shouldReturnDefault() {
// Given
final BaseEnricher enricher = withEnricher(
JKubeEnricherContext.builder().resources(ResourceConfig.builder().build()));
// When
int result = getReplicaCount(new KubernetesListBuilder().addToItems(new ConfigMapBuilder()), new ControllerResourceConfig(), 1337);
int result = enricher.getReplicaCount(new KubernetesListBuilder().addToItems(new ConfigMapBuilder()), 1337);
// Then
assertThat(result).isEqualTo(1337);
}

@Test
void withDeploymentConfigInListBuilderAndEmptyControllerResourceConfig_shouldReturnDeploymentConfig() {
void withEmptyListBuilderAndResourceConfig_shouldReturnResourceConfig() {
// Given
final BaseEnricher enricher = withEnricher(
JKubeEnricherContext.builder().resources(ResourceConfig.builder().replicas(42).build()));
// When
int result = enricher.getReplicaCount(new KubernetesListBuilder().addToItems(new ConfigMapBuilder()), 1337);
// Then
assertThat(result).isEqualTo(42);
}

@Test
void withEmptyListBuilderAndResourceConfig_shouldReturnControllerResourceConfig() {
// Given
final BaseEnricher enricher = withEnricher(
JKubeEnricherContext.builder().resources(ResourceConfig.builder()
.controller(ControllerResourceConfig.builder().replicas(42).build()).build()));
// When
int result = enricher.getReplicaCount(new KubernetesListBuilder().addToItems(new ConfigMapBuilder()), 1337);
// Then
assertThat(result).isEqualTo(42);
}

@Test
void withDeploymentConfigInListBuilderAndResourceConfig_shouldReturnDeploymentConfig() {
// Given
final KubernetesListBuilder klb = new KubernetesListBuilder()
.addToItems(new DeploymentConfigBuilder().withNewSpec().withReplicas(1).endSpec());
final ControllerResourceConfig resourceConfig = ControllerResourceConfig.builder().replicas(313373).build();
final ControllerResourceConfig controller = ControllerResourceConfig.builder().replicas(313373).build();
final BaseEnricher enricher = withEnricher(
JKubeEnricherContext.builder().resources(ResourceConfig.builder().controller(controller).build()));
// When
final int result = getReplicaCount(klb, resourceConfig, 1337);
final int result = enricher.getReplicaCount(klb, 1337);
// Then
assertThat(result).isEqualTo(1);
}

@Test
void withDeploymentAndDeploymentConfigInListBuilderAndEmptyControllerResourceConfig_shouldReturnValueInFirstItem() {
void withDeploymentAndDeploymentConfigInListBuilderAndResourceConfig_shouldReturnValueInFirstItem() {
// Given
final KubernetesListBuilder klb = new KubernetesListBuilder()
.addToItems(new DeploymentBuilder().withNewSpec().withReplicas(2).endSpec())
.addToItems(new DeploymentConfigBuilder().withNewSpec().withReplicas(1).endSpec());
final ControllerResourceConfig resourceConfig = ControllerResourceConfig.builder().replicas(313373).build();
final ControllerResourceConfig controller = ControllerResourceConfig.builder().replicas(313373).build();
final BaseEnricher enricher = withEnricher(
JKubeEnricherContext.builder().resources(ResourceConfig.builder().controller(controller).build()));
// When
int result = getReplicaCount(klb, resourceConfig, 1337);
int result = enricher.getReplicaCount(klb, 1337);
// Then
assertThat(result).isEqualTo(2);
}

private static BaseEnricher withEnricher(JKubeEnricherContext.JKubeEnricherContextBuilder contextBuilder) {
return new BaseEnricher(contextBuilder
.log(new KitLogger.SilentLogger())
.project(JavaProject.builder().build())
.build(), "test-enricher");
}
}
Loading

0 comments on commit 45727b3

Please sign in to comment.