Fix 1831 #1863
Fix 1831 #1863
Conversation
Signed-off-by: wind57 <[email protected]>
Signed-off-by: wind57 <[email protected]>
Signed-off-by: wind57 <[email protected]>
| @@ -49,6 +49,13 @@ | |||
| </dependency> | |||
|
|
|||
| <!-- Testing Dependencies --> | |||
|
|
|||
| <dependency> | |||
There was a problem hiding this comment.
I don't know what you think about this.. In order to properly test that things are indeed fixed, I need this dependency as test scope. It surely looks a bit weird, since the discovery pom is requesting config one, even if it does so for tests only. Let me know your thoughts here please
There was a problem hiding this comment.
Why does the test need to be in the discovery module?
There was a problem hiding this comment.
In order to reproduce the issue in a test, we need two classes :
Fabric8ConfigServerBootstrapperKubernetesConfigDataLocationResolver. Since this one is abstract, we need an implementation, so I chose :Fabric8ConfigDataLocationResolver, thus the maven modulespring-cloud-kubernetes-fabric8-configas a test dependency here.
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.
In the unit test could we provide an implementation of KubernetesConfigDataLocationResolver to simulate a real implementation and test it that way?
There was a problem hiding this comment.
ah nice! just did that, thank you
| 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.
we used to do :
if ( bootstrapContext.isRegistered(PROPERTIES_CLASS) { ... }
but now, we will do :
bootstrapContext.isRegistered(PROPERTIES_CLASS) && bootstrapContext.get(PROPERTIES_CLASS) != null
| .getBeanFactory() | ||
| .registerSingleton(name, event.getBootstrapContext().get(cls)); | ||
|
|
||
| T singleton = event.getBootstrapContext().get(cls); |
There was a problem hiding this comment.
I would have missed this one if it was not for the test tbh.
|
@ryanjbaxter ready to be looked at. Thank you |
| * | ||
| * @author wind57 | ||
| */ | ||
| @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, properties = { |
There was a problem hiding this comment.
without the fix, this test fails
Signed-off-by: wind57 <[email protected]>
No description provided.