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

Jump to bottom

EIP-133: Upgrade dependencies and frameworks under new 4.0.0 major. #43

Merged
merged 11 commits into from
Nov 20, 2023
Merged

EIP-133: Upgrade dependencies and frameworks under new 4.0.0 major. #43

merged 11 commits into from
Nov 20, 2023

Conversation

corneliouzbett
Copy link
Member

@corneliouzbett corneliouzbett commented Oct 27, 2023 •
edited by mks-d
Loading

This PR upgrades project dependencies and frameworks to the latest versions (or for some a more recent version). Some of the notable upgrades include:

Dependency Before upgrade After upgrade
Java 8 17
Spring Boot 2.3.0.RELEASE 3.1.4
Hibernate 5.4.21.Final 6.3.1.Final
Camel 3.3.0 4.1.0
TestContainers 1.15.2 1.19.1

I've also, updated unit tests to use JUnit5.

mks-d
mks-d reviewed Oct 27, 2023
View reviewed changes
Copy link
Member

@mks-d mks-d left a comment

Choose a reason for hiding this comment

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

@corneliouzbett so we're making a new major of openmrs-eip here right?

@corneliouzbett
Copy link
Member Author

corneliouzbett commented Oct 27, 2023 •
edited
Loading

@corneliouzbett so we're making a new major of openmrs-eip here right?

Yes, I agree. We should create a 3.x branch for 3.x releases and development, and update master to 4.0.0-SNAPSHOT. We need to get consensus before we proceed.
@wluyima @Ruhanga @icrc-fdeniger

mks-d
mks-d reviewed Oct 27, 2023
View reviewed changes
Copy link
Member

@mks-d mks-d left a comment

Choose a reason for hiding this comment

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

I think this PR should already make this proposition by upgrading to 4.0.0-SNAPSHOT.

@mks-d mks-d changed the title EIP-133: Upgrade project dependencies and frameworks. EIP-133: Upgrade project dependencies and frameworks under new 4.0.0 major. Oct 27, 2023
@mks-d mks-d changed the title EIP-133: Upgrade project dependencies and frameworks under new 4.0.0 major. EIP-133: Upgrade dependencies and frameworks under new 4.0.0 major. Oct 27, 2023
ibacher
ibacher reviewed Oct 27, 2023
View reviewed changes
Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

Just a couple of comments. Nice work overall!

commons/src/main/java/org/openmrs/eip/Utils.java Outdated Show resolved Hide resolved
commons/src/main/java/org/openmrs/eip/app/management/config/ManagementDataSourceConfig.java Show resolved Hide resolved
mks-d
mks-d reviewed Oct 27, 2023
View reviewed changes
Copy link
Member

@mks-d mks-d left a comment

Choose a reason for hiding this comment

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

My two cents, I couldn't look through all the files. Feel free to ignore and focus on others reviews instead.

commons/src/main/java/org/openmrs/eip/camel/OauthProcessor.java Outdated Show resolved Hide resolved
commons/src/main/java/org/openmrs/eip/app/management/config/ManagementDataSourceConfig.java Outdated Show resolved Hide resolved
commons/src/test/java/org/openmrs/eip/BaseCamelTest.java Show resolved Hide resolved
@wluyima
Copy link
Member

wluyima commented Oct 31, 2023 •
edited
Loading

This is great work, I have some comments below,

  • The PR is too huge to review in a reasonable timeframe, this is partly because it has auto format changes, it would be nice to setup the OpenMRS code formatter and and apply it to the PR, I believe the project uses an older version of the OpenMRS core code formatter from an older branch of OpenMRS core like 2.3.x, I can provide the formatter if you can't find it. It would be nice to configure the formatter in the pom to be part of a build.
  • It would have been nice to ONLY make changes that are directly related to the upgrades as part of this PR, anything else cosmetic or that is not needed could have waited and be done in a separate PR.
  • Please do not change any application behavior or remove existing tests unless otherwise.

@corneliouzbett corneliouzbett marked this pull request as draft November 6, 2023 14:09
@corneliouzbett corneliouzbett mentioned this pull request Nov 7, 2023
Closed
@corneliouzbett corneliouzbett marked this pull request as ready for review November 9, 2023 20:06
Ruhanga
Ruhanga reviewed Nov 14, 2023
View reviewed changes
Copy link
Member

@Ruhanga Ruhanga left a comment

Choose a reason for hiding this comment

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

Thanks for the work @corneliouzbett. Find below a few issues to address.

commons/src/test/java/org/openmrs/eip/DeleteDataTestExecutionListener.java Show resolved Hide resolved
commons/src/test/java/org/openmrs/eip/DeleteDataTestExecutionListener.java Outdated Show resolved Hide resolved
commons/src/test/java/org/openmrs/eip/DeleteDataTestExecutionListener.java Show resolved Hide resolved
commons/src/test/java/org/openmrs/eip/camel/route/GetConceptByMappingFromOpenmrsRouteTest.java Outdated Show resolved Hide resolved
example-app/src/main/resources/application.properties Outdated Show resolved Hide resolved
openmrs-watcher/src/test/java/org/openmrs/eip/mysql/watcher/DebeziumMessageProcessorTest.java Outdated Show resolved Hide resolved
openmrs-watcher/src/test/java/org/openmrs/eip/mysql/watcher/DebeziumMessageProcessorTest.java Outdated Show resolved Hide resolved
openmrs-watcher/src/test/java/org/openmrs/eip/mysql/watcher/WatcherTestUtils.java Outdated Show resolved Hide resolved
...mrs-watcher/src/test/java/org/openmrs/eip/mysql/watcher/route/DbEventProcessorRouteTest.java Outdated Show resolved Hide resolved
...cher/src/test/java/org/openmrs/eip/mysql/watcher/route/WatcherRetryItemHandlerRouteTest.java Outdated Show resolved Hide resolved
Ruhanga
Ruhanga reviewed Nov 17, 2023
View reviewed changes
Copy link
Member

@Ruhanga Ruhanga left a comment

Choose a reason for hiding this comment

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

Generally LGTM. A few more comment to address below.

commons/src/main/java/org/openmrs/eip/Utils.java Show resolved Hide resolved
commons/src/test/java/org/openmrs/eip/camel/OauthProcessorTest.java Outdated Show resolved Hide resolved
openmrs-watcher/src/main/java/org/openmrs/eip/mysql/watcher/route/WatcherShutdownRoute.java
* CustomFileOffsetBackingStore - Sends a notification to the configured recipients
*/
@Component
public class WatcherShutdownRoute extends RouteBuilder {
Copy link
Member

@Ruhanga Ruhanga Nov 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Could you briefly document this issue for the record, maybe in the description (Reason the xml DSL is replaced by the Java counterpart)?

openmrs-watcher/src/main/java/org/openmrs/eip/mysql/watcher/MySqlWatcherEndpointConfigurer.java Outdated Show resolved Hide resolved
openmrs-watcher/src/test/resources/mgt_sender_retry_queue.sql Outdated Show resolved Hide resolved
Ruhanga
Ruhanga approved these changes Nov 17, 2023
View reviewed changes
Copy link
Member

@Ruhanga Ruhanga left a comment

Choose a reason for hiding this comment

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

@corneliouzbett thanks for the great work. If we can finish with this here, that'd be great. Otherwise this LGTM.

@mks-d mks-d merged commit 0ac9286 into openmrs:master Nov 20, 2023
1 check passed
@corneliouzbett corneliouzbett deleted the EIP-133 branch January 30, 2024 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mks-d mks-d mks-d left review comments

@Ruhanga Ruhanga Ruhanga approved these changes

@wluyima wluyima Awaiting requested review from wluyima

@ibacher ibacher Awaiting requested review from ibacher

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@corneliouzbett @wluyima @mks-d @Ruhanga @ibacher