-
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
Refactor ConfigurationChangeDetector to simplify conditionals. Issue 643 #644
Conversation
…ge detectors each for reload detection mode event/polling. Fixes spring-cloud#643
…nfigMap and Secrets for HTTP and Bus implementations
@ryanjbaxter - This is an attempt to refactor the configuration watcher implementation. It also sounds like based on the docs, the watcher as a side-car is the recommended approach as opposed to the native reload mechanism that is deprecated (wonder why? would benefit from some more background on this.) . May be best to also update the docs to reflect this and make it more obvious in the context of all of the reload config options available. |
@krisiye wow! Thanks for this! Amazing work! The reasons for the controller based approach is because using the Kubernetes API by applications running on Kubernetes is generally discouraged. Most of what spring-cloud-kubernetes-config did was read ConfigMaps and Secrets from Kubernetes and creating PropertySources from them. That piece of functionality can easily be replaced by mounting the ConfigMaps and Secrets as volumes. The piece of functionality that is missing when mounting them as volumes is notifying the app that the configuration changed and should be refreshed. This is where the new controller comes in. It is the one watching the ConfigMaps and Secrets for changes and then uses Spring Cloud Bus to notify the app of the changes. Does that make sense? |
Thanks @ryanjbaxter That makes a lot of sense! Decoupling from app and running as a sidecar seems more scalable! For the bus implementation, It may also make sense to support other messaging implementations in the future. For example: kafka |
@krisiye yes, plan is to support everything bus supports! (Again, if you have any interest in helping out there we would appreciate it!). Could you fix the merge conflicts and update the PR? |
@ryanjbaxter - Fixed conflicts. Should be good to go. CI is failing for unrelated modules looking for dependencies. Possible to re-run? I am unable to re-run on my end. missing permissions. |
@ryanjbaxter The watcher (controller) also needs some improved logging both info and debug. I'll work on those and log issues. |
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 looks good to me. I also did some testing on my own and everything seemed to work.
@spencergibb @Haybu if you have a second could you take a look as well?
…ge detectors each for reload detection mode event/polling. Fixes #643