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

Adding only_active parameter to /dags endpoint #14306

Merged
merged 10 commits into from Jun 14, 2021

Conversation

@SamWheating
Copy link
Contributor

@SamWheating SamWheating commented Feb 18, 2021

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.

airflow/api_connexion/openapi/v1.yaml Outdated Show resolved Hide resolved
airflow/api_connexion/openapi/v1.yaml Outdated Show resolved Hide resolved
@SamWheating
Copy link
Contributor Author

@SamWheating SamWheating commented Feb 19, 2021

Looks like I missed updating some Schema tests - I'll push up a fix for this shortly.

@ephraimbuddy
Copy link
Contributor

@ephraimbuddy ephraimbuddy commented Feb 19, 2021

Looks like I missed updating some Schema tests - I'll push up a fix for this shortly.

Not schema test. Linting error. - name is not in line with other parameters

@github-actions
Copy link

@github-actions github-actions bot commented Feb 19, 2021

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*.

@github-actions
Copy link

@github-actions github-actions bot commented Feb 19, 2021

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*.

@github-actions
Copy link

@github-actions github-actions bot commented Feb 19, 2021

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*.

airflow/api_connexion/endpoints/dag_endpoint.py Outdated Show resolved Hide resolved
airflow/api_connexion/endpoints/dag_endpoint.py Outdated Show resolved Hide resolved
airflow/api_connexion/openapi/v1.yaml Outdated Show resolved Hide resolved
airflow/models/dag.py Show resolved Hide resolved
airflow/api_connexion/openapi/v1.yaml Show resolved Hide resolved
airflow/api_connexion/openapi/v1.yaml Outdated Show resolved Hide resolved
airflow/api_connexion/schemas/dag_schema.py Show resolved Hide resolved
@kaxil kaxil requested a review from ashb Feb 21, 2021
kaxil
kaxil approved these changes Feb 21, 2021
@kaxil kaxil added this to the Airflow 2.0.2 milestone Feb 23, 2021
@kaxil
Copy link
Member

@kaxil kaxil commented Feb 25, 2021

@ephraimbuddy @ashb -- Since you folks requested changes, can you review it once more to see all your concerns are addressed

@SamWheating SamWheating force-pushed the sw-filter-active-get-dags branch from c569b5e to f6f5801 Mar 8, 2021
@SamWheating
Copy link
Contributor Author

@SamWheating SamWheating commented Mar 8, 2021

Just rebased on master and fixed the merge conflicts, I believe I have addressed all of the requested changes now as well.

@github-actions
Copy link

@github-actions github-actions bot commented Mar 8, 2021

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 SamWheating force-pushed the sw-filter-active-get-dags branch from 64d79f8 to a1f8d73 Mar 24, 2021
@github-actions
Copy link

@github-actions github-actions bot commented Mar 24, 2021

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*.

@ashb
Copy link
Member

@ashb ashb commented Mar 24, 2021

Code looks fine, but I wonder if we want to rename this now to match the proposed new external names as mentioned in #14459

@ashb ashb removed this from the Airflow 2.0.2 milestone Apr 22, 2021
@ashb ashb added this to the Airflow 2.0.3 milestone Apr 22, 2021
@ashb ashb removed this from the Airflow 2.0.3 milestone May 7, 2021
@ashb ashb added this to the Airflow 2.1.1 milestone May 7, 2021
@SamWheating SamWheating force-pushed the sw-filter-active-get-dags branch from 168ccec to 55d3713 May 8, 2021
@SamWheating
Copy link
Contributor Author

@SamWheating SamWheating commented May 8, 2021

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.

@XD-DENG
Copy link
Member

@XD-DENG XD-DENG commented Jun 7, 2021

Code looks fine, but I wonder if we want to rename this now to match the proposed new external names as mentioned in #14459

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 SamWheating force-pushed the sw-filter-active-get-dags branch from 55d3713 to 81d3df6 Jun 7, 2021
@SamWheating
Copy link
Contributor Author

@SamWheating SamWheating commented Jun 7, 2021

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?

ashb
ashb approved these changes Jun 10, 2021
Copy link
Member

@ashb ashb left a comment

LGTM

@github-actions
Copy link

@github-actions github-actions bot commented Jun 10, 2021

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.

@kaxil
Copy link
Member

@kaxil kaxil commented Jun 10, 2021

@SamWheating can you rebase your PR once more and fix merge conflicts too please

@SamWheating
Copy link
Contributor Author

@SamWheating SamWheating commented Jun 10, 2021

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:

  • Expose an only_active parameter on the /dags endpoint (deafult to True)
  • Include is_active and is_paused in the DAG detail schema

Update: I've updated + rebased this PR, should be good to go now.

@SamWheating SamWheating force-pushed the sw-filter-active-get-dags branch from 81d3df6 to a331f55 Jun 10, 2021
@SamWheating
Copy link
Contributor Author

@SamWheating SamWheating commented Jun 11, 2021

I believe the above test failures are unrelated / transient errors as I was unable to reproduce them locally in breeze.

kaxil
kaxil approved these changes Jun 14, 2021
@kaxil kaxil merged commit 9526a24 into apache:main Jun 14, 2021
47 of 49 checks passed
jhtimmins added a commit to astronomer/airflow that referenced this issue Jun 16, 2021
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)
ashb added a commit that referenced this issue Jun 22, 2021
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants