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

7256 purge referencedata #7355

Merged
merged 8 commits into from
Nov 16, 2020
Merged

Conversation

poikilotherm
Copy link
Contributor

@poikilotherm poikilotherm commented Oct 23, 2020

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 closes relates 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.

@poikilotherm poikilotherm added Type: Feature a feature request Component: Code Infrastructure formerly "Feature: Code Infrastructure" Feature: Installer Feature: Installation Guide User Role: Sysadmin Installs, upgrades, and configures the system, connects via ssh labels Oct 23, 2020
@poikilotherm poikilotherm self-assigned this Oct 23, 2020
@poikilotherm
Copy link
Contributor Author

Hey @landreev @sekmiller @pdurbin I'd love some chatter on this, as you guys crafted the original script.

@poikilotherm
Copy link
Contributor Author

I think this is ready for review. It's narrow scoped. More stuff about bootstrapping to come later.

@poikilotherm poikilotherm marked this pull request as ready for review November 6, 2020 15:47
@coveralls
Copy link

coveralls commented Nov 6, 2020

Coverage Status

Coverage remained the same at 19.479% when pulling 422bc26 on poikilotherm:7256-purge-referencedata into fe9e24d on IQSS:develop.

Copy link
Contributor

@scolapasta scolapasta left a 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?

@landreev
Copy link
Contributor

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?

@poikilotherm
Copy link
Contributor Author

@scolapasta
Actually it's only one new with a to date unknown filename schema: afterMigrate__1-upsert-referenceData.sql.

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 V5.1.1.3__7256-purge-referencedata.sql should be updated to reflect the recent 5.2 release, the files were created before that.

@landreev
No checksum magic here. The file content is simply ignored AFAIK when the migration is already applied. At least it has been ignored when I tried to insert indexes with it... (see fc3dbb6)

@scolapasta
Copy link
Contributor

@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 :)

@poikilotherm
Copy link
Contributor Author

Sure!
So you folks think this is the way to go? Should we talk about it some more? Or just update and push to QA then?

@scolapasta
Copy link
Contributor

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.

@poikilotherm
Copy link
Contributor Author

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

@kcondon kcondon self-assigned this Nov 16, 2020
@kcondon kcondon merged commit 9cce13d into IQSS:develop Nov 16, 2020
@poikilotherm poikilotherm deleted the 7256-purge-referencedata branch November 16, 2020 21:56
@poikilotherm
Copy link
Contributor Author

Thanks for merging @kcondon! 🥳

@djbrooke djbrooke added this to the 5.3 milestone Nov 17, 2020
This was referenced Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Code Infrastructure formerly "Feature: Code Infrastructure" Feature: Installation Guide Feature: Installer Type: Feature a feature request User Role: Sysadmin Installs, upgrades, and configures the system, connects via ssh
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants