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

Added ConditionalOnMissingBean to AutoConfiguration #2414

Merged

Conversation

trimoq
Copy link
Contributor

@trimoq trimoq commented Sep 29, 2022

Fixes #2411.

Add ConditionalOnMissingBean to AxonServerActuatorAutoConfiguration methods to allow users to add AxonServerHealthIndicator and AxonServerStatusAggregator beans more easily, e.g. from third-party libraries.

@trimoq trimoq requested a review from smcvb September 29, 2022 17:39
@trimoq trimoq self-assigned this Sep 29, 2022
@smcvb smcvb added Type: Enhancement Use to signal an issue enhances an already existing feature of the project. Priority 3: Could Low priority. Issues that are nice to have but have a straightforward workaround. Status: In Progress Use to signal this issue is actively worked on. labels Sep 30, 2022
@smcvb smcvb added this to the Release 4.6.1 milestone Sep 30, 2022
Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

Although fair, I'd expect a test case to validate that by defining your own AxonServerHealthIndicator and AxonServerStatusAggregator, you don't receive the defaults from this auto-configuration class. You can check the AxonServerActuatorAutoConfigurationTest to see what such a test would look like.

@sonarcloud
Copy link

sonarcloud bot commented Oct 2, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

The tests look good, but we need to discuss the conditional statement based on the SimpleStatusAggregator.

Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

After a short investigation and discussion on the specifics of StatusAggregator instances, we're confident that only a single instance may exist within an application. As such, annotating it with ConditionalOnMissing(SimpleStatusAggregator) should be fine.

@smcvb smcvb merged commit 453b5d0 into axon-4.6.x Oct 3, 2022
@smcvb smcvb deleted the enhancement/conditional_bean_in_actuator_auto_conf branch October 3, 2022 10:13
@smcvb smcvb added Status: Resolved Use to signal that work on this issue is done. and removed Status: In Progress Use to signal this issue is actively worked on. labels Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority 3: Could Low priority. Issues that are nice to have but have a straightforward workaround. Status: Resolved Use to signal that work on this issue is done. Type: Enhancement Use to signal an issue enhances an already existing feature of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants