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

Check for both alembic and SQLAlchemy Migrate errors #152

Merged
merged 2 commits into from
Apr 15, 2022

Conversation

jdavcs
Copy link
Member

@jdavcs jdavcs commented Apr 7, 2022

Fixes #151

This will support both pre- and post-alembic versions. No change to galaxy core is required.

Because this needs to support both pre- and post-alembic versions.
@jdavcs jdavcs added the bug label Apr 7, 2022
@jdavcs jdavcs requested a review from natefoo April 7, 2022 19:35
@natefoo
Copy link
Member

natefoo commented Apr 8, 2022

Will galaxy.model.migrations.IncorrectVersionError not be thrown if the DB is initialized but at the wrong version?

@jdavcs
Copy link
Member Author

jdavcs commented Apr 8, 2022

Will galaxy.model.migrations.IncorrectVersionError not be thrown if the DB is initialized but at the wrong version?

That would be OutdatedDatabaseError. The IncorrectVersionError is only thrown here
Is my logic faulty?

@jdavcs
Copy link
Member Author

jdavcs commented Apr 8, 2022

Don't merge just yet - checking whether a different error may be more appropriate here (on the galaxy side).

@jdavcs
Copy link
Member Author

jdavcs commented Apr 8, 2022

@natefoo this should be better. Will work correctly after galaxyproject/galaxy#13695 is merged. [update: it is merged]
Essentially, the previous version was correct: that was the error that was raised when the version table was not found (as is the case with a new db). However, it was the same error as the one raised in a different context - when the db is non-empty, is under SQLAlchemy Migrate control AND is outdated (pre-180). Now it raises a more appropriate error: NoVersionTable. This is handled by code that is triggered only when scripts/manage_db.py is called directly, bypassing the shell script manage_db.sh and passing SQLAlchemy Migrate-style command arguments (version/db_version) - which is why I didn't catch this use case earlier.

@jdavcs
Copy link
Member Author

jdavcs commented Apr 12, 2022

@natefoo does this look OK to you?

@natefoo
Copy link
Member

natefoo commented Apr 15, 2022

LGTM, thanks!

@natefoo natefoo merged commit a3dcedd into galaxyproject:main Apr 15, 2022
natefoo added a commit to galaxyproject/ansible-collection-galaxy that referenced this pull request May 31, 2022
natefoo added a commit to galaxyproject/ansible-collection-galaxy that referenced this pull request May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Database task breaks on dev
2 participants