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

Deprecate constructor that uses namespace #1285

Merged

Conversation

wind57
Copy link
Contributor

@wind57 wind57 commented Apr 5, 2023

No description provided.

wind57 and others added 30 commits December 4, 2021 07:59
@wind57 wind57 marked this pull request as ready for review April 6, 2023 04:48
@wind57
Copy link
Contributor Author

wind57 commented Apr 6, 2023

@ryanjbaxter one more minor PR before two more far more interesting ones, imho. thank you!

@@ -85,4 +86,15 @@ public KubernetesInformerReactiveDiscoveryClient kubernetesReactiveDiscoveryClie
serviceLister, endpointsLister, serviceInformer, endpointsInformer, properties);
}

@Bean
@ConditionalOnMissingBean
KubernetesInformerReactiveDiscoveryClient kubernetesClientReactiveDiscoveryClient(
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldnt be creating 2 bean instances, we can remove the @Bean annotation above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's @ConditionalOnMissingBean on both of them, so only one will be created. We don't care which one will be created at this point in time, because they do the same thing in their constructors.

Later, in the next major release, I will have a PR that drops all of these deprecations.

Btw, the same situation exists in KubernetesInformerDiscoveryClientAutoConfiguration (the non-reactive auto-configuration).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or even better now that I think about your suggestion is to remove the @Bean in both reactive and non-reactive auto-configurations. This is one nice piece of advice, thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly

@ryanjbaxter ryanjbaxter added this to the 3.0.3 milestone Apr 6, 2023
@wind57 wind57 requested a review from ryanjbaxter April 7, 2023 07:06
@ryanjbaxter ryanjbaxter merged commit 55ce845 into spring-cloud:main Apr 7, 2023
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
Development

Successfully merging this pull request may close these issues.

3 participants