Uh oh!
There was an error while loading. Please reload this page.
Modernize DAG-related URL routes and rename "tree" to "grid" - #20730
Conversation
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)
|
Uh oh!
There was an error while loading. Please reload this page.
bbovenzi
left a comment
There was a problem hiding this comment.
Looking good! Just some quick comments:
- I agree with the use of
legacy_calendarfor old links instead ofdag_calendarfor 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
50dbc04 to
3ae4b3cCompareIKholopov
commented
Jan 30, 2022
|
Uh oh!
There was an error while loading. Please reload this page.
| <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') }}"> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Ok thats fine. I don't want to expand the scope of this PR too much.
uranusjr
commented
Feb 1, 2022
Need to fix the static checks, but logic-wise lgtm. |
eladkal
commented
Feb 1, 2022
What this means for people who have airflow/airflow/config_templates/default_airflow.cfg Lines 578 to 579 in 4a73d8f |
| """Redirect from url param.""" | ||
| return redirect(url_for('Airflow.landing_times', **request.args)) | ||
| @expose('/dags/<string:dag_id>/landing_times') |
There was a problem hiding this comment.
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).
| @expose('/dags/<string:dag_id>/landing_times') | |
| @expose('/dags/<string:dag_id>/landing-times') |
IKholopov
commented
Feb 1, 2022
Fixed static checks (sorry, forgot about them:) and addressed @ryanahamilton comment. @eladkal DAG's |
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. |
Awesome work, congrats on your first merged pull request! |
…20730) Co-authored-by: Igor Kholopov <kholopovus@gmail.com>
related: #19944
/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/dags/<dag_id>->/dags/<dag_id>/gridOnce 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: