Skip to content

make bundle_name not nullable #47592

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 51 commits into
base: main
Choose a base branch
from
Open

make bundle_name not nullable #47592

wants to merge 51 commits into from

Conversation

gyli
Copy link
Collaborator

@gyli gyli commented Mar 11, 2025

Change bundle_name in dag not nullable and prepopulate the value in the migration: #46734

@boring-cyborg boring-cyborg bot added the area:db-migrations PRs with DB migration label Mar 11, 2025
ephraimbuddy
ephraimbuddy reviewed Mar 11, 2025
View reviewed changes
@gyli @ephraimbuddy
Apply suggestions from code review
24ba85b 
remove migration query

Co-authored-by: Ephraim Anierobi <[email protected]>
jedcunningham
jedcunningham reviewed Mar 12, 2025
View reviewed changes
@gyli gyli requested a review from potiuk as a code owner March 15, 2025 22:24
ephraimbuddy
ephraimbuddy reviewed Mar 16, 2025
View reviewed changes
ephraimbuddy
ephraimbuddy reviewed Mar 16, 2025
View reviewed changes
@gyli
Copy link
Collaborator Author

gyli commented May 5, 2025

Hi @ephraimbuddy @jedcunningham Do you think this PR looks good already, or you think more testing needs to be done?

@ephraimbuddy
Copy link
Contributor

Hi @ephraimbuddy @jedcunningham Do you think this PR looks good already, or you think more testing needs to be done?

Since Airflow 3 has been released, we should update the PR with new migration file instead of editing the add-dagbundlemodel migration file. Sorry that it didn't make it to 3.0.0

@gyli
Copy link
Collaborator Author

gyli commented May 6, 2025

PR is updated to use new migration file, as 3.0 is rolled out.

@gyli @ephraimbuddy
Update airflow-core/src/airflow/migrations/versions/0069_3_1_0_make_b…
2851fe6 
…undle_name_not_nullable.py

Co-authored-by: Ephraim Anierobi <[email protected]>
@gyli @ephraimbuddy
Update airflow-core/src/airflow/migrations/versions/0069_3_1_0_make_b…
c65885f 
…undle_name_not_nullable.py


Update downgrade

Co-authored-by: Ephraim Anierobi <[email protected]>
@ephraimbuddy
Copy link
Contributor

I noticed a pattern where you have to add DagModel with the bundle_name explicitly despite that there's dag.sync_to_db next to it. I feel something is wrong somewhere for us to do that

Hi @ephraimbuddy, This is because DagModel.bundle_name does not have a not-null default value. I think ideally, bulk_write_to_db or sync_to_db can be updated to handle DagModel and DagBundleModel insertion, while I am afraid this is too much to be added to this PR, as these these methods are used in other unrelated unit tests and components as well. I can probably leave a TODO comment and improve this in another PR. What do you think?

Another potential approach is to set dags-folder as the default value in the model. While I don't see a similar case with hard-coded string value in other models, so I assume we don't want to this.

Improving it in another PR is good too.

@gyli gyli requested review from jedcunningham and uranusjr June 30, 2025 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ephraimbuddy ephraimbuddy ephraimbuddy approved these changes

@XD-DENG XD-DENG Awaiting requested review from XD-DENG XD-DENG is a code owner

@ashb ashb Awaiting requested review from ashb ashb is a code owner

@potiuk potiuk Awaiting requested review from potiuk potiuk is a code owner

@vincbeck vincbeck Awaiting requested review from vincbeck vincbeck is a code owner

@hussein-awala hussein-awala Awaiting requested review from hussein-awala hussein-awala is a code owner

@jedcunningham jedcunningham Awaiting requested review from jedcunningham jedcunningham is a code owner

@uranusjr uranusjr Awaiting requested review from uranusjr

Assignees
No one assigned
Labels
AIP-66: DAG Bundle/Manifest area:db-migrations PRs with DB migration
Projects
AIP-66: DAG Bundles & Parsing
Status: In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gyli @ephraimbuddy @uranusjr @jedcunningham