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

Add a couple DAG bundle related helpers #45339

Closed
wants to merge 3 commits into from

Conversation

jedcunningham
Copy link
Member

This is broken out of the larger changes adding DAG bundle parsing, to make reviewing that (eventual) PR a bit easier.

airflow/models/dag.py Outdated Show resolved Hide resolved
@potiuk
Copy link
Member

potiuk commented Jan 2, 2025

@jedcunningham - can you please rebase that one -> we found and issue with @jscheffl with the new caching scheme - fixed in #45347 that would run "main" version of the tests. I am asking in all affected PRs to rebase.

@jedcunningham jedcunningham force-pushed the add_dagbundlemodel_get branch from ca0b0ab to 5357956 Compare January 2, 2025 14:39
@Lee-W Lee-W self-requested a review January 3, 2025 04:16
This is broken out of the larger changes adding DAG bundle parsing, to
make reviewing that (eventual) PR a bit easier.
@jedcunningham jedcunningham force-pushed the add_dagbundlemodel_get branch from 327c909 to c0cf936 Compare January 4, 2025 20:56
@@ -770,6 +770,16 @@ def get_is_paused(self, session=NEW_SESSION) -> None:
"""Return a boolean indicating whether this DAG is paused."""
return session.scalar(select(DagModel.is_paused).where(DagModel.dag_id == self.dag_id))

@provide_session
def get_bundle_name(self, session=NEW_SESSION) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to be None?

Copy link
Member

Choose a reason for hiding this comment

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

It could if the DAG is not synced into the db. This one and get_latest_bundle_version should have the same return type (either both str or both str | None), and if it’s to return str, it should use scalars(...).one() instead to raise an exception when there’s no matching entry.

Copy link
Member Author

@jedcunningham jedcunningham Jan 15, 2025

Choose a reason for hiding this comment

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

Possible right now - yes. Possible next week, no :)

For now bundle is nullable in the db, but that's on my list and very near the top.

The PR these were split out of is already merged, so I actually need to close this PR. But I'll open one to do a little cleanup on these methods. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is that cleanup PR, #45672. I've left it returning None and will tackle the retyping it later this week.

Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

Overall looks good, but there's conflict we might need to resolve and would love to know if it's possible for us to add test cases for this. Thanks!

@jedcunningham jedcunningham deleted the add_dagbundlemodel_get branch January 15, 2025 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

6 participants