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.

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

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

Closed
wants to merge 18 commits into from

Conversation

WolfSchlegel
Copy link
Contributor

Description

Related Issue

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

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

try {
upgradeTestUtil.upgrade("liquibase-master.xml");
} catch (Exception e) {
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

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

Did you just forget to remove the above printStackTrace? :)

Copy link
Member

Choose a reason for hiding this comment

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

Also do not catch the exception. It's better to fail with an exception and not Assert.fail()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@rkorytkowski
Copy link
Member

rkorytkowski commented Jun 10, 2016

You haven't modified any code, which is run in production... just tests, thus only liquibase-update-to-latest.xml is still used. In production if you have unrun changesets they are applied on server restart. I think you will need to modify some logic in https://github.com/openmrs/openmrs-core/blob/master/web/src/main/java/org/openmrs/web/filter/update/UpdateFilter.java to run liquibase-master.xml

referencedColumnNames="user_id"/>
</changeSet>

</databaseChangeLog>
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had put changes prior to and including 2.1 into the file and changes after 2.1 into liquibase-update-to-latest.xml. Happy to reshuffle changes but I would need some help here what should go where. Can we have a quick chat some time so that I understand the constraints you mentioned better?

@dkayiwa
Copy link
Member

dkayiwa commented Aug 3, 2016

@WolfSchlegel thanks for working on this! 👍
Do you plan to have any more cycles on this in the near future? :)

dkayiwa and others added 17 commits August 7, 2016 13:56
properties file by using it as specified instead of modifying it
to avoid having to restart the servlet container to see the login page
ModuleFileParser is missing 1.5 and 1.6 as valid config versions.
HibernateOrderDAO has a whitespace in one of its exceptions message codes in
getNextOrderNumberSeedSequenceValue

* remove whitespace in message code string
* remove unused code in getNextOrderNumberSeedSequenceValue
Add unit tests for ComplexObsHandler.getSupportedViews() and
ComplexObsHandler.supportsView(), as discussed in review notes
* add Order.isActivated()
* add Order.isActivated(Date)
* fix issues introduced by switching isFuture with isStarted/!isStarted for
orders scheduled in the future
** Order.isActive(Date) of stopped orders scheduled for the future were
considered active
** Order.isDiscontinued(Date) of stopped orders scheduled for the future which
were not considered discontinued
* add OrderService tests to ensure we can revise orders scheduled in the
future and that the original orders are considered discontinued
* add OrderService tests to ensure we can discontinue orders scheduled in the
future
@dkayiwa
Copy link
Member

dkayiwa commented Aug 8, 2016

@WolfSchlegel this branch has conflicts. Did you forget to pull the latest upstream changes? :)

@dkayiwa
Copy link
Member

dkayiwa commented Sep 27, 2016

Closing in favour of #1841

@dkayiwa dkayiwa closed this Sep 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants