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

Fix API List DAGs inconsistent with UI/CLI #16318

Merged
merged 9 commits into from Jun 10, 2021
Merged

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.

Copy link
Contributor

@ephraimbuddy ephraimbuddy left a comment

Can you add a test to avoid regression?

@jpyen
Copy link
Contributor Author

@jpyen 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

@ephraimbuddy ephraimbuddy 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?

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

@@ -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

@ephraimbuddy ephraimbuddy Jun 8, 2021

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

@jpyen jpyen Jun 8, 2021

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

@ephraimbuddy ephraimbuddy Jun 9, 2021

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 jpyen commented Jun 9, 2021

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

@ephraimbuddy
Copy link
Contributor

@ephraimbuddy ephraimbuddy commented Jun 9, 2021

@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 jpyen commented Jun 9, 2021

Added @ephraimbuddy suggestions & rebased to master.

@jpyen jpyen requested a review from ephraimbuddy Jun 10, 2021
@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

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

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.

@jpyen
Copy link
Contributor Author

@jpyen jpyen commented Jun 10, 2021

@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
86 of 88 checks passed
@jpyen
Copy link
Contributor Author

@jpyen jpyen commented Jun 10, 2021

Thanks @ephraimbuddy!

@ephraimbuddy
Copy link
Contributor

@ephraimbuddy ephraimbuddy commented Jun 10, 2021

Congrats on the PR @jpyen

@ashb ashb added this to the Airflow 2.1.1 milestone Jun 16, 2021
jhtimmins added a commit to astronomer/airflow that referenced this issue Jun 16, 2021
ashb added a commit that referenced this issue Jun 22, 2021
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

4 participants