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

Fix revision scripts, run migrations in CI, add repair option, improve migrations utils #15811

Merged
merged 16 commits into from Apr 26, 2023

Conversation

jdavcs
Copy link
Member

@jdavcs jdavcs commented Mar 15, 2023

Builds on top of #15876.
Ref #15746, #15772

Some revision scripts are currently were broken, both for postgresql and sqlite. Running ./manage_db.sh downgrade base and/or ./manage_db.sh upgrade (from base) raises would raise errors. There are were multiple causes, all of which are resolved in this PR (see individual commits for more details)

Notes

This builds on top of #15876. First new commit is "Add db migrate test to postgres workflow".

I've added a unit test that applies all revisions (head >> base >> head >> base). See docstring for details
The test will be executed as part of the unit-postgres.yaml github workflow, twice: on sqlite and postgres (locally it'll run on sqlite and on postgres if it's available). In addition to verifying that all revisions can be applied w/o error, this test will also fail when a revision has been added, but the model has not been updated (provided the downgrade script reverses the changes specified in the upgrade script).

I've added a --repair option for the db_dev script's upgrade/downgrade commands. Setting the option will skip revisions that conflict with the state of the database (see help text). The option is not available on the user-facing manage_db.sh script.

The transaction context manager wraps multiple DDL statements ensuring transactional control when running under SQLite (more context: https://bugs.python.org/issue10740)

The legacy_alter_table context manager prevents the error raised when a column belongs to a table referenced in a database view (without this, we can't downgrade to base on SQLite). (more context: sqlalchemy/alembic#1207)

How to test the changes?

  • I've included some automated tests. More tests are needed for the model.migrations.util module; however, those require additional testing infrastructure that's outside of the scope of this PR.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
  1. Start with an up-to-date database.
  2. Run ./manage_db.sh downgrade base; ./manage_db.sh upgrade. Observe no error.
  3. Comment out the statements in a revision's upgrade OR downgrade function (I tried revision 6a67bf27e6a6)
  4. Repeat step 2,.Depending on what you commended out, you may need to run the commands TWICE to trigger the error. Observe the error.
  5. Uncomment what you commented in state 3. Repeat step 4 - you still have an error - the database is broken!
    No need to fear, --repair is here!
  6. Run /scripts/db_dev.sh downgrade base --repair; /scripts/db_dev.sh upgrade --repair.
  7. Now repeat step 2. Observe no error. You've fixed your database!
  8. Repeat all steps using postgresql or sqlite, so you try both databases.

Note: For step 2, I didn't try breaking all revisions, so it still might be possible to break things beyond what --repair can handle. But it does handle the obvious/typical cases.

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@github-actions github-actions bot added area/database Galaxy's database or data access layer area/scripts area/testing labels Mar 15, 2023
@jdavcs jdavcs force-pushed the dev_migrations_cleanup1 branch 4 times, most recently from 25d9d85 to a2aa17a Compare April 13, 2023 22:55
@jdavcs jdavcs changed the title [WIP] Cleanup migrations Fix revision scripts, run migrations in CI, add --repair option, misc. improvements to migrations util module Apr 14, 2023
@jdavcs jdavcs changed the title Fix revision scripts, run migrations in CI, add --repair option, misc. improvements to migrations util module Fix revision scripts, run migrations in CI, add repair option, improve migrations utils Apr 14, 2023
@jdavcs jdavcs marked this pull request as ready for review April 14, 2023 00:26
@github-actions github-actions bot added this to the 23.1 milestone Apr 14, 2023
@jdavcs
Copy link
Member Author

jdavcs commented Apr 14, 2023

Selenium test failures unrelated.

@jdavcs
Copy link
Member Author

jdavcs commented Apr 25, 2023

I've checked - there should be no conflicts after #15876 is merged.

Wrap multiple sql statements in a transaction to ensure per-revision
transactional control.
WIP: Lots of redundant logic here. Needs refactoring.
Factor out reduntant logic into template methods
Add type hints, docstrings
Misc. cleanup
@jdavcs
Copy link
Member Author

jdavcs commented Apr 26, 2023

Test failures unrelated.

@mvdbeek mvdbeek merged commit de488f1 into galaxyproject:dev Apr 26, 2023
35 of 39 checks passed
@jmchilton
Copy link
Member

Very nice - thanks for awesome work and thanks for beating me to review @mvdbeek!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/database Galaxy's database or data access layer area/scripts area/testing kind/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants