Adding only_active parameter to /dags endpoint
#14306
Conversation
SamWheating
requested review from
ashb,
kaxil,
mik-laj,
turbaszek and
XD-DENG
as code owners
|
Looks like I missed updating some Schema tests - I'll push up a fix for this shortly. |
Not schema test. Linting error. |
|
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
|
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
|
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
|
@ephraimbuddy @ashb -- Since you folks requested changes, can you review it once more to see all your concerns are addressed |
SamWheating
force-pushed
the
sw-filter-active-get-dags
branch
from
c569b5e
to
f6f5801
Compare
|
Just rebased on master and fixed the merge conflicts, I believe I have addressed all of the requested changes now as well. |
|
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
SamWheating
force-pushed
the
sw-filter-active-get-dags
branch
from
64d79f8
to
a1f8d73
Compare
|
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
|
Code looks fine, but I wonder if we want to rename this now to match the proposed new external names as mentioned in #14459 |
SamWheating
force-pushed
the
sw-filter-active-get-dags
branch
from
168ccec
to
55d3713
Compare
|
Sorry for the long delay, just rebased and this should be good to go now (pending CI). It looks like #14459 is still under early discussion, so I think we should proceed with these changes and rename fields later as required. Feel free to tag me in an issue if/when you need the wording updated. |
Hi @ashb , this issue #14459 (comment) is blocking us progressing on #14459 . If folks have no clear clue/solution on resolving #14459 (comment), maybe can consider merging this PR first. |
SamWheating
force-pushed
the
sw-filter-active-get-dags
branch
from
55d3713
to
81d3df6
Compare
|
Alright I've rebased on main and ran the tests locally - Happy to help update this if we change the wording or some field names later on. Could I get your reviews when you have a chance @ashb @turbaszek? |
|
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
github-actions
bot
added
the
full tests needed
|
@SamWheating can you rebase your PR once more and fix merge conflicts too please |
|
Ah, looks like the original issue leading to this PR was fixed in #16318, which prevents the API endpoint from ever returning inactive DAGs. I'll refactor this PR accordingly, but the changes introduced by this PR are now:
Update: I've updated + rebased this PR, should be good to go now. |
Co-authored-by: Ephraim Anierobi <splendidzigy24@gmail.com>
Co-authored-by: Xiaodong DENG <xd.deng.r@gmail.com>
SamWheating
force-pushed
the
sw-filter-active-get-dags
branch
from
81d3df6
to
a331f55
Compare
|
I believe the above test failures are unrelated / transient errors as I was unable to reproduce them locally in breeze. |
I noticed that the `/dags` endpoint returns information on all entries in the DAG table, which is often many more DAGs than are activeand likely includes DAGs which have been removed from Airflow. This PR adds a boolean `only_active` parameter to the `/dags` endpoint which will then only return active DAGs. I also noticed that this endpoint was hitting a deprecated codepath by dumping a `DAG` object to the DAGDetailSchema, thus hitting calling `DAG.is_paused()` I have updated the schema to call the correct function (`DAG.get_is_paused`) since I'm assuming the deprecated functions may be removed some day. (cherry picked from commit 9526a24)
I noticed that the
/dagsendpoint returns information on all entries in the DAG table, which is often many more DAGs than are activeand likely includes DAGs which have been removed from Airflow.This PR adds a boolean
only_activeparameter to the/dagsendpoint which will then only return active DAGs.I also noticed that this endpoint was hitting a deprecated codepath by dumping a
DAGobject to the DAGDetailSchema, thus hitting callingDAG.is_paused()I have updated the schema to call the correct function (DAG.get_is_paused) since I'm assuming the deprecated functions may be removed some day.