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

Update Config Watcher Configuration #1660

Merged
merged 178 commits into from
Jun 12, 2024
Merged

Conversation

wind57
Copy link
Contributor

@wind57 wind57 commented May 30, 2024

No description provided.

wind57 and others added 30 commits December 4, 2021 07:59
@wind57 wind57 changed the title Fix 1657 Fix 1657 - part 1 May 30, 2024
}

@AutoConfiguration
static class RefreshTriggerConfiguration {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not removed, but moved to a separate configuration class

@Configuration
@Profile(KAFKA)
@Import({ ContextFunctionCatalogAutoConfiguration.class, RefreshTriggerConfiguration.class })
static class BusKafkaConfiguration {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not removed, but moved to a separate auto-configuration class

@Profile(AMQP)
@Import({ ContextFunctionCatalogAutoConfiguration.class, RabbitHealthContributorAutoConfiguration.class,
RefreshTriggerConfiguration.class })
static class BusRabbitConfiguration {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not removed, but moved to a separate auto-configuration class

@@ -46,13 +41,10 @@
*/
@Configuration(proxyBeanMethods = false)
@EnableConfigurationProperties({ ConfigurationWatcherConfigurationProperties.class })
@Import({ ConfigurationWatcherAutoConfiguration.RefreshTriggerConfiguration.class })
@ConditionalOnCloudPlatform(CloudPlatform.KUBERNETES)
@Import({ RefreshTriggerConfiguration.class })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

two changes here :

  • @Import has changed the class, since its not an inner class anymore
  • added @ConditionalOnCloudPlatform(CloudPlatform.KUBERNETES) here, and for two other auto configuration for amqp and kafka

@wind57 wind57 marked this pull request as ready for review May 30, 2024 15:31
@wind57
Copy link
Contributor Author

wind57 commented May 30, 2024

I want to extract some classes as individual auto configurations, because this will help me fix 1657. This PR does not do much, just extracts inner classes as separate auto-configuration ones @ryanjbaxter thank you for looking into it

@ryanjbaxter
Copy link
Contributor

I guess I am confused on how these changes help fix the issue

@ryanjbaxter
Copy link
Contributor

@wind57 see my comment above

@wind57
Copy link
Contributor Author

wind57 commented Jun 12, 2024

sorry, I missed that comment indeed. I had two ideas in my mind when creating this PR:

  • some time ago we had some nasty bugs with inner classes as configurations, so I decided to refactor things here, not to be in that position again.
  • the second one, related to the issues, is that I plan to not touch existing functionality too much. So I was thinking to have some Conditionals to differentiate between our existing implementation and the new one. If I separate configurations in this manner, it will be easier achieve that.

That is what I was leaning towards

@ryanjbaxter ryanjbaxter changed the title Fix 1657 - part 1 Update Config Watcher Configuration Jun 12, 2024
@ryanjbaxter ryanjbaxter added this to the 3.1.3 milestone Jun 12, 2024
@ryanjbaxter ryanjbaxter merged commit 42bb80e into spring-cloud:main Jun 12, 2024
14 checks passed
@ryanjbaxter
Copy link
Contributor

OK I am going to rename this issue since it is not actually directly addressing #1657.

@wind57
Copy link
Contributor Author

wind57 commented Jun 12, 2024

Perfect, thank you

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