TRUNK-6208: Upgrade 2.7 Platform to Liquibase 4.27.0 #4637
Conversation
Yes, I am working on that. PR is currently in draft status and I will move out of draft once builds are passing. Thanks |
9f6d767
to
46bf41c
Compare
|
@k4pran when i run these changes against an existing openmrs database, it takes me to the maintenance mode database setup wizard page with very many changes that are considered as have never been run before. Do you still have sometime to look into this? 😊 |
I was able to reproduce this and the issue went away when I revert the
instead of
Please try again and let me know if you still have the issue, also let me know if you think we need to be concerned about someone running liquibase from a different location and I can see if there are alternative solutions. |
|
@k4pran thank you so much for looking into this. Did you find a way for getting rid of the so many logs about the liquibase search path? |
I am working on adding options to ignore these warnings or log at debug level, but I need to do that in liquibase, haven’t found a way to ignore them without updating liquibase - liquibase/liquibase#5856 EDIT: Actually I just had an idea, luckily the class that logs the warning only has a single log statement in it (the warning log about duplicate changelogs), so I could add a logging rule to set the log level for that class to error? Not a long term solution but might be ok until we make the changes in liquibase for the long term solution? |
I agree with you! 😊 |
So another complication cropped up, setting the log level to ERROR for this class was fine, but I realised that liquibase have a feature where some messages sent to both the logger and the stdout / stderr via this UI Service - https://docs.liquibase.com/parameters/ui-service.html. I couldn't seem to control it with the parameter in the docs so used a workaround suggested in these threads - liquibase/liquibase#3651 and liquibase/liquibase#2396. The change sets the ui service to use the This is another solution that doesn't seem ideal as using |
I still have the |
I found one place that I had missed and committed it, I will try to check more tomorrow in case I missed anywhere else |
Awesome! 👍 |
Hopefully there are not more logs now from my commit last night if you want to try again. I tested different combinations of initializing the database, but if you see it come up again please let me know how you set it up and maybe I can reproduce it, seems there are a lot of paths the lead to that log being triggered. |
|
@k4pran this is the log that i get when running your current changes: https://pastebin.com/kSzkPEc1 |
No I don’t have any modules running, will try it with other modules, are there many modules that call liquibase library directly or do they generally just uss core for that? What way did you set it up with additional modules? When I used the openmrs-sdk and deployed the latest platform I ran into checksum issues, it mentions "Importing an initial database from classpath:https://openmrs-platform.sql...", maybe I need to update something here too? EDIT: I was able to reproduce it with some modules and fix it. Maybe it would be useful for me to list the different ways I have initialized the database in case there are any gaps and you can advise if there are other ways I should try
When running I have been running just as the jetty server from intellij. I pushed that small fix to handle with modules. I am starting work now but please let me know any other scenarios I should try and will try over the weekend. 🙂 |
|
@k4pran the fix worked! 😊 There is a new error showing up which you can reproduce by downloading this module https://addons.openmrs.org/show/org.openmrs.module.initializer and dropping it into the modules folder before restarting the app. |
I am having some difficulty reproducing an error for this module. I initialized the database on 2.6.x xmls, I started the jetty server which created this directory structure Directory: C:\Users\ryanm\AppData\Roaming\OpenMRS
Mode LastWriteTime Length Name
---- ------------- ------ ----
d----- 25/05/2024 09:34 configuration
-a---- 25/05/2024 09:34 0 openmrs.logLogs at this point - https://pastebin.com/5nx0NrNm Then I created a modules folder and dropped the initializer omod in from the link and started the jetty server again. I went through the startup wizard with "simple -> no demo data". My directory structure ended up With logs https://pastebin.com/zGzJXY8a Let me know if I am doing it different than you somehow and if you get time could you also share your logs please? I also tried it with omods fhir2-1.9.0, owa-1.14.0 and webservices.rest-2.38.0 |
|
This is my full log: https://pastebin.com/HLsmztHD |
I'm not sure that this error is related to my changes, on windows it seems to pass but windows is stringifying the null avoiding the NPE, unix file system is maybe handling it differently. On windows I get a success I think the issue might be that null is being hardcoded here - here as this null is what is being used to create the path. I will try to set up a linux environment to try it out. |
Instead of taking you through all the linux environment setup, how about just fixing initializer module? And then i test it for you on the linux environment. 😊 |
I made the change here and did a quick regression test on windows, please give it a go on linux and let me know - https://github.com/mekomsolutions/openmrs-module-initializer/pull/267/files |
| greater than 10 is now interpreted as BIGINT in H2 databases. | ||
| See: https://github.com/liquibase/liquibase/pull/4008/files | ||
| --> | ||
| <modifyDataType tableName="program_attribute_type" columnName="retired_by" newDataType="int(10)"/> |
There was a problem hiding this comment.
There are many columns of type="INT"
Is there a reason why you did this for only this?
There was a problem hiding this comment.
This was the only one that seemed to fail validation, but updated to include the others
There was a problem hiding this comment.
Is the validation failure reproduced by simply running mvn clean install without the changes in this file?
There was a problem hiding this comment.
I think it was from running the integration tests that triggered the validation failure (before changes to this file)
There was a problem hiding this comment.
Do you remember why it complained for only program_attribute_type but not others?
There was a problem hiding this comment.
yea sure, I rolled back the other other modifies except the one that causes the test failures
There was a problem hiding this comment.
Do you, by any chance, have an idea of how we can avoid having to add this changeset to the liquibase-update-to-latest-2.7.x.xml file? After all is it only for that failing test.
There was a problem hiding this comment.
I’m not sure because I guess we will still want to keep the test that does the hibernate validation, but I will look at other options to see if we can avoid adding the changeset
There was a problem hiding this comment.
but I will look at other options to see if we can avoid adding the changeset
That will be awesome!
There was a problem hiding this comment.
I just changed the type from int(11) to int at da88f5a, just like the rest of the retired_by fields, then i removed the h2 specific changeset.
| @@ -1790,6 +1790,7 @@ | |||
| <validCheckSum>1b6141be59a8a243b9e56d0aad952af</validCheckSum> <!-- checksum from before idExceptions added --> | |||
| <validCheckSum>89cc7a14b0582f157ea2dbb9de092fd</validCheckSum> <!-- current checksum with idExceptions param added --> | |||
| <validCheckSum><comment>Changes to new concept_reference tables</comment>3:4edd135921eb263d4811cf1c22ef4846</validCheckSum> | |||
| <validCheckSum><comment>Account for trailing whitespace</comment>9:5d41399f930d6de5c0030986b78f35e5</validCheckSum> | |||
There was a problem hiding this comment.
Was this brought about by the liquibase upgrade?
There was a problem hiding this comment.
Yes IIRC there were some changes to how whitespace was handled so there were a few checksum mismatches
| @@ -1314,6 +1315,9 @@ public static enum PERSON_TYPE { | |||
| /** Value for the long person name format */ | |||
| public static final String PERSON_NAME_FORMAT_LONG = "long"; | |||
|
|
|||
| // Liquibase Constants | |||
| public static String LIQUIBASE_DUPLICATE_FILE_MODE_DEFAULT = GlobalConfiguration.DuplicateFileMode.WARN.name(); | |||
| @@ -310,7 +320,7 @@ public static Boolean allowAutoUpdate() { | |||
| String allowAutoUpdate = Context.getRuntimeProperties() | |||
| .getProperty(OpenmrsConstants.AUTO_UPDATE_DATABASE_RUNTIME_PROPERTY, "false"); | |||
|
|
|||
| return "true".equals(allowAutoUpdate); | |||
There was a problem hiding this comment.
Yes this was a mistake, fixed
It works like a charm! 😊 |
|
@k4pran could you be knowing what is going on here? https://qa-refapp.openmrs.org/openmrs/initialsetup |
* TRUNK-6208: Upgrade 2.7 Platform to Liquibase 4.27.0 * TRUNK-6208 remove h2 changeset * TRUNK-6208 update changeset count after remove h2 changeset * TRUNK-6208 change type from int(11) to int --------- Co-authored-by: dkayiwa <[email protected]>
Description of what I changed
This upgrades liquibase to the latest version 4.27.0. There are few other changes worth mentioning
duplicateFileModeis a new config added to liquibase to handle duplicate changelogs. The only options at the minute is for it to error on any duplicates or do log a warning. I couldn't work around this config without overriding some of the lower level liquibase classes, so I opted to default it to WARN. The issue is that the warning logs are quite excessive. I raised this with liquibase and got the green light to add more options - Add Additional Duplicate File Modes (DEBUG and / or IGNORE) to ResourceAccessor liquibase/liquibase#5856duplicateFileModeas a config in openmrs, it seems that liquibase needs it set in the pom, a liquibase.properties file or an environment variable. It also needs set before liquibase runs, I opted to add a constant defaults toWARNwith the option to override this either in the openmrs-runtime.properties or in the environment variables with the openmrs properties taking precedence.logicalFilePathattribute, this resulted in an issue whereChangeLogDetectivehad difficulty determining the changelog used to initialize the database. I only added the missing attribute in 2.6.x and later. I could also add it to 2.5.x but didn't want to cause a compatibility issue doing so.I updated a few deprecated methods, there are some additional deprecation that I didn't touch, I can do it here or separately in case it is more involved:
Issue I worked on
see https://issues.openmrs.org/browse/TRUNK-6208
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