-
Notifications
You must be signed in to change notification settings - Fork 1k
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 1278 #1280
Fix 1278 #1280
Conversation
Configure Renovate
@@ -29,6 +32,7 @@ | |||
@Configuration(proxyBeanMethods = false) | |||
@ConditionalOnProperty("spring.cloud.config.discovery.enabled") | |||
@Import({ Fabric8AutoConfiguration.class, KubernetesDiscoveryClientAutoConfiguration.class }) | |||
@EnableConfigurationProperties({ KubernetesDiscoveryProperties.class, KubernetesClientProperties.class }) |
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.
this is the core problem right here. this bootstrap imports two configurations:
Fabric8AutoConfiguration
, which internally requiresKubernetesClientProperties
KubernetesDiscoveryClientAutoConfiguration
which internally requiresKubernetesDiscoveryProperties
These work just fine on their own if used by auto-configuration only, but once we use bootstrap annotations like @AutoConfigureBefore
break.
The one change that I did not account for was the fact that before this change, we used to provide an explicit KubernetesDiscoveryProperties
@Bean
and I dropped it once I fixed that issue. Totally my mistake here.
The other one with KubernetesClientProperties
was still present, but as said in the issue itself, we got very lucky that no one actually stepped on it.
It's kind of sad that I spent so much time thinking into not breaking anything once I make my changes, and it still happens...
@@ -0,0 +1,16 @@ | |||
spring: |
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've added an integration test with bootstrap enabled, that should catch these kind of things in the future.
@ryanjbaxter ready to be looked at. thank you. |
No description provided.