TRUNK-3638 Separate liquibase file into separate files per version #1841

WolfSchlegel · 2016-09-21T13:42:20Z

Description

This pull request implements most of your feedback on #1804.

The following feedback still needs to be resolved:

"To me it seems that all those changes happened in 2.0 and not 2.1. It's what the pull request title suggests (I may be wrong here so please reassure me). Since 2.0 has been released in beta already we cannot move those changesets out of liquibase-update-to-latest.xml. This file should really contain only changesets introduced in 2.1 (probably none at the moment). ... Actually let me answer myself. They happened in 1.12.x and have been released in 1.12.0. It means we cannot move them out of liquibase-update-to-latest.xml and this file should be empty initially."

Could you please provide some guidance which changes to put into which liquibase files?

Related Issue

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

Checklist:

My pull request only contains one single commit.
My pull request is based on the latest master branch
git pull --rebase upstream master.
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.
My code follows the code style of this project.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

…to liquibase-update-2.1.xml

mention-bot · 2016-09-21T13:42:21Z

@WolfSchlegel, thanks for your PR! By analyzing the annotation information on this pull request, we identified @bmamlin, @wluyima and @suniala to be potential reviewers

dkayiwa · 2016-09-22T08:43:12Z

@WolfSchlegel since you opened this new pull request, should we close the other one? #1804

WolfSchlegel · 2016-09-27T08:43:35Z

Yes, this pull request supersedes #1804.

dkayiwa · 2016-09-27T17:55:35Z

@WolfSchlegel because all the 2.1 changes, so far, are also in 2.0, you are correct to say that for now, liquibase-update-2.1.xml will be empty. So yes none is moving out of liquibase-update-to-latest.xml
Therefore, create liquibase-master.xml and in it, reference these two files.
Is this enough for you to finish this up? :)

I also somehow think that we need to do something about liquibase-schema-only.xml along the same lines as we are doing for liquibase-update-to-latest.xml

dkayiwa · 2016-09-27T18:18:13Z

api/src/main/java/org/openmrs/util/DatabaseUpdater.java

@@ -58,7 +58,7 @@
 /**
 * This class uses Liquibase to update the database. <br>
 * <br>
- * See /metadata/model/liquibase-update-to-latest.xml for the changes. This class will also run
+ * See /metadata/model/liquibase-master.xml for the changes. This class will also run


The folder containing this is no longer metadata/model :)

dkayiwa · 2016-09-27T18:20:40Z

api/src/main/resources/liquibase-master.xml

+        xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-2.0.xsd
+        http://www.liquibase.org/xml/ns/dbchangelog-ext http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-ext.xsd">
+
+    <include file="liquibase-update-2.1.xml"/>


Shouldn't this order be reversed? What is the order of execution of the files listed here?

Quite right, given that the liquibase-update-2.1.xml file is currently empty. I renamed the files to

liquibase-update-baseline.xml - contains the historic baseline

liquibase-update-to-latest.xml - starts as an empty file. At some point in time you would close the baseline and add any changes either to this file or to another file referring to a particular version.

Please let me know if that makes sense.

coveralls · 2016-10-11T19:55:29Z

Coverage decreased (-0.1%) to 55.13% when pulling 29882a5 on WolfSchlegel:TRUNK-3638-B into 1f3727b on openmrs:master.

WolfSchlegel · 2016-10-11T20:02:32Z

Regarding liquibase-schema-only.xml, please let me know your requirements. Would it be the same exercise, i.e. introduce a master file that includes multiple files?

dkayiwa · 2016-10-12T10:53:44Z

api/src/main/resources/liquibase-update-update-to-latest.xml

+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+    This Source Code Form is subject to the terms of the Mozilla Public License,


Is this file name correct? :)

It depends, please have a look at my other comment on renaming files.

@WolfSchlegel i looked at the comments but i still do not yet understand the need to have "update" twice in the file name. :)

wluyima · 2016-10-14T14:12:26Z

api/src/main/java/org/openmrs/util/DatabaseUpdater.java

@@ -68,7 +68,7 @@

 	private static final Log log = LogFactory.getLog(DatabaseUpdater.class);

-	private static final String CHANGE_LOG_FILE = "liquibase-update-to-latest.xml";
+	private static final String CHANGE_LOG_FILE = "liquibase-master.xml";


Since this is really still an update file, it would be nice for it to contain the word update,
how about liquibase-update-to-latest-master.xml? I'll update the ticket

I have updated the ticket description, we don't need to rename the liquibase-update-to-lastest.xml fie

wluyima · 2016-10-14T14:13:33Z

api/src/main/resources/liquibase-master.xml

+        http://www.liquibase.org/xml/ns/dbchangelog-ext http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-ext.xsd">
+
+    <include file="liquibase-update-baseline.xml"/>
+    <include file="liquibase-update-to-latest.xml"/>


Thought this was renamed

wluyima · 2016-10-14T14:16:08Z

api/src/main/resources/liquibase-master.xml

+        xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-2.0.xsd
+        http://www.liquibase.org/xml/ns/dbchangelog-ext http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-ext.xsd">
+
+    <include file="liquibase-update-baseline.xml"/>


If we're doing this in 2.1, this should be probably named liquibase-update-2.0.xml

coveralls · 2016-10-28T07:16:26Z

Coverage increased (+0.1%) to 55.407% when pulling 111d8a7 on WolfSchlegel:TRUNK-3638-B into 1f3727b on openmrs:master.

dkayiwa · 2016-10-28T14:14:05Z

@WolfSchlegel did you pull the latest upstream changes before making another commit for this? :)

…to liquibase-update-2.1.xml

WolfSchlegel · 2016-11-07T20:42:45Z

The files containing change sets have been renamed
liquibase-update-to-2.0.xml contains the bulk of the changes that were previously in liquibase-update-to-latest.xml
liquibase-update-to-2.1.xml contains a single changeset

coveralls · 2016-11-07T20:56:20Z

Coverage increased (+0.02%) to 55.635% when pulling a34a020 on WolfSchlegel:TRUNK-3638-B into 17c22ce on openmrs:master.

dkayiwa · 2016-11-14T08:07:33Z

@WolfSchlegel i tried testing your changes by doing a fresh openmrs setup but got the error message below:

There was an error while updating the database to the latest. file: liquibase-update-to-latest.xml. Error: Migration failed for change set liquibase-update-to-2.0.xml::20090225-1551::dthomas: Reason: liquibase.exception.DatabaseException: Error executing SQL ALTER TABLE location_tag CHANGE retired_reason retire_reason varchar(255): Unknown column 'retired_reason' in 'location_tag': Caused By: Error executing SQL ALTER TABLE location_tag CHANGE retired_reason retire_reason varchar(255): Unknown column 'retired_reason' in 'location_tag': Caused By: Unknown column 'retired_reason' in 'location_tag' Error while trying to update to the latest database version

Does it work on your side?

dkayiwa · 2016-11-14T10:20:28Z

Since your changes look obviously correct, i tried to look into the cause of this failure. I discovered that the changeset id is inserted in liquibase-schema-only.sql and hence is supposed to be marked as run. This was not the case and i found out that it's because liquibase does not use the changeset id alone to skip a changeset, but also the name of the file in which the changeset belongs. So this https://github.com/openmrs/openmrs-core/blob/master/api/src/main/resources/liquibase-schema-only.xml#L3455 needs to be changed to the name of the file where the changeset currently resides, and that is, liquibase-update-to-2.0.xml

dkayiwa mentioned this pull request Sep 27, 2016

move changes since version 2.0.x from liquibase-update-to-latest.xml … #1804

Closed

7 tasks

dkayiwa reviewed Sep 27, 2016

View reviewed changes

WolfSchlegel force-pushed the TRUNK-3638-B branch from a799656 to 29882a5 Compare October 11, 2016 19:45

dkayiwa reviewed Oct 12, 2016

View reviewed changes

wluyima reviewed Oct 14, 2016

View reviewed changes

move changes since version 2.0.x from liquibase-update-to-latest.xml …

a34a020

…to liquibase-update-2.1.xml

WolfSchlegel force-pushed the TRUNK-3638-B branch from 111d8a7 to a34a020 Compare November 7, 2016 20:37

dkayiwa merged commit 9a05f6c into openmrs:master Nov 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TRUNK-3638 Separate liquibase file into separate files per version #1841

TRUNK-3638 Separate liquibase file into separate files per version #1841

WolfSchlegel commented Sep 21, 2016

mention-bot commented Sep 21, 2016

dkayiwa commented Sep 22, 2016

WolfSchlegel commented Sep 27, 2016

dkayiwa commented Sep 27, 2016 •

edited

dkayiwa Sep 27, 2016 •

edited

WolfSchlegel Oct 11, 2016

dkayiwa Sep 27, 2016

WolfSchlegel Oct 11, 2016

coveralls commented Oct 11, 2016

WolfSchlegel commented Oct 11, 2016

dkayiwa Oct 12, 2016

WolfSchlegel Oct 12, 2016

dkayiwa Oct 14, 2016

wluyima Oct 14, 2016 •

edited

wluyima Oct 14, 2016

wluyima Oct 14, 2016

wluyima Oct 14, 2016

coveralls commented Oct 28, 2016

dkayiwa commented Oct 28, 2016

WolfSchlegel commented Nov 7, 2016

coveralls commented Nov 7, 2016

dkayiwa commented Nov 14, 2016

dkayiwa commented Nov 14, 2016 •

edited

TRUNK-3638 Separate liquibase file into separate files per version #1841

TRUNK-3638 Separate liquibase file into separate files per version #1841

Conversation

WolfSchlegel commented Sep 21, 2016

Description

Related Issue

Checklist:

mention-bot commented Sep 21, 2016

dkayiwa commented Sep 22, 2016

WolfSchlegel commented Sep 27, 2016

dkayiwa commented Sep 27, 2016 • edited

dkayiwa Sep 27, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 11, 2016

WolfSchlegel commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wluyima Oct 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 28, 2016

dkayiwa commented Oct 28, 2016

WolfSchlegel commented Nov 7, 2016

coveralls commented Nov 7, 2016

dkayiwa commented Nov 14, 2016

dkayiwa commented Nov 14, 2016 • edited

dkayiwa commented Sep 27, 2016 •

edited

dkayiwa Sep 27, 2016 •

edited

wluyima Oct 14, 2016 •

edited

dkayiwa commented Nov 14, 2016 •

edited