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

Jump to bottom

Fix API List DAGs inconsistent with UI/CLI #16318

Merged
merged 9 commits into from Jun 10, 2021
Merged

Fix API List DAGs inconsistent with UI/CLI #16318

merged 9 commits into from Jun 10, 2021

Conversation

jpyen
Copy link
Contributor

@jpyen jpyen commented Jun 8, 2021

related: #16228

/api/v1/dags should yield the same output as the Web UI and CLI, namely:

  • show all dags
  • that the user has access to
  • which are present as DAG (can be scheduled, source file available)

The current implementation does only list DAGs that the user has access to, but fails to get the true DAG bag list. Which means the API endpoint is showing many more, deleted DAGs, that the UI and Web CLI do not.

@boring-cyborg boring-cyborg bot added the area:API Airflow's REST/HTTP API label Jun 8, 2021
ephraimbuddy
ephraimbuddy requested changes Jun 8, 2021
View reviewed changes
Copy link
Contributor

@ephraimbuddy ephraimbuddy left a comment

Choose a reason for hiding this comment

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

Can you add a test to avoid regression?

@jpyen
Copy link
Contributor Author

jpyen commented Jun 8, 2021

Can you add a test to avoid regression?

Sure thing, although I am not too sure how to reproduce this state/ explicitly test for this state. Do you have any recommendation?

@ephraimbuddy
Copy link
Contributor

Can you add a test to avoid regression?

Sure thing, although I am not too sure how to reproduce this state/ explicitly test for this state. Do you have any recommendation?

I think you can achieve it by marking dag inactive. DagModel.is_active exists, so many mark a dag inactive and another active

ephraimbuddy
ephraimbuddy reviewed Jun 8, 2021
View reviewed changes
tests/api_connexion/endpoints/test_dag_endpoint.py
@@ -387,6 +396,7 @@ def test_should_raise_404_when_dag_is_not_found(self):
class TestGetDags(TestDagEndpoint):
def test_should_respond_200(self):
self._create_dag_models(2)
self._create_deactivated_dag()
Copy link
Contributor

Choose a reason for hiding this comment

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

How about you add a query that asserts that the number of dags in DagModel is 3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Number of DAGs in DagModel should exclude inactive DAGs, as far as I understand. In my opinion are inactive DAGs .trash and play no significant role.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know but how are we sure that an inactive dag exists here? Do we have a test for the new method you created? I think that querying DagModel and asserting that it has 3 dags(both active and inactive) at this point will help to know that the method you created works fine. If there's an alteration in the method in the future to work another way, it'll be noticed. What do you think?

@jpyen
Copy link
Contributor Author

jpyen commented Jun 9, 2021

@ephraimbuddy Is it possible to run the full CI suite?

@ephraimbuddy
Copy link
Contributor

ephraimbuddy commented Jun 9, 2021 •
edited

@ephraimbuddy Is it possible to run the full CI suite?

Once approved, it'll run more tests as needed

@kaxil kaxil closed this Jun 9, 2021
@kaxil kaxil reopened this Jun 9, 2021
@jpyen
Copy link
Contributor Author

jpyen commented Jun 9, 2021

Added @ephraimbuddy suggestions & rebased to master.

@jpyen jpyen requested a review from ephraimbuddy June 10, 2021 01:27
@jpyen jpyen closed this Jun 10, 2021
@jpyen jpyen reopened this Jun 10, 2021
@jpyen jpyen closed this Jun 10, 2021
@jpyen jpyen reopened this Jun 10, 2021
@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Jun 10, 2021
@jpyen
Copy link
Contributor Author

jpyen commented Jun 10, 2021 •
edited

@kaxil @ephraimbuddy Changes approved, checks passed and ready to merge. Thanks for the patience.

@ephraimbuddy ephraimbuddy self-assigned this Jun 10, 2021
@ephraimbuddy ephraimbuddy merged commit 9ba796e into apache:main Jun 10, 2021
@jpyen
Copy link
Contributor Author

jpyen commented Jun 10, 2021

Thanks @ephraimbuddy!

@ephraimbuddy
Copy link
Contributor

Congrats on the PR @jpyen

@SamWheating SamWheating mentioned this pull request Jun 10, 2021
Merged
@ashb ashb added this to the Airflow 2.1.1 milestone Jun 16, 2021
ashb pushed a commit that referenced this pull request Jun 22, 2021
@jpyen @ashb
Make REST API List DAGs endpoint consistent with UI/CLI behaviour (#1…
dbf3064 
…6318)

Co-authored-by: jpyen <>
(cherry picked from commit 9ba796e)
@psg2 psg2 mentioned this pull request Jul 7, 2021
Closed
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

@kaxil kaxil Awaiting requested review from kaxil

@mik-laj mik-laj Awaiting requested review from mik-laj

Assignees

@ephraimbuddy ephraimbuddy

Labels
area:API Airflow's REST/HTTP API okay to merge It's ok to merge this PR as it does not require more tests
Projects
None yet
Milestone
Airflow 2.1.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jpyen @ephraimbuddy @ashb @kaxil