Skip to content
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

Revert changes in config server bootstrapper and use new PropertyResolver class #328

Merged

Conversation

ryanjbaxter
Copy link
Contributor

@ryanjbaxter ryanjbaxter commented Jan 18, 2024

@ryanjbaxter
Copy link
Contributor Author

@pantherdd I think this is ultimately a better solution to #324. It takes your idea of registering the Binder from the profile resolution code in Spring Boot and instead wraps that Binder in a class called PropertyResolver allowing it to all be encapsulated for the downstream code. The original code of ZookeeperConfigServerBootstrapper is basically restored with the only exception being he new use of PropertyResolver. Your custom Bootstrapper should also work as before except you would also need to use the new PropertyResolver class.

Note it depends on changes in spring-cloud/spring-cloud-config#2375

If you agree we can merge these changes and close #327

@pantherdd
Copy link

pantherdd commented Jan 19, 2024

@ryanjbaxter Great news, thanks for giving this idea a try and making it work. I'm going to review this PR today.

FTR, the breaking changes (that this PR is reverting) were introduced to the 4.0.x branch in v4.0.1, so I'm using the following comparison of ZookeeperConfigServerBootstrapper to v4.0.0 to better see what the net changes are going to be: v4.0.0...ad32f45 (deep linking to the exact file doesn't seem to work :( )

@pantherdd
Copy link

pantherdd commented Jan 19, 2024

With most of the earlier changes reverted, do we still need these to exist, or can we remove them too?

After this change, they seem to be dead code basically. Unless I'm missing some other use for them.

(Ref.: spring-cloud/spring-cloud-config#2269)

@ryanjbaxter
Copy link
Contributor Author

Thanks @pantherdd! I have made your suggested changes.

Regarding the "dead code" in ConfigServerInstanceProvider, I agree this code is likely not needed anymore. Once we are satisfied with these changes, I will need to make similar changes in Spring Cloud Netflix, Consul, and Kubernetes which also have implementations. If I can use the same pattern there, the code won't be used anymore. Unfortunately since it made it out into a release (which is my fault) we can only deprecate the constructor for now and remove it in the next major.

Copy link

@pantherdd pantherdd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, and the plan for the "dead code" sounds reasonable too.
Thanks! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants