TRUNK-6155: Upgrade 2.7 Platform to Liquibase 4.8.0 #4401
Conversation
There was a problem hiding this comment.
@k4pran thanks for your work here. Take note of;-
- You added the wrong ticket ID, do you mind correcting that?
- The build is failing probably because of the big leap in versions, take a look at those please since alot could have happened in between the two versions.
My mistake I fixed the ticket number. I did some testing locally for the version update but seems the build is picking up some issues, will convert this PR to a draft while I look into it |
I fixed the issue and updated the description of this PR, there was a breaking change that wasn't documented in the github release notes. Seems to be building now. I'm not sure why it was passing locally though. Seemed to pass running mvn clean install, but failed when I ran the individual tests. |
For the integration tests which failed, you would need to run |
| @@ -42,14 +42,14 @@ | |||
| </changeSet> | |||
| <changeSet author="vshankar" id="201609171146-1"> | |||
| <preConditions onFail="MARK_RAN"> | |||
| <foreignKeyConstraintExists foreignKeyName="member_patient"/> | |||
| <foreignKeyConstraintExists foreignKeyTableName="cohort_member" foreignKeyName="member_patient"/> | |||
There was a problem hiding this comment.
@k4pran Is this required or just a hedge against the future? If it's required, we just need to ensure that we make similar updates to the modules as well.
There was a problem hiding this comment.
@k4pran Is this required or just a hedge against the future? If it's required, we just need to ensure that we make similar updates to the modules as well.
This would be required for integration tests to pass as some of them run through all the update xmls, liquibase now require the table to be referenced when setting up these constraints.
I didn’t realise other modules have update xmls but yes I think we should update them all. Do you know what modules will have them?
I guess another question, are there cases where clients using openmrs will have their own update xml changesets that we don’t have access to? I am just thinking if they have taken a snapshot that don’t have the foreignKeyTableName?
I also added a link to the breaking change in the PR description if you want to read a bit more about it
There was a problem hiding this comment.
Do you know what modules will have them?
Well, at least the modules turned up by this search
I guess another question, are there cases where clients using openmrs will have their own update xml changesets that we don’t have access to?
That's a real situation, but we can easily tell people who run into the issue how to fix it. Most modules (as opposed to core) rely on hand-maintained Liquibase scripts rather than snapshots.
I also added a link to the breaking change in the PR description if you want to read a bit more about it
Ah, I was under the impression that that change was is Liquibase 4.11, not 4.7. I see.
There was a problem hiding this comment.
Yea liquibase 4.11 has a few additional breaking changes and also a few code changes we will need. I haven't looked deeply into the additional breaking changes. I will update the constraints for those other repos too. Should it be new tickets for each repo?
There was a problem hiding this comment.
@k4pran Yeah, most modules have their own separate Jira project, so we should stick to that.
ibacher
changed the title
|
Thank you so much @k4pran for your work on this!! We're so thankful for your involvement and contributions. 💐 🏆 It's especially valuable and appreciated when folks help us with this kind of platform work. Can't thank you enough! |
Description of what I changed
Upgraded liquibase core and liquibase plugin to version 4.8.0. See the jira ticket below to review attached logs from manual testing.
There was a breaking change in version 4.7.0. This isn't shown in the github release notes but you can find them here - https://docs.liquibase.com/start/release-notes/home.html. See below
Breaking Changes
[PR#2228] [Nathan Voxland] Handle the fact that FK names are not always unique. Fixes PR#2227. When using the foreignKeyConstraintExists precondition, you must now specify foreignKeyTableName. See also: PR#2565 fixing #2389. in 4.11.0.
I added the table name to the constraints to fix this breaking change.
Issue I worked on
see https://issues.openmrs.org/browse/TRUNK-6155
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