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

Added extension point to manage database migrations #292

Merged
merged 8 commits into from
Mar 26, 2020

Conversation

puehringer
Copy link
Contributor

@puehringer puehringer commented Jan 16, 2020

Summary

This PR adds the ability to tightly integrate Alembic migrations into tdp_core applications.

How it works

On startup, the DBMigrationManager retrieves all extension points with the key tdp-sql-database-migration and creates a DBMigration object for each them. This procedure is very similar to how DBConnector are loaded. A DBMigration then can execute all Alembic commands (upgrade, downgrade, history, stamp, ...) with all options within the context of a specific migration.

How to use

The documentation moved to https://wiki.datavisyn.io/tdp/extension-points/db-migration

@puehringer puehringer force-pushed the mp/291_db_migration branch 2 times, most recently from 7653b28 to 7d8b3e4 Compare January 30, 2020 11:15
@puehringer puehringer marked this pull request as ready for review January 30, 2020 13:52
Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

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

Thanks for your implementation and the good documentation! 👍 For me it works as described.

@puehringer Please add an according minor or major release label to the PR.

@lehnerchristian Please have a look if the implementation is fine for you.

Copy link

@lehnerchristian lehnerchristian 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. however I haven't tested it yet

@lehnerchristian
Copy link

After the demo yesterday I think we can merge. @thinkh you made some notes as far as I saw. do you want to copy them over?

@thinkh
Copy link
Member

thinkh commented Mar 4, 2020

I did not take notes, since @puehringer rembembered the to dos.

@puehringer Your turn. 😉

@puehringer
Copy link
Contributor Author

puehringer commented Mar 4, 2020

  • Add possibility to configure schema where the alembic_version table is stored
  • Check commands which require -arg (especially revision -m "...")

@thinkh Of course I remembered everything 😉

@puehringer puehringer force-pushed the mp/291_db_migration branch from 4e2ee7a to 3d83fb6 Compare March 23, 2020 17:48
@puehringer
Copy link
Contributor Author

@thinkh @lehnerchristian The two remaining issues are solved and documented in the wiki. This PR is therefore ready to merge.

@puehringer puehringer force-pushed the mp/291_db_migration branch from 1e64239 to c8740ae Compare March 23, 2020 18:16
@puehringer puehringer force-pushed the mp/291_db_migration branch from c8740ae to 7dad884 Compare March 23, 2020 18:20
@lehnerchristian
Copy link

one thing I noticed in the docs: in https://wiki.datavisyn.io/tdp/extension-points/db-migration#initializing-the-migrations-folder you're updating the access rights for the new migration folder

the command you use is sudo chown $USER:$USER -R alembic alembic.ini

however it should be sudo chown $USER:$USER -R migration alembic.ini, shouldn't it?

@puehringer
Copy link
Contributor Author

@lehnerchristian I changed it to sudo chown $USER:$USER -R migration, as the ini is supplied by tdp_core#dbmigration.ini. Thanks for the hint!

@lehnerchristian lehnerchristian merged commit 2d6b952 into develop Mar 26, 2020
@lehnerchristian lehnerchristian deleted the mp/291_db_migration branch March 26, 2020 09:14
thinkh added a commit to Caleydo/tdp_publicdb that referenced this pull request Apr 21, 2020
The required PR datavisyn/tdp_core#292 was merged into the develop branch.
@thinkh thinkh mentioned this pull request Jul 30, 2020
48 tasks
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.

3 participants