Copilot commented on code in PR #62234:
URL: https://github.com/apache/airflow/pull/62234#discussion_r2938514616
##########
scripts/in_container/run_generate_migration.sh:
##########
@@ -19,8 +19,14 @@
. "$(dirname "${BASH_SOURCE[0]}")/_in_container_script_init.sh"
cd "${AIRFLOW_SOURCES}" || exit 1
+export
AIRFLOW__DATABASE__EXTERNAL_DB_MANAGERS="airflow.providers.fab.auth_manager.models.db.FABDBManager,airflow.providers.edge3.models.db.EdgeDBManager"
+airflow db reset -y --use-migration-files
+
cd "airflow-core/src/airflow" || exit 1
-airflow db reset -y
-airflow db downgrade -n 2.10.3 -y
-airflow db migrate -r heads
+alembic revision --autogenerate -m "${@}"
Review Comment:
`alembic revision --autogenerate -m "${@}"` will expand to multiple
arguments when more than one parameter is passed to the script, which can break
Alembic argument parsing (only the first token belongs to `-m`). Consider using
a single message string (e.g. `$*` or `$1`) and passing any additional Alembic
args separately.
##########
scripts/in_container/run_generate_migration.sh:
##########
@@ -19,8 +19,14 @@
. "$(dirname "${BASH_SOURCE[0]}")/_in_container_script_init.sh"
cd "${AIRFLOW_SOURCES}" || exit 1
+export
AIRFLOW__DATABASE__EXTERNAL_DB_MANAGERS="airflow.providers.fab.auth_manager.models.db.FABDBManager,airflow.providers.edge3.models.db.EdgeDBManager"
+airflow db reset -y --use-migration-files
+
cd "airflow-core/src/airflow" || exit 1
-airflow db reset -y
-airflow db downgrade -n 2.10.3 -y
-airflow db migrate -r heads
+alembic revision --autogenerate -m "${@}"
+
+cd "${AIRFLOW_SOURCES}/providers/fab/src/airflow/providers/fab" || exit 1
+alembic revision --autogenerate -m "${@}"
Review Comment:
Same issue as above: `-m "${@}"` will split into multiple args if the script
is invoked with multiple parameters, so Alembic may treat the extra tokens as
unexpected arguments. Use a single string for the message and keep other args
separate.
##########
scripts/in_container/run_generate_migration.sh:
##########
@@ -19,8 +19,14 @@
. "$(dirname "${BASH_SOURCE[0]}")/_in_container_script_init.sh"
cd "${AIRFLOW_SOURCES}" || exit 1
+export
AIRFLOW__DATABASE__EXTERNAL_DB_MANAGERS="airflow.providers.fab.auth_manager.models.db.FABDBManager,airflow.providers.edge3.models.db.EdgeDBManager"
+airflow db reset -y --use-migration-files
+
cd "airflow-core/src/airflow" || exit 1
-airflow db reset -y
-airflow db downgrade -n 2.10.3 -y
-airflow db migrate -r heads
+alembic revision --autogenerate -m "${@}"
+
+cd "${AIRFLOW_SOURCES}/providers/fab/src/airflow/providers/fab" || exit 1
+alembic revision --autogenerate -m "${@}"
+
+cd "${AIRFLOW_SOURCES}/providers/edge3/src/airflow/providers/edge3" || exit 1
alembic revision --autogenerate -m "${@}"
Review Comment:
Same issue as above: `-m "${@}"` expands to multiple args; Alembic expects a
single message string for `-m`. Use `$*`/`$1` (or accept `-m` from the caller)
to avoid breaking when multiple args are provided.
##########
airflow-core/src/airflow/migrations/utils.py:
##########
@@ -75,8 +75,8 @@ def mysql_drop_foreignkey_if_exists(constraint_name,
table_name, op):
CONSTRAINT_NAME = '{constraint_name}' AND
CONSTRAINT_TYPE = 'FOREIGN KEY'
) THEN
- ALTER TABLE {table_name}
- DROP CONSTRAINT {constraint_name};
+ ALTER TABLE `{table_name}`
+ DROP CONSTRAINT `{constraint_name}`;
Review Comment:
In MySQL, dropping a foreign key constraint uses `ALTER TABLE ... DROP
FOREIGN KEY ...`, not `DROP CONSTRAINT`. Using `DROP CONSTRAINT` here is likely
to fail when this helper is used by MySQL migrations (e.g. 0027_2_10_3...).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
