Issue 635 #638
Issue 635 #638
Conversation
…m optional dependencies for reload auto config as they could be enabled/disabled through config independent of each other. Fixes spring-cloud#635
…ndencies for reload default auto configuration. spring-cloud#635
| @@ -71,10 +71,10 @@ | |||
| @Autowired | |||
| private KubernetesClient kubernetesClient; | |||
|
|
|||
| @Autowired | |||
| @Autowired(required = false) | |||
| private ConfigMapPropertySourceLocator configMapPropertySourceLocator; | |||
There was a problem hiding this comment.
It seems like we can just make the bean ConfigurationChangeDetector bean condition on the presence of these PropertySourceLocators and then add them to the method signature.
There was a problem hiding this comment.
@ryanjbaxter - Thanks for the review! The ConfigurationChangeDetector implementation takes care of the conditions and does appropriate checking before the watchers (polling/event) are registered. I believe we should be okay with that? Also I can resubmit this against 1.1.x. Do you want me to close this in favor of that? Just FYI - this issue also exists on the master branch.
There was a problem hiding this comment.
Submitted #639 against 1.1.x
There was a problem hiding this comment.
@krisiye are you talking about here?
There is no null check...
Besides the preferred pattern here would to not autowire them and just inject them into the method.
There was a problem hiding this comment.
@ryanjbaxter - I meant the property checks here - https://github.com/spring-cloud/spring-cloud-kubernetes/blob/master/spring-cloud-kubernetes-config/src/main/java/org/springframework/cloud/kubernetes/config/reload/EventBasedConfigurationChangeDetector.java#L68 . Sure we could probably get rid of Autowired dependency here to make it a bit more cleaner.
There was a problem hiding this comment.
@krisiye that is why I am saying to create it conditionally
There was a problem hiding this comment.
@ryanjbaxter - correct me if i am wrong, you are suggesting we create ConfigurationChangeDetector beans conditionally based on either properties configMap/secrets are enabled (maybe use @ConditionalOnExpression) and in addition we would add null checks under the polling/event change detectors. Right? pushed a commit to this effect. should help with any NPE's.
There was a problem hiding this comment.
@krisiye No what I am thinking is adding @ConditionalOnBean({ ConfigMapPropertySourceLocator.class, SecretsPropertySourceLocator.class }) on propertyChangeWatcher
There was a problem hiding this comment.
@ryanjbaxter okay to close this PR and make these updates on #639 ?
There was a problem hiding this comment.
@ryanjbaxter - Updated #639 with review comments. thanks!
Fixes #635