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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Added property spring.autoconfigure.exclude to sender application.properties #3

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5529e5
Update application.properties
icrc-fdeniger Feb 13, 2023
bcb2148
Update application.properties
mks-d Feb 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions sender-app/configuration/application.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

@wluyima wluyima Mar 1, 2023 •
edited
Loading

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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

Copy link
Collaborator

@wluyima wluyima Mar 1, 2023 •
edited
Loading

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


#Whether to enable or disable all actuator endpoints by default.
management.endpoints.enabled-by-default=false
#management.endpoint.prometheus.enabled=true
Expand Down