-
Notifications
You must be signed in to change notification settings - Fork 4
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 property spring.autoconfigure.exclude to sender application.properties #3
base: master
Are you sure you want to change the base?
Added property spring.autoconfigure.exclude to sender application.properties #3
Conversation
@@ -61,6 +61,10 @@ logging.level.sender-complex-obs-sync=${openmrs.eip.log.level} | |||
|
|||
# ****************************** Actuator Configuration **************************************************************** | |||
# | |||
# To prevent errors at startup with missing class: META-INF/services/org/apache/activemq/transport/vm | |||
# because of overlaps between ActiveMQ and Artemis. | |||
spring.autoconfigure.exclude=org.springframework.boot.actuate.autoconfigure.jms.JmsHealthContributorAutoConfiguration |
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.
Since this is a configurable thing, don't you think we should keep it out of the codebase? Otherwise other implementations will need the health checker.
If the property doesn't work and this is a blocker may be in the interest of time we might just merge this as is otherwise I suggest we do the following for a future release,
I want to think this is only happening for the sender and not receiver which explains why you're actually only adding it to the sender's properties file, this is because in the sender we never define a connection factory with a broker URL with a proper transport like we do in the receiver, so I think the fix is to add a connection factory bean to the sender too that explicitly uses another transport and not vm.
I have also created EIP-125 to fix openmrs-eip in the future too because it unnecessarily ships with activemq libraries.
What do you suggest @mks-d and @icrc-fdeniger?
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.
I added this property because I believed it will fix the unit test issues on branch 3.0.x
But yes maybe if we remove the activemq libraries we won't have this issue.
For other applications, I believe that in any case, they will have their own application.properties file.
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.
The tests still fail for me even with your change, note that this change is in a file that is not on the classpath, so it has no effect on unit tests
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.
The failing tests are OpenmrsLoadProducerTest
, OpenmrsLoadProducerIntegrationTest
and HashBatchUpdaterTest
and their issue is something else that is project setup related
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.
Added comment
Have to exclude the autoconfiguration
org.springframework.boot.actuate.autoconfigure.jms.JmsHealthContributorAutoConfiguration
to avoid errors at startup due to missing class:META-INF/services/org/apache/activemq/transport/vm