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

TRUNK-4830 Avoid running legacy liquibase changesets #2712

Merged
merged 1 commit into from May 13, 2020
Merged

TRUNK-4830 Avoid running legacy liquibase changesets #2712

merged 1 commit into from May 13, 2020

Conversation

WolfSchlegel
Copy link
Contributor

…nitialisation of OpenMRS database and upgrade liquibase-core to 3.6.1

Description of what I changed

Add Liquibase Maven Plugin 3.6.1, introduce Liquibase snapshots for initialisation of OpenMRS database and upgrade liquibase-core to 3.6.1

Issue I worked on

see https://issues.openmrs.org/browse/TRUNK-4830

Checklist: I completed these to help reviewers :)

  • My pull request only contains ONE single commit
    (the number above, next to the 'Commits' tab is 1).

    No? -> read here on how to squash multiple commits into one

  • 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 --amend

  • I 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 --amend

  • I ran mvn clean package right before creating this pull request and
    added 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

@coveralls
Copy link

coveralls commented Sep 15, 2018 •
edited

Coverage Status

Coverage decreased (-0.3%) to 60.294% when pulling 9dc36ba on WolfSchlegel:TRUNK-4830-N into 52598a4 on openmrs:master.

dkayiwa
dkayiwa reviewed Sep 19, 2018
View reviewed changes
liquibase/README.md Outdated Show resolved Hide resolved
@openmrs openmrs deleted a comment Sep 19, 2018
@openmrs openmrs deleted a comment Sep 20, 2018
@dkayiwa
Copy link
Member

dkayiwa commented Sep 21, 2019

@WolfSchlegel great to see you resume working on this. 😊

@WolfSchlegel WolfSchlegel force-pushed the TRUNK-4830-N branch 3 times, most recently from edd560a to b00a9c5 Compare October 20, 2019 14:46
@WolfSchlegel
Copy link
Contributor Author

Hey there,

I fixed all review comments but one, rebased upstream master and created new liquibase snapshot files for OpenMRS 2.2.x.

Could you please resume the review?

In the meantime I look into the pending fix of the comment on the datatype in DatabaseUpgradeTestUtil.java.

Thanks,
Wolf

@WolfSchlegel
Copy link
Contributor Author

Also, some pom files fail the security.snyk check but I cannot view the details.

Snyk says: "Unable to display this organization. The organization does not exist, or you do not have permissions to access it."

Could you please have a look at the reported vulnerability and let me know what it is about?

Thank you,
Wolf

@WolfSchlegel
Copy link
Contributor Author

That sounds doable enough, in particular changing the commit message :).

Regarding the old modify column jar, let me see if that is actually needed at all in the openmrs-liquibase module as the purpose of that module is to generate snapshots but not to execute them (that happens in other modules). If that works we are good, if the library is needed I move all liquibase extensions either to openmrs-liquibase or a new module.

Come to think of it, I guess I copied everything in the pom file that had liquibase in its name into the openmrs-liquibase pom file when I started coding.

dkayiwa
dkayiwa reviewed May 12, 2020
View reviewed changes
liquibase/README.md
* **not to add demo data** in step 3 of the installation wizard

#### Step 3 - Create a snapshot of the OpenMRS schema
cd <some root folder>/openmrs-core/liquibase
Copy link
Member

Choose a reason for hiding this comment

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

Do you remember how long this command took to finish on your computer? For me, it takes around 3hours. Is that the expected time frame?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, definitely not for hours, I need to re-run this when removing the old jar files from the pom.xml but it was maybe some 10 minutes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What kind of machine are you using pls?

Copy link
Member

Choose a reason for hiding this comment

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

MacBook Pro (Retina, 15-inch, Mid 2015)
Processor 2.8 GHz Intel Core i7
Memory 16 GB 1600 MHz DDR3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

~/Projects/OpenMRS/code10/openmrs-core/liquibase(TRUNK-4830-N) $ mvn -DoutputChangelogfile=liquibase-schema-only-SNAPSHOT.xml -Dusername=root -Dpassword=Admin123 liquibase:generateChangeLog
[INFO] Scanning for projects...
[INFO] 
...
[INFO] changeSets count: 881
[INFO] ../api/src/main/resources/liquibase-schema-only-SNAPSHOT.xml does not exist, creating and adding 881 changesets.
[INFO] Output written to Change Log file, ../api/src/main/resources/liquibase-schema-only-SNAPSHOT.xml
[INFO] ------------------------------------------------------------------------
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:35 min
[INFO] Finished at: 2020-05-12T17:44:02+02:00
[INFO] ------------------------------------------------------------------------
~/Projects/OpenMRS/code10/openmrs-core/liquibase(TRUNK-4830-N) $ mvn -DdiffTypes=data -DoutputChangelogfile=liquibase-core-data-SNAPSHOT.xml -Dusername=root -Dpassword=Admin123 liquibase:generateChangeLog
[INFO] Scanning for projects...
[INFO] 
[INFO] --------------< org.openmrs.liquibase:openmrs-liquibase >---------------
[INFO] Building openmrs-liquibase 2.4.0-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- liquibase-maven-plugin:3.8.9:generateChangeLog (default-cli) @ openmrs-liquibase ---
[INFO] ------------------------------------------------------------------------
[INFO] 
[INFO] 
[INFO] Liquibase Community 3.8.9 by Datical
[INFO] Starting Liquibase at Tue, 12 May 2020 17:46:53 CEST (version 3.8.9 #73 built at Mon Apr 06 09:32:58 UTC 2020)
[INFO] Executing on Database: jdbc:mysql://127.0.0.1:3306/openmrs
[INFO] Generating Change Log from database root@localhost @ jdbc:mysql://127.0.0.1:3306/openmrs (Default Schema: openmrs)
[INFO] changeSets count: 27
[INFO] ../api/src/main/resources/liquibase-core-data-SNAPSHOT.xml does not exist, creating and adding 27 changesets.
[INFO] Output written to Change Log file, ../api/src/main/resources/liquibase-core-data-SNAPSHOT.xml
[INFO] ------------------------------------------------------------------------
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  3.047 s
[INFO] Finished at: 2020-05-12T17:46:56+02:00
[INFO] ------------------------------------------------------------------------
~/Projects/OpenMRS/code10/openmrs-core/liquibase(TRUNK-4830-N) $

Copy link
Contributor Author

Choose a reason for hiding this comment

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

01:30 minutes for schema-only and 3 seconds for core-data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And it worked well w/o the legacy libraries, so I can remove them from the module's pom file.

@dkayiwa please hold your fire before you re-run the generation of snapshots for a few minutes and use the next commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The update is out, without the old modify column jar (which ticks the second box on @ibacher 's list)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re hardware I am using an 2018 Macbook Pro, the 2015 model should be perfectly ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A few colleagues of mine had tested the generation of snapshots and they would have let me know if it had taken ages for them...

@WolfSchlegel
Copy link
Contributor Author

WolfSchlegel commented May 13, 2020 •
edited

@dkayiwa I just received your questions about the <artifactId>modify-column</artifactId> dependency by mail but cannot find it in this pull request.

...i have been able to test upgrades without this dependency. What failure error message does this dependency solve for the upgrades?

Messages must have overlapped, I had removed the dependency yesterday from openmrs-liquibase and tested that they are not needed for snapshot generation.

So all should be good now with respect to that task, there are no references to that liibrary left.

@dkayiwa
Copy link
Member

dkayiwa commented May 13, 2020

@WolfSchlegel am referring to the lucene-backward-codecs dependency.

@WolfSchlegel
Copy link
Contributor Author

@dkayiwa got you, could you please let me know the steps of your test?

Me and a colleague of mine found that bug independently when testing the 2.2.x -> 2.4.x scenario.

I would then validate your steps on my side.

@dkayiwa
Copy link
Member

dkayiwa commented May 13, 2020

@WolfSchlegel in did a fresh installation of platform 2.2.x, which i later on upgraded to 2.4.x
Before the upgrade, i deleted the lucene folder in the application data directory.
Having this dependency does not remove the need to first delete the lucene folder.

@WolfSchlegel
Copy link
Contributor Author

WolfSchlegel commented May 13, 2020 •
edited

Ok, manually deleting the lucene folder is something we did not do.

Having this dependency does not remove the need to first delete the lucene folder.

What happens if the dependency is there AND folder is not deleted? The application starts and I would assume it continues to use the old lucene folder.

@dkayiwa
Copy link
Member

dkayiwa commented May 13, 2020

@WolfSchlegel i have tested again and confirmed that without deleting the lucene folder, i would need the lucene-backward-codecs dependency in order to upgrade from 2.2 to 2.4 :)

@WolfSchlegel
Copy link
Contributor Author

@dkayiwa that sounds good, so we have consistent behaviour.

Are you ok with leaving the lucene-backward-codecs dependency in place and remove it in the context of TRUNK-5731?

@dkayiwa
Copy link
Member

dkayiwa commented May 13, 2020

@WolfSchlegel yes, leaving the lucene-backward-codecs dependency in place, makes a lot of sense. 👍

@WolfSchlegel
Copy link
Contributor Author

Nice, it looks the end is near...

Did you figure out why the creation of the snapshots took so long on your machine?

@dkayiwa
Copy link
Member

dkayiwa commented May 13, 2020

I did not figure that out. But since you and colleagues reported a different result, and given the fact that running this snapshot creation process is rarely done, i ignored it. 😊

@WolfSchlegel
Copy link
Contributor Author

Cool. Anything else I should look into? Or are we done (I scarcely dare to ask...)

@dkayiwa
Copy link
Member

dkayiwa commented May 13, 2020

@WolfSchlegel did you say that you would rather the log level stay as INFO instead of ERROR?

@WolfSchlegel
Copy link
Contributor Author

Yes, for the time being and for the new class ChangeLogDetective only. That makes it easy for us to ask for startup logs when things misbehave in the wild.

As a follow up to this story I am planning to refactor the InitialisationFilter and UpdateFilter, when that is done we could reduce the log level.

How does that sound?

@ibacher
Copy link
Member

ibacher commented May 13, 2020

@WolfSchlegel Maybe we could just have the ChangeLogDetective log to the org.openmrs.api logger. A little bit hackish, but that's what most of the components that need to log INFO level messages do.

@dkayiwa
Copy link
Member

dkayiwa commented May 13, 2020

Sounds cool. 👍

@dkayiwa
Copy link
Member

dkayiwa commented May 13, 2020

Though i have no objection to @ibacher's proposal. 😊

@dkayiwa
Copy link
Member

dkayiwa commented May 13, 2020 •
edited

I just noticed that we already have this: https://github.com/openmrs/openmrs-core/blob/master/webapp/src/main/resources/log4j2.xml#L37

@ibacher
Copy link
Member

ibacher commented May 13, 2020

Heh... I forgot about that... That probably works then.

@dkayiwa
Copy link
Member

dkayiwa commented May 13, 2020

@WolfSchlegel do we still need one for ChangeLogDetective given this? https://github.com/openmrs/openmrs-core/blob/master/webapp/src/main/resources/log4j2.xml#L37

@WolfSchlegel
Copy link
Contributor Author

I would say yes, as liquibase is not the top level package of the class ChangeLogDetective.

Happy to change the logger to log to org.openmrs.api instead.

@dkayiwa
Copy link
Member

dkayiwa commented May 13, 2020

Ok you can go ahead and log to org.openmrs.api

@WolfSchlegel
TRUNK-4830: Add Liquibase Maven Plugin 3.8.9, introduce Liquibase sna…
9dc36ba 
…pshots for initialisation of OpenMRS database and upgrade liquibase-core to 3.8.9
@WolfSchlegel
Copy link
Contributor Author

Pushed the change, the class org.openmrs.liquibase.ChangeLogDetective is now logging under the alias org.openmrs.api.ChangeLogDetective.

Please let me know if anything else needs to be done.

If not I would like to thank you for a thorough and enjoyable review season, I learned quite a few things and look forward to the next ticket.

@dkayiwa dkayiwa merged commit 47463eb into openmrs:master May 13, 2020
@WolfSchlegel WolfSchlegel deleted the TRUNK-4830-N branch October 23, 2020 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkayiwa dkayiwa dkayiwa left review comments

@samuelmale samuelmale samuelmale left review comments

@ibacher ibacher ibacher left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@WolfSchlegel @coveralls @dkayiwa @ibacher @samuelmale