-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix 1831 #1863
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix 1831 #1863
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,6 +52,8 @@ | |
public abstract class KubernetesConfigDataLocationResolver | ||
implements ConfigDataLocationResolver<KubernetesConfigDataResource>, Ordered { | ||
|
||
private static final Class<KubernetesClientProperties> PROPERTIES_CLASS = KubernetesClientProperties.class; | ||
|
||
private static final boolean RETRY_IS_PRESENT = isPresent("org.springframework.retry.annotation.Retryable", null); | ||
|
||
private final Log log; | ||
|
@@ -139,8 +141,7 @@ private void registerProperties(ConfigDataLocationResolverContext resolverContex | |
SecretsConfigProperties secretsProperties) { | ||
|
||
ConfigurableBootstrapContext bootstrapContext = resolverContext.getBootstrapContext(); | ||
registerSingle(bootstrapContext, KubernetesClientProperties.class, clientProperties, | ||
"configDataKubernetesClientProperties"); | ||
registerSingle(bootstrapContext, PROPERTIES_CLASS, clientProperties, "configDataKubernetesClientProperties"); | ||
|
||
if (configMapProperties != null) { | ||
registerSingle(bootstrapContext, ConfigMapConfigProperties.class, configMapProperties, | ||
|
@@ -174,15 +175,14 @@ private static PropertyHolder of(ConfigDataLocationResolverContext context) { | |
private static KubernetesClientProperties clientProperties(ConfigDataLocationResolverContext context, | ||
String namespace) { | ||
KubernetesClientProperties kubernetesClientProperties; | ||
ConfigurableBootstrapContext bootstrapContext = context.getBootstrapContext(); | ||
|
||
if (context.getBootstrapContext().isRegistered(KubernetesClientProperties.class)) { | ||
kubernetesClientProperties = context.getBootstrapContext() | ||
.get(KubernetesClientProperties.class) | ||
.withNamespace(namespace); | ||
if (bootstrapContext.isRegistered(PROPERTIES_CLASS) && bootstrapContext.get(PROPERTIES_CLASS) != null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we used to do :
but now, we will do :
|
||
kubernetesClientProperties = bootstrapContext.get(PROPERTIES_CLASS).withNamespace(namespace); | ||
} | ||
else { | ||
kubernetesClientProperties = context.getBinder() | ||
.bindOrCreate(KubernetesClientProperties.PREFIX, Bindable.of(KubernetesClientProperties.class)) | ||
.bindOrCreate(KubernetesClientProperties.PREFIX, Bindable.of(PROPERTIES_CLASS)) | ||
.withNamespace(namespace); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,6 +49,13 @@ | |
</dependency> | ||
|
||
<!-- Testing Dependencies --> | ||
|
||
<dependency> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know what you think about this.. In order to properly test that things are indeed fixed, I need this dependency as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does the test need to be in the discovery module? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In order to reproduce the issue in a test, we need two classes :
So the test is either here with that test dependency, or I move it to the integration tests project. Though I did not try that, so not sure if extra work would not be needed, but this is the explanation. I hope it makes sense There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the unit test could we provide an implementation of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah nice! just did that, thank you |
||
<groupId>org.springframework.cloud</groupId> | ||
<artifactId>spring-cloud-kubernetes-fabric8-config</artifactId> | ||
<scope>test</scope> | ||
</dependency> | ||
|
||
<dependency> | ||
<groupId>org.springframework.boot</groupId> | ||
<artifactId>spring-boot-starter-test</artifactId> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
/* | ||
* Copyright 2013-2025 the original author or authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package org.springframework.cloud.kubernetes.fabric8.discovery; | ||
|
||
import com.github.tomakehurst.wiremock.WireMockServer; | ||
import com.github.tomakehurst.wiremock.client.WireMock; | ||
import org.assertj.core.api.Assertions; | ||
import org.junit.jupiter.api.AfterAll; | ||
import org.junit.jupiter.api.BeforeAll; | ||
import org.junit.jupiter.api.Test; | ||
|
||
import org.springframework.beans.factory.annotation.Autowired; | ||
import org.springframework.boot.autoconfigure.SpringBootApplication; | ||
import org.springframework.boot.test.context.SpringBootTest; | ||
import org.springframework.context.ApplicationContext; | ||
|
||
import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; | ||
import static com.github.tomakehurst.wiremock.client.WireMock.get; | ||
import static com.github.tomakehurst.wiremock.client.WireMock.stubFor; | ||
import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; | ||
import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.options; | ||
|
||
/** | ||
* Test that proves that this | ||
* <a href="https://github.com/spring-cloud/spring-cloud-kubernetes/issues/1831">issue</a> | ||
* is fixed. | ||
* | ||
* @author wind57 | ||
*/ | ||
@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, properties = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. without the fix, this test fails |
||
"spring.main.cloud-platform=KUBERNETES", "spring.config.import=kubernetes:, optional:configserver:" }) | ||
class Fabric8ConfigServerTest { | ||
|
||
private static WireMockServer wireMockServer; | ||
|
||
@Autowired | ||
private ApplicationContext applicationContext; | ||
|
||
@BeforeAll | ||
static void beforeAll() { | ||
wireMockServer = new WireMockServer(options().port(8888)); | ||
wireMockServer.start(); | ||
WireMock.configureFor("localhost", wireMockServer.port()); | ||
} | ||
|
||
@AfterAll | ||
static void after() { | ||
WireMock.shutdownServer(); | ||
wireMockServer.stop(); | ||
} | ||
|
||
@Test | ||
void test() { | ||
stubFor(get(urlEqualTo("/application/default")).willReturn(aResponse().withStatus(200).withBody("{}"))); | ||
Assertions.assertThat(applicationContext).isNotNull(); | ||
} | ||
|
||
@SpringBootApplication | ||
protected static class TestConfig { | ||
|
||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have missed this one if it was not for the test tbh.