-
Notifications
You must be signed in to change notification settings - Fork 687
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
Alembic related failures on Focal #5605
Comments
For The error is:
|
Too many other errors are also related to this |
Here is the dump of the schema from our code: This
|
On Xenial:
On Focal:
|
Digging more into the sqlite db:
|
Found the issue on https://sqlite.org/lang_altertable.html:
|
We can solve it for now by using the following diff in a PR: diff --git a/securedrop/alembic/env.py b/securedrop/alembic/env.py
index c16d34a5a..d6bce65b5 100644
--- a/securedrop/alembic/env.py
+++ b/securedrop/alembic/env.py
@@ -5,6 +5,8 @@ import sys
from alembic import context
from sqlalchemy import engine_from_config, pool
+from sqlalchemy.engine import Engine
+from sqlalchemy import event
from logging.config import fileConfig
from os import path
@@ -16,6 +18,12 @@ fileConfig(config.config_file_name)
sys.path.insert(0, path.realpath(path.join(path.dirname(__file__), '..')))
from db import db # noqa
+@event.listens_for(Engine, "connect")
+def set_sqlite_pragma(dbapi_connection, connection_record):
+ cursor = dbapi_connection.cursor()
+ cursor.execute("PRAGMA legacy_alter_table=ON")
+ cursor.close()
+
try:
# These imports are only needed for offline generation of automigrations.
# Importing them in a prod-like environment breaks things. But, I personally think in future we should rewrite the migration files. I will wait to talk to the team before I open the PR. |
If we don't rewrite the offending migrations, where we've used the procedure labeled incorrect under Making Other Kinds Of Table Schema Changes, but just call that pragma first thing within them instead of globally in I moved the fix to the beginning of the migrations that rename tables, the Alembic tests pass, and a database created with conn = op.get_bind()
conn.execute("PRAGMA legacy_alter_table=ON") If I remove the pragma from one of the later migrations, the tests fail, so that suggests we're getting a connection per migration, and so these local fixes shouldn't affect new migrations. |
Due to SQLite3 upgrade in Focal, our previous migration scripts were failing. From SQLite website https://sqlite.org/lang_altertable.html: ``` Compatibility Note: The behavior of ALTER TABLE when renaming a table was enhanced in versions 3.25.0 (2018-09-15) and 3.26.0 (2018-12-01) in order to carry the rename operation forward into triggers and views that reference the renamed table. This is considered an improvement. Applications that depend on the older (and arguably buggy) behavior can use the PRAGMA legacy_alter_table=ON statement or the SQLITE_DBCONFIG_LEGACY_ALTER_TABLE configuration parameter on sqlite3_db_config() interface to make ALTER TABLE RENAME behave as it did prior to version 3.25.0. ```
…e_to_sqlite_upgrade Fixes #5605 alembic migration errors on Focal
Description
We have various alembic related failures on Focal application tests.
Steps to Reproduce
dev_focal
branch.git checkout dev_focal
.make test-focal
Expected Behavior
Tests should pass.
Actual Behavior
Comments
Suggestions to fix, any other relevant information.
The text was updated successfully, but these errors were encountered: