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

Remove get_readable_dag, get_editable_dag, and get_accessible_dags. #19961

Merged
merged 1 commit into from
Dec 6, 2021

Conversation

jhtimmins
Copy link
Contributor

@jhtimmins jhtimmins commented Dec 2, 2021

As part of cleaning up FAB, we're minimizing the size of the interface by removing unnecessary helper methods. This PR removes get_readable_dags, get_editable_dags, and get_accessible_dags.

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:webserver Webserver related Issues labels Dec 2, 2021
Copy link
Contributor

@bbovenzi bbovenzi left a comment

Choose a reason for hiding this comment

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

UI still seems to work but I can't comment on the code itself.

@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 Dec 2, 2021
@github-actions
Copy link

github-actions bot commented Dec 2, 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.

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Why though?

It seems expensive to have to get full DAGModel objects just to then throw away all that info and keep the dag_ids only.

How many places do we actually use get_readable_dags etc and want the dags vs the IDs?

Would it perhaps make more sense to replace get_accessible_dags with a get_accesible_dag_ids that directly returns the IDs?

@jhtimmins jhtimmins force-pushed the remove-fab-helper-methods branch from a9a590f to f8ab965 Compare December 4, 2021 01:00
@jhtimmins jhtimmins changed the title Remove get_readable_dag_ids and get_editable_dag_ids. Remove get_readable_dag and get_editable_dag. Dec 4, 2021
@jhtimmins jhtimmins changed the title Remove get_readable_dag and get_editable_dag. Remove get_readable_dag, get_editable_dag, and get_accessible_dags. Dec 4, 2021
@jhtimmins
Copy link
Contributor Author

jhtimmins commented Dec 4, 2021

@ashb good call. That was actually a simpler change overall, which I've made.

@jhtimmins jhtimmins requested a review from ashb December 4, 2021 01:05
@jhtimmins jhtimmins merged commit fc0fb22 into apache:main Dec 6, 2021
@jhtimmins jhtimmins deleted the remove-fab-helper-methods branch December 6, 2021 21:29
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API area:webserver Webserver related Issues okay to merge It's ok to merge this PR as it does not require more tests type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants