You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/01/24 21:10:14 UTC
[GitHub] [airflow] XD-DENG opened a new pull request #13884: Fix wrong parameter for drawDagStatsForDag() in dags.html
XD-DENG opened a new pull request #13884:
URL: https://github.com/apache/airflow/pull/13884
I believe this is a typo. It should be `states` rather than `stats`.
Please refer to
https://github.com/apache/airflow/blob/326c74e60871736f43c303068b0aaa5927c50de0/airflow/www/templates/airflow/dags.html#L398
and the signature of similar function `drawTaskStatsForDag()`
https://github.com/apache/airflow/blob/326c74e60871736f43c303068b0aaa5927c50de0/airflow/www/templates/airflow/dags.html#L467
_**So far this is not causing any visible error/issue because of how scoping works in JavaScript.**_
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG merged pull request #13884: Fix wrong parameter for drawDagStatsForDag() in dags.html
Posted by GitBox <gi...@apache.org>.
XD-DENG merged pull request #13884:
URL: https://github.com/apache/airflow/pull/13884
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #13884: Fix wrong parameter for drawDagStatsForDag() in dags.html
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #13884:
URL: https://github.com/apache/airflow/pull/13884#issuecomment-766808559
cc @ryanahamilton
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #13884: Fix wrong parameter for drawDagStatsForDag() in dags.html
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #13884:
URL: https://github.com/apache/airflow/pull/13884#issuecomment-766808559
cc @ryanahamilton
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on pull request #13884: Fix wrong parameter for drawDagStatsForDag() in dags.html
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on pull request #13884:
URL: https://github.com/apache/airflow/pull/13884#issuecomment-766845097
This is certainly a typo. With and without the fix, I see no issues in the browser. Would take a bit more effort to determine if/why it is necessary to be there at all.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on pull request #13884: Fix wrong parameter for drawDagStatsForDag() in dags.html
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on pull request #13884:
URL: https://github.com/apache/airflow/pull/13884#issuecomment-766848218
> This is certainly a typo. With and without the fix, I see no issues in the browser. Would take a bit more effort to determine if/why it is necessary to be there at all.
Hi @ryanahamilton , yes you are right, there will be no issue without this fix. Because of the typo, this parameter is not useful/helpful at all. When `.data(states)` is called, it cannot find `states` in its current block, so it goes one level up, and `states` is available there https://github.com/apache/airflow/blob/326c74e60871736f43c303068b0aaa5927c50de0/airflow/www/templates/airflow/dags.html#L460-L463
This may become a potential source of error/confusion though in the future, so fixing it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on pull request #13884: Fix wrong parameter for drawDagStatsForDag() in dags.html
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on pull request #13884:
URL: https://github.com/apache/airflow/pull/13884#issuecomment-766845097
This is certainly a typo. With and without the fix, I see no issues in the browser. Would take a bit more effort to determine if/why it is necessary to be there at all.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG merged pull request #13884: Fix wrong parameter for drawDagStatsForDag() in dags.html
Posted by GitBox <gi...@apache.org>.
XD-DENG merged pull request #13884:
URL: https://github.com/apache/airflow/pull/13884
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13884: Fix wrong parameter for drawDagStatsForDag() in dags.html
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13884:
URL: https://github.com/apache/airflow/pull/13884#issuecomment-766849977
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 master or amend the last commit of the PR, and push it with --force-with-lease.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13884: Fix wrong parameter for drawDagStatsForDag() in dags.html
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13884:
URL: https://github.com/apache/airflow/pull/13884#issuecomment-766849977
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 master or amend the last commit of the PR, and push it with --force-with-lease.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on pull request #13884: Fix wrong parameter for drawDagStatsForDag() in dags.html
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on pull request #13884:
URL: https://github.com/apache/airflow/pull/13884#issuecomment-766848218
> This is certainly a typo. With and without the fix, I see no issues in the browser. Would take a bit more effort to determine if/why it is necessary to be there at all.
Hi @ryanahamilton , yes you are right, there will be no issue without this fix. Because of the typo, this parameter is not useful/helpful at all. When `.data(states)` is called, it cannot find `states` in its current block, so it goes one level up, and `states` is available there https://github.com/apache/airflow/blob/326c74e60871736f43c303068b0aaa5927c50de0/airflow/www/templates/airflow/dags.html#L460-L463
This may become a potential source of error/confusion though in the future, so fixing it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org