Replace SQLAlchemy Migrate with Alembic #13513
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Otherwise alembic/env.py gets executed out of context at test collection stage (via tox -e unit >> run_tests.sh), which raises an AttributeError (alembic.context is not set in this case)
Otherwise alembic/env.py gets executed out of context at test collection stage (via tox -e unit >> run_tests.sh), which raises an AttributeError (alembic.context is not set in this case)
To eliminate the repetitive `engine_options = engine_options or {}` in
multiple places.
This tests sqlalchemy migrate which is being replaced.
(does not include an import in mapping.py: to be added in a separate commit, grouped with other edits in mapping.py)
Squashed: - pick 01a42712d2 Avoid extra trip to db via lazy load of alembic version heads - squash 8d729974d6 Fix mypy error
- pick 058f09a Integrate migrations design into config and model code - squash 89ca1a537b Drop config.database_create_tables (not used) Next squash: - pick cbf29e138c Integrate migrations into config, model (squashed) - squash dd51fd578e Address existing mypy error (see note) Address existing mypy error (see note) This is a (possible) bug that has existed on dev. To expose it, simply change this call to init_models_from_config (https://github.com/galaxyproject/galaxy/blob/release_21.09/lib/galaxy/config/__init__.py#L1302) to a call directly to mapping.init(). The result will be 6 mypy errors. The reason is that the mapping.init() function (https://github.com/galaxyproject/galaxy/blob/release_21.09/lib/galaxy/model/mapping.py#L79) specifies its return type as GalaxyModelMapping - which results in several errors of this type: lib/galaxy/app.py:178: error: Definition of "model" in base class "MinimalApp" is incompatible with definition in base class "ConfiguresGalaxyMixin" When we call an intermediary function (that does not have a return type specified), mypy is happy. This should be addressed in a separate issue/PR. Removing the return type is the simplest (temporary) solution. Next squash: - squash c842973 Add type hints to migrations modules - squash a54be22 Remove circular dependency between migrations and config Setup infrastructure for running db scripts (squashed) Squashed: - pick 146ff8019a Setup infrastructure for running db scripts - squash e0a26dcf3a Move is-one-db check into model.database_utils Squashed: - pick a82c44a Setup infrastructure for running db scripts (squashed) - squash 31a1a702fe Fix mypy error Squashed: - pick 14a4eb08e2 Setup infrastructure for running db scripts (squashed) - squash b1c6acb0f1 When overriding, override. Add type hints to migrations modules Remove circular dependency between migrations and config
Also add exec permissions to create/migrate python scripts. (not sure this is required, adding for consistency with previous version) + A few fixes for TS scripts, tests Squashed: - pick b624432 Add create/migrate scripts for Alembic dbs - squash 06f0a92 Fix integration test (test only relevant for TS)
jdavcs
added
area/database
jdavcs
changed the title
This will be added as an alembic revision as a separate commit
5 tasks
jdavcs
force-pushed
the
dev_alembic20_rebase2_1
branch
from
c8ee55e
to
55448d4
Compare
See inline comment ("version too old" is misleading if the incorrect
version is more recent than what the system expects).
jdavcs
force-pushed
the
dev_alembic20_rebase2_1
branch
from
55448d4
to
44d245f
Compare
mvdbeek
approved these changes
Mar 11, 2022
|
Thanks a lot @ic4f, very nice work! |
|
Incredible work, thanks @ic4f! |
|
Thank you for merging, @mvdbeek! And thank you, everyone, for reviewing these branches! 🎆 |
This was referenced Mar 11, 2022
mvdbeek
changed the title
hexylena
added
highlight/dev
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This supersedes #13108. Please check that PR for a description of the new migration system and its testing infrastructure, how to test manually, and discussion.
Why a new branch/PR
Merging #13461 introduced conflicts in
config/__init__.pyandmodel/mapping.pythat were not easy to resolve. The gist of it is that both PRs moved and refactored parts of these two files early on, with multiple commits following in either case. As a result, the alembic branch has 50+ commits building upon code that does not exist anymore. After trying several approaches, I've settled on the following:HEAD(i.e., conflicting alembic branch edits discarded fromconfig/__init__.pyandmodel/mapping.pyin favor of edits made in [22.01] Create config package, move app-related functionality to app #13461)The following will be submitted as a follow-up PR immediately after this is merged (these edits do not belong in this PR):
How to test the changes?
(Select all options that apply)
License