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

OBGM-130 Refactor database migration trigger in BootStrap.groovy to avoid having to update list of existing versions #4391

Merged
merged 6 commits into from
Dec 8, 2023

Conversation

alannadolny
Copy link
Collaborator

The description / Question is moved to the ticket

@alannadolny alannadolny self-assigned this Nov 24, 2023
@alannadolny alannadolny marked this pull request as draft November 24, 2023 15:48
@alannadolny alannadolny marked this pull request as ready for review November 28, 2023 13:06
Copy link
Member

@jmiranda jmiranda left a comment

Choose a reason for hiding this comment

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

Let's hold off on this one until we have a chance to tech huddle it.

grails-app/init/org/pih/warehouse/BootStrap.groovy Outdated Show resolved Hide resolved
Copy link
Member

@jmiranda jmiranda left a comment

Choose a reason for hiding this comment

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

Looks good to me. Great find on that API, @alannadolny. Minor changes requested.

// Find directories with names matching current versions pattern
List<String> changelogVersions = resources
.collect { it.filename }
.findAll { it.matches("[0-9]{1,3}.[0-9]{1,3}.x") }
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add a comment to describe the regexp being used? Something like

// Find directories with names matching current versions pattern which 
// will match to any version number between 0.0.x to 999.999.x

Also let's replace [0-9] with

.findAll { it.matches("[[:digit:]]{1,3}.[[:digit:]]{1,3}.x") }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's interesting, because [[:digit:]] seems to be not working 🤔
image
neither with single [:
image
but it works with \d and just [0-9]:
image

Copy link
Member

Choose a reason for hiding this comment

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

Let's use \d then.

grails-app/init/org/pih/warehouse/BootStrap.groovy Outdated Show resolved Hide resolved
Copy link
Member

@jmiranda jmiranda left a comment

Choose a reason for hiding this comment

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

One more thing to test (see below).

.reverse()

// Exclude the newest changelog version, this one should be run separately
List<String> previousChangelogVersions = changelogVersions.size() ? changelogVersions.tail() : []
Copy link
Member

Choose a reason for hiding this comment

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

Rename all the directories under migrations so nothing matches and see what happens. I want to check whether we need to add null safety.

If you need to change the code, also replace

changelogVersions.size() 

with

!changelogVersions.empty

just to make it more readable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

.size was working, but I changed this method to .empty to increase readability

@jmiranda
Copy link
Member

jmiranda commented Dec 1, 2023

Also, fix the conflict.

Copy link
Collaborator

@awalkowiak awalkowiak left a comment

Choose a reason for hiding this comment

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

While working on other tickets related to db changelogs I noticed, that if you run the install changelogs on fresh db, and then current 0.9.0 changelogs, then on each app start bootstrap thinks that previous changelogs were not yet executed and will try to again install db from scratch. I think we need to expand the logic here to cover or look into either if there were any install changelogs executed or any current version changelogs executed, and based on that execute proper changelogs.

(part of the issues I had were caused after changing a branch and having different changeset IDs in the install/changelogs..., so normally it should be no problem to rerun install changelogs because those executed would be skipped, so treat this rather as a "comment", not necessarily as a "change request", but I'd like to discuss it. Additionally, this ticket: https://pihemr.atlassian.net/browse/OBGM-133 might be more suitable for this)

@awalkowiak awalkowiak merged commit 93fa6b1 into feature/upgrade-to-grails-3.3.10 Dec 8, 2023
1 check passed
@awalkowiak awalkowiak deleted the OBGM-130 branch December 8, 2023 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants