Modernize DAG-related URL routes and rename "tree" to "grid" #20730

IKholopov · 2022-01-06T18:51:52Z

related: #19944

Rename "tree" view to "grid" view
Update URL routes and add redirects from old path:
- /tree -> /dags/<dag_id>/grid
- /graph -> /dags/<dag_id>/graph
- /landing_times -> /dags/<dag_id>/landing_times
- /duration -> /dags/<dag_id>/duration
- /tries -> /dags/<dag_id>/tries
- /calendar -> /dags/<dag_id>/calendar
- /gantt -> /dags/<dag_id>/gantt
- /code -> /dags/<dag_id>/code
- /dag_details -> /dags/<dag_id>/details
New redirect - /dags/<dag_id> -> /dags/<dag_id>/grid

Once a new single DAG page is ready, the subroutes can be dropped and transformed into redirects to single /dags/<dag_id> page (with proper query params). As alternative, only single view could've been renamed (/tree -> /dags/<dag_id>), but I thought that it would be better to keep routes self-consistent mid-flight (in case if single-page view won't make it into 2.3.0).

DAGs page before:
DAGs page after:

boring-cyborg · 2022-01-06T18:51:55Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (flake8, mypy and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected]
Slack: https://s.apache.org/airflow-slack

airflow/www/templates/airflow/dag.html

bbovenzi

Looking good! Just some quick comments:

I agree with the use of legacy_calendar for old links instead of dag_calendar for new links.
We should also update the reference from tree to grid here
There are a lot more places where we need to change references of "tree" to "grid" in the code and documentation. That should happen shortly after this PR.

…" view - Rename tree view to grid view - Updated test cases for new paths - Update URL routes and add redirects

IKholopov · 2022-01-30T23:59:33Z

Renamed. This did add an additional backwards compatibility complexity. There is a number of places in code, where it is assumed that dag_id is passed exclusively over query parameter. To keep those working (especially the ones that are controlled by configuration, like default_view. To keep those working, both 'tree' and 'legacy_tree' views had to be kept/introduced. I'll address those places and make them work nicely with new routing schema shortly in separate PR.
Done
Yes, I'll tackle that in subsequent PR.

airflow/www/decorators.py

bbovenzi · 2022-01-31T17:18:09Z

airflow/www/templates/airflow/dags.html

  <meta name="dag_stats_url" content="{{ url_for('Airflow.dag_stats') }}">
  <meta name="task_stats_url" content="{{ url_for('Airflow.task_stats') }}">
-  <meta name="tree_url" content="{{ url_for('Airflow.tree') }}">
+  <meta name="tree_url" content="{{ url_for('Airflow.legacy_tree') }}">


Why do we need to use the legacy urls here? Is it because of how it gets dag_id? I feel like it would be best to update those if it isn't too much effort.

Yes. There are multiple places in the JS code where the dag_id is specified as a query parameter. As I've mentioned before, I would much prefer to address it in a separate PR as I imagine that it is not obvious what would be the most correct way to pass Python-generated URL to JS code with view_arg. (And I'd like to keep changes compact, rather than having a full and hard to debug rewrite in a single PR).

Are we doing to insert some templated string like '<:dag_id>'? If yes, how can we avoid breakage if the view_arg is renamed/new view_args added for a path? Or should we avoid passing dynamic URLs over the meta tags all together and come up with something else?

Plus, right now with legacy_ and tree views we will have a clear indication for places that will need to be updated (which I've started working on already in subsequent commit).

Ok thats fine. I don't want to expand the scope of this PR too much.

uranusjr · 2022-02-01T05:37:07Z

Need to fix the static checks, but logic-wise lgtm.

eladkal · 2022-02-01T09:21:04Z

What this means for people who have
dag_default_view = tree in airflow.cfg?

airflow/airflow/config_templates/default_airflow.cfg

Lines 578 to 579 in 4a73d8f

    
           # Default DAG view. Valid values are: ``tree``, ``graph``, ``duration``, ``gantt``, ``landing_times`` 
        
           dag_default_view = tree

ryanahamilton · 2022-02-01T20:16:11Z

airflow/www/views.py

+        """Redirect from url param."""
+        return redirect(url_for('Airflow.landing_times', **request.args))
+
+    @expose('/dags/<string:dag_id>/landing_times')


While we're changing this, could we use a hyphen instead? Hyphens are a more common, modern practice and we've used them in recent additions (e.g. /rendered-k8s, /rendered-templates).

Suggested change

@expose('/dags/<string:dag_id>/landing_times')

@expose('/dags/<string:dag_id>/landing-times')

IKholopov · 2022-02-01T22:06:15Z

Fixed static checks (sorry, forgot about them:) and addressed @ryanahamilton comment.

@eladkal DAG's default_view property used to dynamically construct URL in one of two ways:
a) {{ url_for('Airflow.' + dag.default_view, dag_id=...)}} - will direct to new views right away (except for tree, in this case it will redirect to grid).
b) {{ url_for('Airflow.' + dag.default_view)}} - changed to {{ url_for('Airflow.legacy_' + dag.default_view)}}, will turn into a link redirecting to new versions of views.
This configuration is one of the next places in the code to cleanup and migrate to new naming (grid). We will have to keep backwards compatibility for tree though.

bbovenzi

LGTM.

outdated

github-actions · 2022-02-10T09:53:39Z

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.

boring-cyborg · 2022-02-10T09:54:02Z

Awesome work, congrats on your first merged pull request!

…20730)" This reverts commit f217bec.

…20730) Co-authored-by: Igor Kholopov <[email protected]>

IKholopov requested review from ashb, bbovenzi and ryanahamilton as code owners January 6, 2022 18:51

boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Jan 6, 2022

uranusjr reviewed Jan 10, 2022

View reviewed changes

airflow/www/templates/airflow/dag.html Outdated Show resolved Hide resolved

bbovenzi reviewed Jan 13, 2022

View reviewed changes

IKholopov and others added 2 commits January 30, 2022 20:49

Webserver - Change URL routes for DAG page and rename "tree" to "grid…

6f281e6

…" view - Rename tree view to grid view - Updated test cases for new paths - Update URL routes and add redirects

Webserver - Change old path names to 'legacy_'

3ae4b3c

IKholopov force-pushed the ikholopov/url_single_path_dag_page branch from 50dbc04 to 3ae4b3c Compare January 30, 2022 23:54

IKholopov requested a review from bbovenzi January 31, 2022 00:21

uranusjr previously requested changes Jan 31, 2022

View reviewed changes

airflow/www/decorators.py Outdated Show resolved Hide resolved

bbovenzi reviewed Jan 31, 2022

View reviewed changes

Igor Kholopov added 2 commits January 31, 2022 22:41

Webserver - Use 'view_args' as optional source of 'dag_id'

a2a65bf

Webserver - Do not use 'legacy_tree' view in the link meta

ba01d65

IKholopov requested review from bbovenzi and uranusjr January 31, 2022 22:10

ryanahamilton reviewed Feb 1, 2022

View reviewed changes

Igor Kholopov added 2 commits February 1, 2022 22:52

Webserver - Fixing static checks

cb0c827

Webserver - Use hyphen instead of underscore in URL

875256e

IKholopov requested a review from ryanahamilton February 3, 2022 21:09

bbovenzi approved these changes Feb 7, 2022

View reviewed changes

github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Feb 10, 2022

uranusjr changed the title ~~Webserver - Change URL routes for DAG page and rename "tree" to "grid"~~ Modernize DAG-related URL routes and rename "tree" to "grid" Feb 10, 2022

uranusjr merged commit f217bec into apache:main Feb 10, 2022

ashb added a commit that referenced this pull request Feb 10, 2022

Revert "Modernize DAG-related URL routes and rename "tree" to "grid" (#…

2dc9233

…20730)" This reverts commit f217bec.

ashb mentioned this pull request Feb 10, 2022

Fix broken tests after URL restructure PR #21489

Merged

ferruzzi pushed a commit to ferruzzi/airflow that referenced this pull request Feb 11, 2022

Modernize DAG-related URL routes and rename "tree" to "grid" (apache#…

862ec76

…20730) Co-authored-by: Igor Kholopov <[email protected]>

jedcunningham added the type:improvement Changelog: Improvements label Feb 28, 2022

bbovenzi mentioned this pull request Mar 9, 2022

Update url routes and naming #19944

Closed

bbovenzi mentioned this pull request Feb 3, 2023

Remove /legacy_tree endpoint #29359

Merged

	@expose('/dags/<string:dag_id>/landing_times')
	@expose('/dags/<string:dag_id>/landing-times')

Modernize DAG-related URL routes and rename "tree" to "grid" #20730

Modernize DAG-related URL routes and rename "tree" to "grid" #20730

Uh oh!

Conversation

IKholopov commented Jan 6, 2022

Uh oh!

boring-cyborg bot commented Jan 6, 2022

Uh oh!

Uh oh!

bbovenzi left a comment

Choose a reason for hiding this comment

Uh oh!

IKholopov commented Jan 30, 2022

Uh oh!

Uh oh!

bbovenzi Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IKholopov Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbovenzi Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

uranusjr commented Feb 1, 2022

Uh oh!

eladkal commented Feb 1, 2022

Uh oh!

ryanahamilton Feb 1, 2022

Choose a reason for hiding this comment

Uh oh!

IKholopov Feb 1, 2022

Choose a reason for hiding this comment

Uh oh!

IKholopov commented Feb 1, 2022

Uh oh!

bbovenzi left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 10, 2022

Uh oh!

boring-cyborg bot commented Feb 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bbovenzi Jan 31, 2022 •

edited

Loading

IKholopov Jan 31, 2022 •

edited

Loading