TRUNK-6418: Run liquibase checks and data imports only when version of core or modules changes #5446
Conversation
|
@dkayiwa @rkorytkowski could you please review this PR, your feedback will be appreciated. |
|
@IamMujuziMoses did you actually test this out in a running openms webapp to confirm that it works? |
|
@dkayiwa how do you mean? |
|
What is this change supposed to accomplish? |
|
@dkayiwa yes, I tested this out in a running openmrs instance successfully |
|
Other than simply running the webapp, how did you test that these changes do what they are supposed to? |
|
@dkayiwa, I have tried to screen record the process, hope these videos help in explaining a bit, they're not very clear, apologies: |
rkorytkowski
left a comment
rkorytkowski
left a comment
There was a problem hiding this comment.
Please address review comments and add logic for skipping core liquibase changesets in Context.checkForDatabaseUpdaes
| }); | ||
|
|
||
| storeModuleVersion(module.getModuleId(), currentModuleVersion); | ||
| } |
There was a problem hiding this comment.
It would be much cleaner to put this logic inside a service method. It would run in a transaction this way so runInSingleTransaction would not be needed.
| } | ||
|
|
||
| protected static String getStoredCoreVersion() { | ||
| return Context.getAdministrationService().getGlobalProperty("core.version"); |
There was a problem hiding this comment.
Better to inject AdministrationService since you already made it a component, but as noted above I would just move the logic to boolean AdministrationService.isModuleSetupOnVersionChangeNeeded(String moduleId) for checking core and modules and similarly boolean AdministrationService.isCoreSetupOnVersionChangeNeeded() for checking just coreVersion and running core liquibase, which you haven't implement yet.
|
|
||
| if (versionChanged) { | ||
| runInSingleTransaction(() -> { | ||
| module.getModuleActivator().setupOnVersionChangeBeforeSchemaChanges(prevCoreVersion, prevModuleVersion); |
There was a problem hiding this comment.
The idea of this method is to call it before running ANY liquibase changesets, including ones run by core so it should not be here rather in Context.checkForDatabaseUpdates...
|
Thanks @IamMujuziMoses and I'm sorry for getting back to you after that long. I was away... Please let me know if you are able to work on it within the next week or else I'll take it over and finish up. |
|
@rkorytkowski No worries at all, welcome back! Thanks for the review and Yes, I can work on it within the next week. I’ll keep you updated as I progress. |
IamMujuziMoses
force-pushed
the
TRUNK-6418
branch
4 times, most recently
from
7f0a355 to
065820b
Compare
…f core or modules changes
…f core or modules changes
rkorytkowski
force-pushed
the
TRUNK-6418
branch
from
065820b to
c721ef9
Compare
3 participants
Issue I worked on
see https://issues.openmrs.org/browse/TRUNK-6418
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amendI have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amendI ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master