Uh oh!
There was an error while loading. Please reload this page.
Deactivate trigger, refresh, and delete controls on dag detail view. - #14144
Conversation
jhtimmins
commented
Feb 9, 2021
ryanahamilton
left a comment
There was a problem hiding this comment.
The nittiest of nitpicks
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
jhtimmins
commented
Feb 9, 2021
Thanks @ryanahamilton |
ryanahamilton
left a comment
There was a problem hiding this comment.
I know you had previously asked me if there was anywhere else this needed to be updated… Unfortunately my previous response ("no") was wrong. The Task Instance views also inherit the same dag.html, so you're going to need to add _add_user_permissions_to_dag(dag) to each of the following methods in views.py too:
taskrendered_templatesrendered_k8slogxcom
3674ca6 to
4999b4bCompare4999b4b to
98fdb68Compare
ashb
left a comment
There was a problem hiding this comment.
I wonder if it's worth using the before_render_template flask signal to automatically inject this in all templates that have a dag object in the context? https://flask.palletsprojects.com/en/1.1.x/api/?highlight=before_render_template#flask.signals.signals_available
ryanahamilton
left a comment
There was a problem hiding this comment.
My changes were addressed. Will leave it to you to determine if the additional refactoring suggested by Ash should be included in this PR.
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. |
a8ec497 to
362f638Comparejhtimmins
commented
Mar 2, 2021
@ashb updated to use the signal |
412b4c7 to
a9fc88dComparea9fc88d to
6f736d1CompareThe Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
The Workflow run is cancelling this PR. Building image for the PR has been cancelled |
6ef8afc to
eb44807CompareThe 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. |
kaxil
commented
Mar 8, 2021
Tests are failing @jhtimmins |
eb44807 to
66d895cCompare66d895c to
c8520d6CompareCo-authored-by: Ryan Hamilton <ryan@ryanahamilton.com>
Co-authored-by: Ryan Hamilton <ryan@ryanahamilton.com>
c8520d6 to
a11cf76CompareThe Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
This PR fixes a problem introduced by apache#14144 This is a very weird and unforeseen issue. The change introduced a new import from flask `before_render_template` and this caused flask to require `blinker` dependency, even if it was not specified before as 'required' by flask. We have not seen it before, because changes to this part of the code do not trigger K8S tests, however subsequent PRs started to fail because the setup.py did not have `blinker` as dependency. However in CI image `blinker` was installed because it is needed by sentry. So the problem was only detectable in the production image. This is an ultimate proof that our test harness is really good in catchig this kind of errors. The root cause for it is described in https://stackoverflow.com/questions/38491075/flask-testing-signals-not-supported-error Flask support for signals is optional and it does not blinker as dependency, but importing some parts of flask triggers the need for signals.
This PR fixes a problem introduced by #14144 This is a very weird and unforeseen issue. The change introduced a new import from flask `before_render_template` and this caused flask to require `blinker` dependency, even if it was not specified before as 'required' by flask. We have not seen it before, because changes to this part of the code do not trigger K8S tests, however subsequent PRs started to fail because the setup.py did not have `blinker` as dependency. However in CI image `blinker` was installed because it is needed by sentry. So the problem was only detectable in the production image. This is an ultimate proof that our test harness is really good in catchig this kind of errors. The root cause for it is described in https://stackoverflow.com/questions/38491075/flask-testing-signals-not-supported-error Flask support for signals is optional and it does not blinker as dependency, but importing some parts of flask triggers the need for signals.
This PR fixes a problem introduced by #14144 This is a very weird and unforeseen issue. The change introduced a new import from flask `before_render_template` and this caused flask to require `blinker` dependency, even if it was not specified before as 'required' by flask. We have not seen it before, because changes to this part of the code do not trigger K8S tests, however subsequent PRs started to fail because the setup.py did not have `blinker` as dependency. However in CI image `blinker` was installed because it is needed by sentry. So the problem was only detectable in the production image. This is an ultimate proof that our test harness is really good in catchig this kind of errors. The root cause for it is described in https://stackoverflow.com/questions/38491075/flask-testing-signals-not-supported-error Flask support for signals is optional and it does not blinker as dependency, but importing some parts of flask triggers the need for signals. (cherry picked from commit 437850b)
This PR fixes a problem introduced by #14144 This is a very weird and unforeseen issue. The change introduced a new import from flask `before_render_template` and this caused flask to require `blinker` dependency, even if it was not specified before as 'required' by flask. We have not seen it before, because changes to this part of the code do not trigger K8S tests, however subsequent PRs started to fail because the setup.py did not have `blinker` as dependency. However in CI image `blinker` was installed because it is needed by sentry. So the problem was only detectable in the production image. This is an ultimate proof that our test harness is really good in catchig this kind of errors. The root cause for it is described in https://stackoverflow.com/questions/38491075/flask-testing-signals-not-supported-error Flask support for signals is optional and it does not blinker as dependency, but importing some parts of flask triggers the need for signals. (cherry picked from commit 437850b)
Deactivate the trigger, refresh, and delete DAG buttons on the DAG details pages if the current user doesn't have the necessary permissions to perform those actions. This was added to the DAGs list page in 2.0.1, and is now being added to the detail page as well (not including it before was an oversight).