-
Notifications
You must be signed in to change notification settings - Fork 494
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
7256 purge referencedata #7355
7256 purge referencedata #7355
Conversation
Hey @landreev @sekmiller @pdurbin I'd love some chatter on this, as you guys crafted the original script. |
…will be applied in order (cannot create an older, out-of-order version as this would break migrations for everyone). IQSS#7256
8993a35
to
5132e04
Compare
I think this is ready for review. It's narrow scoped. More stuff about bootstrapping to come later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you explain the names of the two new files?
I noticed that you are adding a couple of comments to the V1__flyway_schema_baseline.sql. I'm just curious, why is it NOT causing a problem? - Is that because Flyway purposefully does not save the checksum for the baseline file? |
@scolapasta This is simply a script executed every time after a successfull migration happened (so this will be executed everytime a new DB migration happens). Please see flyway callback docs for more details. It will not cause trouble because precautions are in place to only insert data if the DB is empty (during bootstrap). In the future, more scripts can be added with incremented numbers. The name of @landreev |
@poikilotherm ah gotcha. the 5.1.1.3 confused me. Can you update? And I had not head the term "upsert" before, so didn't know what that meant (now I do :) |
Sure! |
Seems reasonable enough to me and I know @landreev took a look, as well. @poikilotherm if you feel it needs any more discussion, we can include as part of the Tuesday meeting. |
@scolapasta I updated the files. I'll take the liberty to move to QA, as y'all were happy with code review. Please revert if any concerns arise with that. |
Thanks for merging @kcondon! 🥳 |
What this PR does / why we need it:
This is a first step to ease deployments by making parts of the installer unnecessary.
One step during install is loading the SQL script
reference_data.sql
. I'd replace it by Flyway migrations & callback actions.That way, the database related script is streamlined with the other database operations plus bootstrapping gets easier (no more installing Python Postgres nor
psql
in containers necessary).Which issue(s) this PR
closesrelates to:Related to #7256 (closed, but that set the trigger)
Related to #5361 (this PR is only a partial solution, so don't close)
Special notes for your reviewer:
Nada.
Suggestions on how to test this:
Deploy a fresh installation and check if indexes and initial data are still present.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Nope.
Is there a release notes update needed for this change?:
I don't think so.
Additional documentation:
Nope.