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.

Sign up for GitHub

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

Jump to bottom

[WIP] Alembic #12869

Closed
wants to merge 44 commits into from
Closed

[WIP] Alembic #12869

wants to merge 44 commits into from

Conversation

jdavcs
Copy link
Member

@jdavcs jdavcs commented Nov 8, 2021 •
edited
Loading

UPDATE 12/30/21: everything works, tests pass. I'll open this for review after some minor cleanup and adding a few missing tests.

Ref #10369
Supersedes #11060.

To do:

  • Add alembic to dependencies
  • Setup migration infrastructure for galaxy and tool_shed_install models (combined and separate)
  • Write tests for different database states (see comments in conftest.py)
  • Resolve AttributeError during test collection for package tests (env.py raises the error, path to test version locations resolved incorrectly; also ignore alembic directory when collecting tests)
  • Modify tests to allow both sqlite and postgres as the test database (postgres is essential to expose bugs invisible under sqlite)
    (e.g. sqlite handles dropping nonexistant db objects differently 66b2c83; automatically creates new db from path)
  • Ensure tests use postgres in CI workflows (trying separate workflow? Connection, test databases different from api/integration tests. Alternative: add service, env var to unit tests workflow, passenv to tox.ini)
  • Fix bug: we can't load db state if postgres db does not exist
  • Integrate into config
  • Move triggers out of migrate/ (must be removed before we can remove the migrate directory)
  • Updata galaxy-data package on pypi to reflect the new location of triggers at model/triggers
  • Edit scripts for manual upgrades/downgrades
  • Ensure alembic works seamlessly as standalone scripts
  • Add tests for verifying behavior when invoked via script
  • Solve mypy error mystery 9311f0b (at least verify it's not masking a real bug)
  • Add type hints
  • Address all TODOs in new code (proper log messages, minor cleanup, etc.)
  • Provide script for ToolShed migrations (previous version of manage_db.[sh|py])
  • Remove tool migrations (done in Drop tool migration scripts and related code #13099)
  • Review + resolve + remove all other references to SQLAlchemy Migrate from code base
  • Cleanup commit history; simplify review

Nest steps (not this PR):

  • Write a description of how the new Alembic-based migration system works
  • Write documentation on how to modify the model (model + test + alembic revision)
  • Add integration test(s) verifying upgrade/downgrade (ideally, triggered by each new revision?)
  • Verify all new code is SQLAlchemy 2.0-compatible

How to test the changes?

(Select all options that apply)

License

  • I agree to license these contributions under Galaxy's current license.
  • I agree to allow the Galaxy committers to license these and all my past contributions to the core galaxy codebase under the MIT license. If this condition is an issue, uncheck and just let us know why with an e-mail to [email protected].

@jdavcs jdavcs added this to the 22.01 milestone Nov 8, 2021
@jdavcs jdavcs mentioned this pull request Nov 9, 2021
Closed
@jdavcs jdavcs force-pushed the dev_alembic12 branch 2 times, most recently from 928434a to 5ce4492 Compare November 9, 2021 00:24
@jdavcs jdavcs mentioned this pull request Nov 18, 2021
Merged
3 tasks
@jdavcs jdavcs force-pushed the dev_alembic12 branch 6 times, most recently from 652d2e4 to ac37bd9 Compare November 30, 2021 20:33
@jdavcs jdavcs force-pushed the dev_alembic12 branch 7 times, most recently from d77f287 to c181ef1 Compare December 7, 2021 15:22
jdavcs added 16 commits December 21, 2021 14:50
@jdavcs
Use database_auto_migrate config setting
c57675e
@jdavcs
Do not load multiple metadata objects; drop test
aed5390 
No longer needed with current implementation.
@jdavcs
Set 179 as last sqlalchemy migrate version
019f63e
@jdavcs
Remove migrations test from first starup workflow
bf927a1 
This tests sqlalchemy migrate which is being replaced.
@jdavcs
WIP: Rewrite tests to allow postgres as test db
bb66e23
@jdavcs
Ignore SA error when dropping migrate_version tbl (see note)
c9acc66 
This is expected behavior. Normally we won't try to add or delete the
same object to/from the same database. The migrate_version table is an
exception: it exists in both models: galaxy and install model. When
dropping it, we drop it from both schemas - so we have 2 revisions
defined, and if the database is combined (as it is in at least one
test), the revisions with the same operations will be applied to the
same database.

We catch and ignore SQLAlchemy's InvalidRequstError: this handles both
sqlite and postgres use cases (OperationalError will only handle
sqlite).
@jdavcs
Modify remaining tests; leave one exposing bug
6c07cb5
@jdavcs
Fix postgres connection issue; 2 tests expected to fail
7496c2b 
2 tests expose a bug that is not fixed yet.
@jdavcs
Add workflow for migration tests
95b6dd3
@jdavcs
Do not load db state before creating a db
3af2367 
Also, do not call private method of DatabaseVerifier, even in tests: now
it will break because it's supposed to work only AFTER the db state has
been loaded - which is loaded lazily and only after verify() has been called.

Instead, add a helper method to the test module + a test for the helper.
@jdavcs
Fix mypy error.
d2013a8
@jdavcs
WIP: second pass at integrating; mypy not happy
0d6119e
@jdavcs
Refactor, remove duplication, fix tests
405c37b
@jdavcs
WIP: temporary fix for the weird mypy error
aec43a7 
No idea whatsoever what's causing mypy to complain:
- calling the method fixes the error
- inlining exactly the same stmt, causes an 'incompatible' mypy error.
Digging in.....
@jdavcs
Fix integration tests
189b8cf
@jdavcs
Move triggers out of migrate directory
515b786
@jdavcs
Run tests under Python 3.7
7a34c33
@jdavcs
Rewrite migration algorithm, update tests
37a1ba0
@jdavcs
Fix passing config values
6842867
@jdavcs
Fix tests: tsi and gxy may have different stored versions
83ec8e5 
This is how the current codebase works.
If the database is combined, we only store the galaxy version in the
  migrate_version table
But if we have 2 separate databases, we store 2 different versions
(currently, 179 for galaxy and 17 for install).

This commit adjusts the tests to test for this case; it also fixes the
error that prevented an upgrade from proceeding with separate databases
and automigrate set.
@jdavcs
Remove tmp stuff
bb9dc55
@jdavcs
Update latest version from 179 to 180
43234c4
@jdavcs
Cleanup postgres db when not created by test
73cb41b
@jdavcs jdavcs mentioned this pull request Dec 31, 2021
Closed
5 tasks
@jdavcs
Copy link
Member Author

jdavcs commented Jan 7, 2022

Closing (final version in #13108)

@jdavcs jdavcs closed this Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
area/configuration Galaxy's configuration system area/database Galaxy's database or data access layer area/documentation area/scripts area/testing kind/feature
Projects
None yet
Milestone
22.01
Development

Successfully merging this pull request may close these issues.
1 participant
@jdavcs