diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d7cd9a0f9..59811171cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ Usage: ./scripts/extract-changelog-for-version.sh 1.3.37 5 ``` ### 1.6.0-SNAPSHOT +* Fix #887: Incorrect warning about overriding environment variable ### 1.5.1 (2021-10-28) * Fix #1084: Gradle dependencies should be test or provided scope diff --git a/jkube-kit/enricher/generic/src/main/java/org/eclipse/jkube/enricher/generic/ImageEnricher.java b/jkube-kit/enricher/generic/src/main/java/org/eclipse/jkube/enricher/generic/ImageEnricher.java index ed202f1c7b..3e627a61a1 100644 --- a/jkube-kit/enricher/generic/src/main/java/org/eclipse/jkube/enricher/generic/ImageEnricher.java +++ b/jkube-kit/enricher/generic/src/main/java/org/eclipse/jkube/enricher/generic/ImageEnricher.java @@ -83,7 +83,7 @@ private enum Config implements Configs.Config { PULL_POLICY("pullPolicy"); @Getter - protected String key; + private String key; } @Override @@ -283,30 +283,21 @@ private void mergeEnvVariables(Container container) { .withName(resourceEnvEntry.getKey()) .withValue(resourceEnvEntry.getValue()) .build(); - if (!hasEnvWithName(containerEnvVars, newEnvVar.getName())) { + + EnvVar oldEnvVar = containerEnvVars.stream() + .filter(e -> e.getName().equals(newEnvVar.getName())) + .findFirst().orElse(null); + if (oldEnvVar == null) { containerEnvVars.add(newEnvVar); - } else { + } else if (!newEnvVar.getValue().equals(oldEnvVar.getValue())) { log.warn( - "Environment variable %s will not be overridden: trying to set the value %s, but its actual value is %s", - newEnvVar.getName(), newEnvVar.getValue(), getEnvValue(containerEnvVars, newEnvVar.getName())); + "Environment variable %s will not be overridden: trying to set the value %s, but its actual value is %s", + newEnvVar.getName(), newEnvVar.getValue(), oldEnvVar.getValue()); } } }); } - private String getEnvValue(List envVars, String name) { - for (EnvVar var : envVars) { - if (var.getName().equals(name)) { - return var.getValue(); - } - } - return "(not found)"; - } - - private boolean hasEnvWithName(List envVars, String name) { - return envVars.stream().anyMatch(e -> e.getName().equals(name)); - } - static String containerImageName(ImageConfiguration imageConfiguration) { String prefix = ""; if (StringUtils.isNotBlank(imageConfiguration.getRegistry())) { diff --git a/jkube-kit/enricher/generic/src/test/java/org/eclipse/jkube/enricher/generic/ImageEnricherTest.java b/jkube-kit/enricher/generic/src/test/java/org/eclipse/jkube/enricher/generic/ImageEnricherTest.java index b2c9d6d8e0..bc313bdf13 100644 --- a/jkube-kit/enricher/generic/src/test/java/org/eclipse/jkube/enricher/generic/ImageEnricherTest.java +++ b/jkube-kit/enricher/generic/src/test/java/org/eclipse/jkube/enricher/generic/ImageEnricherTest.java @@ -55,16 +55,9 @@ public class ImageEnricherTest { @Before public void prepareMock() { // Setup mock behaviour + givenResourceConfigWithEnvVar("MY_KEY", "MY_VALUE"); // @formatter:off new Expectations() {{ - Configuration configuration = Configuration.builder() - .resource(ResourceConfig.builder() - .env(Collections.singletonMap("MY_KEY", "MY_VALUE")) - .build()) - .image(imageConfiguration) - .build(); - context.getConfiguration(); result = configuration; - imageConfiguration.getName(); result = "busybox"; imageConfiguration.getAlias(); result = "busybox"; }}; @@ -78,7 +71,7 @@ public void checkEnrichDeployment() throws Exception { KubernetesListBuilder builder = new KubernetesListBuilder().addToItems(new DeploymentBuilder().build()); imageEnricher.create(PlatformMode.kubernetes, builder); - assertCorrectlyGeneratedResources(builder.build(), "Deployment"); + assertCorrectlyGeneratedResources(builder.build(), "Deployment", "MY_KEY", "MY_VALUE"); } @Test @@ -86,7 +79,7 @@ public void checkEnrichReplicaSet() throws Exception { KubernetesListBuilder builder = new KubernetesListBuilder().addToItems(new ReplicaSetBuilder().build()); imageEnricher.create(PlatformMode.kubernetes, builder); - assertCorrectlyGeneratedResources(builder.build(), "ReplicaSet"); + assertCorrectlyGeneratedResources(builder.build(), "ReplicaSet", "MY_KEY", "MY_VALUE"); } @Test @@ -96,7 +89,7 @@ public void checkEnrichReplicationController() throws Exception { .endReplicationControllerItem(); imageEnricher.create(PlatformMode.kubernetes, builder); - assertCorrectlyGeneratedResources(builder.build(), "ReplicationController"); + assertCorrectlyGeneratedResources(builder.build(), "ReplicationController", "MY_KEY", "MY_VALUE"); } @Test @@ -104,7 +97,7 @@ public void checkEnrichDaemonSet() throws Exception { KubernetesListBuilder builder = new KubernetesListBuilder().addToItems(new DaemonSetBuilder().build()); imageEnricher.create(PlatformMode.kubernetes, builder); - assertCorrectlyGeneratedResources(builder.build(), "DaemonSet"); + assertCorrectlyGeneratedResources(builder.build(), "DaemonSet", "MY_KEY", "MY_VALUE"); } @Test @@ -112,7 +105,7 @@ public void checkEnrichStatefulSet() throws Exception { KubernetesListBuilder builder = new KubernetesListBuilder().addToItems(new StatefulSetBuilder().build()); imageEnricher.create(PlatformMode.kubernetes, builder); - assertCorrectlyGeneratedResources(builder.build(), "StatefulSet"); + assertCorrectlyGeneratedResources(builder.build(), "StatefulSet", "MY_KEY", "MY_VALUE"); } @Test @@ -120,17 +113,66 @@ public void checkEnrichDeploymentConfig() throws Exception { KubernetesListBuilder builder = new KubernetesListBuilder().addToItems(new DeploymentConfigBuilder().build()); imageEnricher.create(PlatformMode.kubernetes, builder); - assertCorrectlyGeneratedResources(builder.build(), "DeploymentConfig"); + assertCorrectlyGeneratedResources(builder.build(), "DeploymentConfig", "MY_KEY", "MY_VALUE"); + } + + @Test + public void create_whenEnvironmentVariableAbsent_thenAddsEnvironmentVariable() throws JsonProcessingException { + // Given + KubernetesListBuilder builder = new KubernetesListBuilder().addToItems(new DeploymentBuilder().build()); + + // When + imageEnricher.create(PlatformMode.kubernetes, builder); + + // Then + assertCorrectlyGeneratedResources(builder.build(), "Deployment", "MY_KEY", "MY_VALUE"); } - private void assertCorrectlyGeneratedResources(KubernetesList list, String kind) throws JsonProcessingException { + @Test + public void create_whenEnvironmentVariablePresentWithDifferentValue_thenOldValueIsPreserved() throws JsonProcessingException { + // Given + givenResourceConfigWithEnvVar("key", "valueNew"); + KubernetesListBuilder builder = new KubernetesListBuilder().addToItems(new DeploymentBuilder() + .withNewSpec() + .withNewTemplate() + .withNewSpec() + .addNewContainer() + .addNewEnv().withName("key").withValue("valueOld").endEnv() + .endContainer() + .endSpec() + .endTemplate() + .endSpec() + .build()); + + // When + imageEnricher.create(PlatformMode.kubernetes, builder); + + // Then + assertCorrectlyGeneratedResources(builder.build(), "Deployment", "key", "valueOld"); + } + + private void assertCorrectlyGeneratedResources(KubernetesList list, String kind, String expectedKey, String expectedValue) throws JsonProcessingException { assertEquals(1, list.getItems().size()); String json = ResourceUtil.toJson(list.getItems().get(0)); assertThat(json, JsonPathMatchers.isJson()); assertThat(json, JsonPathMatchers.hasJsonPath("$.kind", Matchers.equalTo(kind))); - assertThat(json, JsonPathMatchers.hasJsonPath("$.spec.template.spec.containers[0].env[0].name", Matchers.equalTo("MY_KEY"))); - assertThat(json, JsonPathMatchers.hasJsonPath("$.spec.template.spec.containers[0].env[0].value", Matchers.equalTo("MY_VALUE"))); + assertThat(json, JsonPathMatchers.hasJsonPath("$.spec.template.spec.containers[0].env[0].name", Matchers.equalTo(expectedKey))); + assertThat(json, JsonPathMatchers.hasJsonPath("$.spec.template.spec.containers[0].env[0].value", Matchers.equalTo(expectedValue))); + } + + private void givenResourceConfigWithEnvVar(String name, String value) { + // @formatter:off + new Expectations() {{ + Configuration configuration = Configuration.builder() + .resource(ResourceConfig.builder() + .env(Collections.singletonMap(name, value)) + .build()) + .image(imageConfiguration) + .build(); + context.getConfiguration(); result = configuration; + }}; + // @formatter:on } }