Skip to content

Integrate alembic check in ci #49002

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

Closed

Conversation

guan404ming
Copy link
Contributor

@guan404ming guan404ming commented Apr 9, 2025

Related Issue

closes #48998
cc: @ashb

Why

Since Alembic 1.9, a built-in command alembic check is available to checks if there are pending upgrade operations in the migration file, to integrating this check into our CI ensures that no model changes go unnoticed, preventing potential issues that could arise when applying migrations in production.

How

  • add a new step to the ci

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@guan404ming guan404ming changed the title CI: integrate alembic check ci: integrate alembic check Apr 9, 2025
@guan404ming guan404ming force-pushed the integrate-alembic-check-in-ci branch from 5513306 to 9a67d8b Compare April 9, 2025 10:40
@ashb
Copy link
Member

ashb commented Apr 9, 2025

Could you add a change to a model in a second commit on this PR (that we will shortly revert/undo) to test that this fails as expected please?

@guan404ming
Copy link
Contributor Author

Sure! I would do it later after this CI finished (make sure it could pass now)

Comment on lines 1409 to 1415
ActionCommand(
name="check-models",
help="Check if there are model changes without a corresponding migration",
description="Check if the current models require new migrations to be generated",
func=lazy_load_command("airflow.cli.commands.db_command.check_models"),
args=(ARG_VERBOSE,),
),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I'm not sure we want this in here though actually.

The airflow db is designed for end users/Airflow administrators to run, but the check command is only useful for Airflow devs

Copy link
Contributor Author

@guan404ming guan404ming Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing, I would remove it from airflow db ~

@guan404ming guan404ming force-pushed the integrate-alembic-check-in-ci branch from 9a67d8b to f389818 Compare April 9, 2025 14:33
@guan404ming guan404ming force-pushed the integrate-alembic-check-in-ci branch from f389818 to b13a09c Compare April 9, 2025 14:50
@guan404ming guan404ming requested a review from XD-DENG as a code owner April 9, 2025 15:38
@guan404ming guan404ming force-pushed the integrate-alembic-check-in-ci branch 2 times, most recently from b3e4358 to 31fa445 Compare April 9, 2025 17:15
@guan404ming guan404ming marked this pull request as draft April 10, 2025 01:42
@guan404ming guan404ming force-pushed the integrate-alembic-check-in-ci branch 14 times, most recently from cc8b840 to f09bb95 Compare April 12, 2025 08:58
@guan404ming guan404ming marked this pull request as ready for review April 12, 2025 09:10
@guan404ming guan404ming force-pushed the integrate-alembic-check-in-ci branch 2 times, most recently from 66cd694 to 4c3b055 Compare April 29, 2025 07:22
@guan404ming guan404ming marked this pull request as ready for review April 29, 2025 07:22
@guan404ming guan404ming force-pushed the integrate-alembic-check-in-ci branch from 4c3b055 to 9645890 Compare April 29, 2025 16:55
@guan404ming guan404ming force-pushed the integrate-alembic-check-in-ci branch 3 times, most recently from 2cc7087 to ae67e5a Compare April 30, 2025 17:26
@guan404ming guan404ming changed the title ci: integrate alembic check Integrate alembic check in ci Apr 30, 2025
@guan404ming guan404ming force-pushed the integrate-alembic-check-in-ci branch 3 times, most recently from 9354811 to 01e5c34 Compare May 6, 2025 11:21
@ephraimbuddy
Copy link
Contributor

alembic check does not check if there's a difference between ORM models and migration files as you described. It only checks if there are pending upgrade operations in the migration file. For example, if you upgraded to Airflow 3.0 and your database migration head is still at Airflow 2.7.0. Running alembic check will detect that you have not upgraded your database to Airflow 3.0. As Ash suggested, you can add a field to a model and let us see that the CI fails due to the alembic check detecting the added field.

@guan404ming
Copy link
Contributor Author

Thanks for pointing out. I would modify my PR summary later.
I’ve also tried multiple times about the suggestion from Ash. But I can’t see the ci fail because of alembic check after I delete or add field. I’m not really sure the root cause about it but I could try it again. Then we could see the result. Thanks a lots.

@guan404ming
Copy link
Contributor Author

guan404ming commented May 6, 2025

I've added a field named test in dag and tried locally with breeze shell which work well to detect the change.

$ breeze shell "cd airflow-core/src/airflow && alembic check"

[2025-05-06T12:23:10.917+0000] {migration.py:207} INFO - Context impl SQLiteImpl.
[2025-05-06T12:23:10.919+0000] {migration.py:210} INFO - Will assume non-transactional DDL.
[2025-05-06T12:23:11.124+0000] {compare.py:400} INFO - Detected added column 'dag.test_field'
[2025-05-06T12:23:11.158+0000] {messaging.py:71} ERROR - New upgrade operations detected: [('add_column', None, 'dag', Column('test_field', String(length=2000), table=<dag>))]
  FAILED: New upgrade operations detected: [('add_column', None, 'dag', Column('test_field', String(length=2000),
  table=<dag>))]
Error 255 returned

but

$ pre-commit run check-model-changes
Check if model changes require DB migration..............................Passed

@ephraimbuddy
Copy link
Contributor

I've added a field named test in dag and tried locally with breeze shell which work well to detect the change.

$ breeze shell "cd airflow-core/src/airflow && alembic check"

[2025-05-06T12:23:10.917+0000] {migration.py:207} INFO - Context impl SQLiteImpl.
[2025-05-06T12:23:10.919+0000] {migration.py:210} INFO - Will assume non-transactional DDL.
[2025-05-06T12:23:11.124+0000] {compare.py:400} INFO - Detected added column 'dag.test_field'
[2025-05-06T12:23:11.158+0000] {messaging.py:71} ERROR - New upgrade operations detected: [('add_column', None, 'dag', Column('test_field', String(length=2000), table=<dag>))]
  FAILED: New upgrade operations detected: [('add_column', None, 'dag', Column('test_field', String(length=2000),
  table=<dag>))]
Error 255 returned

but

$ pre-commit run check-model-changes
Check if model changes require DB migration..............................Passed

wow. That's nice

@ephraimbuddy
Copy link
Contributor

Did you check if we can have this as a test in utils/test_db.py

@guan404ming guan404ming force-pushed the integrate-alembic-check-in-ci branch from a9cebfd to d2a7d02 Compare May 8, 2025 03:43
@guan404ming
Copy link
Contributor Author

guan404ming commented May 8, 2025

It works as well in my local computer. Let me test it on ci
Thanks for the suggestion.

@ephraimbuddy
Copy link
Contributor

There's a bug in CI that might be causing all these to pass: https://github.com/apache/airflow/actions/runs/14898553456/job/41845929327?pr=49002#step:5:5931
I'm working on it

@guan404ming
Copy link
Contributor Author

Thanks a lots!

@guan404ming guan404ming force-pushed the integrate-alembic-check-in-ci branch from acf3800 to c4dcb81 Compare May 8, 2025 14:29
@guan404ming
Copy link
Contributor Author

Rebased and hope ci failed!

@ephraimbuddy
Copy link
Contributor

So, the bug I fixed is unrelated. I think I understand what's happening. The DB is initialized from the ORM, and we try to run the file migration, too, by running downgrade and upgrade commands. In the end, the new field you added is already in the DB. When the alembic check command is run, the database already has the new field, and therefore, it's not detected.

In airflow 2, we could easily detect this new field because our tests initializes the database from the migration files instead of the ORM.

@guan404ming
Copy link
Contributor Author

Yes, I think that is unrelated. What you said is match with my local experiment. It would only successfully detect the error when I init with right ORM and then try add/delete something. I still try to figure how to deal with this without breaking the whole initialization process.

@guan404ming guan404ming marked this pull request as draft May 14, 2025 02:59
@guan404ming
Copy link
Contributor Author

Close now and would come back if I have better idea about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate alembic check into CI to check for model changes without a matching migration
3 participants