Conversation
ewaterman
added
the
warn: do not merge
github-actions
bot
added
type: maintenance
|
@jmiranda take a look at this when you get a chance. I've only tested the happy path on my local so it's not ready to merge yet but I wanted to get something into review so that we could talk about it. If we like it, and want to move forward with a change like this, we can open it up to the rest of the team and start testing it more properly. |
| * Represents an app version from the perspective of a tagged liquibase changeSet. | ||
| * By convention these should map to directories under the grails-app/migrations folder (Ex: '0.9.x'). | ||
| */ | ||
| class TaggedMigrationVersion implements Comparable<TaggedMigrationVersion> { |
There was a problem hiding this comment.
This is just a convenience class to make comparing versions easier
|
okay, I see that this is failing for integration tests with: I'll try to figure out why migrations aren't running properly for test containers but we can still review this change as normal for now. Maybe it just needs the following config: |
| * List all of our tagged migration versions. Any new tagged versions should be added to this list. | ||
| * By convention, these should map to the migrations directories. | ||
| */ | ||
| final List<TaggedMigrationVersion> ALL_VERSIONS = [ |
There was a problem hiding this comment.
I thought about having it read all the folder names in the /migrations folder to extract these versions so that we wouldn't need to update this list with every release but that felt pretty hacky so I left it like this. We're already expecting that specific folder structure though so maybe it's fine to do.
jmiranda
changed the title
ewaterman
removed
the
warn: do not merge
ewaterman
requested review from
alannadolny,
awalkowiak,
drodzewicz and
kchelstowski
|
I've tested this change on my local 0.9.x setup and on clean installs via integration tests. It could be useful to further test this for 0.8.x installs just to be extra safe though |
|
I promise I'll get back to reviewing this one soon. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4860 +/- ##
============================================
+ Coverage 7.67% 7.73% +0.05%
- Complexity 834 854 +20
============================================
Files 605 610 +5
Lines 42451 42581 +130
Branches 10308 10343 +35
============================================
+ Hits 3260 3292 +32
- Misses 38711 38802 +91
- Partials 480 487 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I encountered a weird issue when running this against a 0.8.x database. Upon further investigation, it turns out the database was not getting restored completely as the mysql import was stopping around the shipment tables. I hadn't noticed it the first time around, but upon re-running the restore process off the demo database dump I saw the following error. For context, I'm running the mysqldump on MySQL 8 and the MySQL import on MariaDB, so there's something wonky between the two. However, the weird thing is that none of our tables should be using utf8mb4 since we always default to utf8 which is a pointer to utf8mb3. Long story short, I need a bit more time to debug this. At the very least the changelog seems to be executing the expected versions, so I think once I think we'll be good once I get passed this. One caveat is that I was hoping to see the same logging from before (in particular, the lines that output executeChangelogVersions and upgradeChangelogVersions). Is it possible to restore those? It's just helpful for debugging in case we encounter issues in the future. You can make them DEBUG if necessary. |
|
@jmiranda first off, thanks for testing these thoroughly. Some of the log statements are necessarily gone because I moved the logic from BootStrap.groovy to inside the root changelog.groovy itself, which I don't think I can log from (I'm actually not certain about that). I can definitely log before and after the migrations run, so I'll try to add some more helpful log statements there at least. They shouldn't be too bloated so I don't mind them staying as INFO. EDIT: okay it does seem like I can at least print to the console while inside the changelog so there are options. Obviously not ideal (a proper log would be better) but it works |
|
@jmiranda I updated the logs to the following. Turns out the changelog can in fact log properly: |
jmiranda
left a comment
jmiranda
left a comment
There was a problem hiding this comment.
I finally tested the three cases I wanted to cover
- empty database
- existing 0.8.x database
- existing 0.9.x database
... and everything worked as expected.
|
With respect to the issue I encountered with the 0.8.x database, it looks like MySQL 8 set the character set to utf8mb4 for one table on the demo server. I'm not sure how that happens, but will check to see if obdev1 and/or any of the other 0.8.x databases to see if they have any tables with the wrong character set. |
ewaterman
deleted the
maintenance/OBPIH-6740-unify-liquibase-changelog
branch
2 participants
✨ Description of Change
Link to GitHub issue or Jira ticket: https://pihemr.atlassian.net/browse/OBPIH-6740
Description: There are more details in the ticket, but essentially this change moves the migration logic that we had in Bootstrap into the entrypoint changelog. I've also taken the chance to clean up how we call our clean install migrations vs regular upgrade migrations to hopefully simplify the process somewhat. This approach lets us take advantage of the migration plugin's properties (which will be convenient for tests to be able to override the changelog, providing its own that will insert test data). For example:
You'll also notice that I added a 'final' tagged changeset to each release folder. We don't strictly need to use tags for this change to work, but it is a more proper solution in my opinion. It's easy enough to revert though if we choose not to use them.