Deactivate trigger, refresh, and delete controls on dag detail view. #14144

jhtimmins · 2021-02-09T02:12:45Z

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).

jhtimmins · 2021-02-09T02:12:58Z

@kaxil @ashb @ryanahamilton

ryanahamilton

The nittiest of nitpicks

airflow/www/templates/airflow/dag.html

jhtimmins · 2021-02-09T03:27:25Z

Thanks @ryanahamilton

ryanahamilton

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:

task
rendered_templates
rendered_k8s
log
xcom

ashb

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

My changes were addressed. Will leave it to you to determine if the additional refactoring suggested by Ash should be included in this PR.

github-actions · 2021-03-01T14:41:33Z

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.

jhtimmins · 2021-03-02T04:12:09Z

@ashb updated to use the signal

github-actions · 2021-03-04T01:17:54Z

The 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*.

github-actions · 2021-03-04T01:18:35Z

The Workflow run is cancelling this PR. Building image for the PR has been cancelled

github-actions · 2021-03-05T17:33:48Z

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.

kaxil · 2021-03-08T21:28:44Z

Tests are failing @jhtimmins

Co-authored-by: Ryan Hamilton <[email protected]>

github-actions · 2021-03-24T22:38:41Z

The 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)

boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Feb 9, 2021

ryanahamilton reviewed Feb 9, 2021

View reviewed changes

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

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

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

ryanahamilton requested changes Feb 9, 2021

View reviewed changes

jhtimmins force-pushed the deactivate-dag-controls-on-detail-view branch from 3674ca6 to 4999b4b Compare February 9, 2021 22:38

jhtimmins force-pushed the deactivate-dag-controls-on-detail-view branch from 4999b4b to 98fdb68 Compare February 22, 2021 23:05

jhtimmins requested a review from ashb as a code owner February 22, 2021 23:05

ashb reviewed Feb 23, 2021

View reviewed changes

ryanahamilton approved these changes Mar 1, 2021

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 Mar 1, 2021

ryanahamilton removed the okay to merge It's ok to merge this PR as it does not require more tests label Mar 1, 2021

jhtimmins force-pushed the deactivate-dag-controls-on-detail-view branch 2 times, most recently from a8ec497 to 362f638 Compare March 2, 2021 03:00

jhtimmins force-pushed the deactivate-dag-controls-on-detail-view branch 2 times, most recently from 412b4c7 to a9fc88d Compare March 4, 2021 00:39

jhtimmins requested review from XD-DENG, kaxil and turbaszek as code owners March 4, 2021 00:39

jhtimmins force-pushed the deactivate-dag-controls-on-detail-view branch from a9fc88d to 6f736d1 Compare March 4, 2021 01:00

jhtimmins force-pushed the deactivate-dag-controls-on-detail-view branch from 6ef8afc to eb44807 Compare March 4, 2021 01:33

ashb approved these changes Mar 5, 2021

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 Mar 5, 2021

jhtimmins force-pushed the deactivate-dag-controls-on-detail-view branch from eb44807 to 66d895c Compare March 9, 2021 20:47

jhtimmins force-pushed the deactivate-dag-controls-on-detail-view branch from 66d895c to c8520d6 Compare March 24, 2021 03:33

jhtimmins and others added 5 commits March 24, 2021 15:10

Deactivate trigger, refresh, and delete controls on dag detail view.

0bc2028

Add task-level templates that use dag.html.

920b189

Update airflow/www/templates/airflow/dag.html

65b326e

Co-authored-by: Ryan Hamilton <[email protected]>

Update airflow/www/templates/airflow/dag.html

5242363

Co-authored-by: Ryan Hamilton <[email protected]>

Add back missing files.

a11cf76

jhtimmins force-pushed the deactivate-dag-controls-on-detail-view branch from c8520d6 to a11cf76 Compare March 24, 2021 22:10

kaxil approved these changes Apr 3, 2021

View reviewed changes

kaxil added this to the Airflow 2.1 milestone Apr 3, 2021

kaxil merged commit 3a38134 into apache:master Apr 3, 2021

kaxil deleted the deactivate-dag-controls-on-detail-view branch April 3, 2021 00:10

kaxil modified the milestones: Airflow 2.1, Airflow 2.0.3 Apr 3, 2021

potiuk mentioned this pull request Apr 3, 2021

Adds Blinker dependency which is missing after recent changes #15182

Merged

potiuk removed this from the Airflow 2.0.3 milestone May 9, 2021

Deactivate trigger, refresh, and delete controls on dag detail view. #14144

Deactivate trigger, refresh, and delete controls on dag detail view. #14144

Uh oh!

Conversation

jhtimmins commented Feb 9, 2021

Uh oh!

jhtimmins commented Feb 9, 2021

Uh oh!

ryanahamilton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhtimmins commented Feb 9, 2021

Uh oh!

ryanahamilton left a comment

Choose a reason for hiding this comment

Uh oh!

ashb left a comment

Choose a reason for hiding this comment

Uh oh!

ryanahamilton left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 1, 2021

Uh oh!

jhtimmins commented Mar 2, 2021

Uh oh!

github-actions bot commented Mar 4, 2021

Uh oh!

github-actions bot commented Mar 4, 2021

Uh oh!

github-actions bot commented Mar 5, 2021

Uh oh!

kaxil commented Mar 8, 2021

Uh oh!

github-actions bot commented Mar 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants