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 2020/10/22 10:19:29 UTC
[GitHub] [airflow] shivanshs9 opened a new pull request #11742: Fix: add support for no-menu plugin views (#10788)
shivanshs9 opened a new pull request #11742:
URL: https://github.com/apache/airflow/pull/11742
closes: #10788
old PR: #10987
Adds support in Airflow to add custom views from plugins without associating a menu entry. If "name" key is missing in the given plugin view, then it adds the view without menu, otherwise it behaves as usual by adding a view with menu.
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in 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] shivanshs9 commented on a change in pull request #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
shivanshs9 commented on a change in pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#discussion_r510527972
##########
File path: airflow/www/extensions/init_views.py
##########
@@ -73,8 +73,14 @@ def init_plugins(app):
appbuilder = app.appbuilder
for view in plugins_manager.flask_appbuilder_views:
- log.debug("Adding view %s", view["name"])
- appbuilder.add_view(view["view"], view["name"], category=view["category"])
+ try:
+ log.debug("Adding view %s with menu", view["name"])
Review comment:
Handled the case for empty string now. Do you suppose I should check for "None" value too?
----------------------------------------------------------------
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 #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#issuecomment-721445126
Can you please rebase your PR on latest Master since we have applied [Black](https://github.com/apache/airflow/commit/4e8f9cc8d02b29c325b8a5a76b4837671bdf5f68) and [PyUpgrade](https://github.com/apache/airflow/commit/8c42cf1b00c90f0d7f11b8a3a455381de8e003c5) on Master.
It will help if your squash your commits into single commit first so that there are less conflicts.
----------------------------------------------------------------
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 #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#issuecomment-723456268
[The Workflow run](https://github.com/apache/airflow/actions/runs/351186979) 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*.
----------------------------------------------------------------
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 #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#issuecomment-714415362
[The Workflow run](https://github.com/apache/airflow/actions/runs/321870976) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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] mik-laj commented on pull request #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#issuecomment-752796599
It looks like we have a problem with CI, so I did a rebase to master.
----------------------------------------------------------------
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] shivanshs9 commented on pull request #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
shivanshs9 commented on pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#issuecomment-750927635
@mik-laj finished the requested changes. I guess it's good to merge?
----------------------------------------------------------------
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 #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#issuecomment-715107256
[The Workflow run](https://github.com/apache/airflow/actions/runs/323687793) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#discussion_r510440451
##########
File path: airflow/www/extensions/init_views.py
##########
@@ -73,8 +73,14 @@ def init_plugins(app):
appbuilder = app.appbuilder
for view in plugins_manager.flask_appbuilder_views:
- log.debug("Adding view %s", view["name"])
- appbuilder.add_view(view["view"], view["name"], category=view["category"])
+ try:
+ log.debug("Adding view %s with menu", view["name"])
+ appbuilder.add_view(view["view"], view["name"], category=view["category"])
+ except KeyError as e:
+ if e.args[0] == 'name':
Review comment:
What is the expected behavior if the user does not provide the "view" key? It seems to me that in this case the exception will be silenced. It seems to me that in this case the exception will be silenced. This can lead to errors that are difficult to spot.
----------------------------------------------------------------
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 #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#issuecomment-715108907
[The Workflow run](https://github.com/apache/airflow/actions/runs/323690799) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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] shivanshs9 commented on a change in pull request #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
shivanshs9 commented on a change in pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#discussion_r510721062
##########
File path: airflow/www/extensions/init_views.py
##########
@@ -73,8 +73,14 @@ def init_plugins(app):
appbuilder = app.appbuilder
for view in plugins_manager.flask_appbuilder_views:
- log.debug("Adding view %s", view["name"])
- appbuilder.add_view(view["view"], view["name"], category=view["category"])
+ try:
+ log.debug("Adding view %s with menu", view["name"])
+ appbuilder.add_view(view["view"], view["name"], category=view["category"])
+ except KeyError as e:
+ if e.args[0] == 'name':
Review comment:
@turbaszek Yeah, that'd work too and I guess, it's better in terms of readability. I was just concerned about following python's EAFP principle here, but I guess it defeats the purpose if we're anyways checking for the "name" value. :thinking:
----------------------------------------------------------------
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] shivanshs9 commented on a change in pull request #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
shivanshs9 commented on a change in pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#discussion_r510522241
##########
File path: airflow/www/extensions/init_views.py
##########
@@ -73,8 +73,14 @@ def init_plugins(app):
appbuilder = app.appbuilder
for view in plugins_manager.flask_appbuilder_views:
- log.debug("Adding view %s", view["name"])
- appbuilder.add_view(view["view"], view["name"], category=view["category"])
+ try:
+ log.debug("Adding view %s with menu", view["name"])
+ appbuilder.add_view(view["view"], view["name"], category=view["category"])
+ except KeyError as e:
+ if e.args[0] == 'name':
Review comment:
Right, it should've re-raise exception in that case. Missed this one.
----------------------------------------------------------------
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] shivanshs9 commented on a change in pull request #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
shivanshs9 commented on a change in pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#discussion_r548621557
##########
File path: airflow/www/extensions/init_views.py
##########
@@ -73,8 +73,14 @@ def init_plugins(app):
appbuilder = app.appbuilder
for view in plugins_manager.flask_appbuilder_views:
- log.debug("Adding view %s", view["name"])
- appbuilder.add_view(view["view"], view["name"], category=view["category"])
+ try:
+ log.debug("Adding view %s with menu", view["name"])
+ appbuilder.add_view(view["view"], view["name"], category=view["category"])
+ except KeyError as e:
+ if e.args[0] == 'name':
Review comment:
@turbaszek implemented your suggestion :+1:
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#discussion_r510439124
##########
File path: airflow/www/extensions/init_views.py
##########
@@ -73,8 +73,14 @@ def init_plugins(app):
appbuilder = app.appbuilder
for view in plugins_manager.flask_appbuilder_views:
- log.debug("Adding view %s", view["name"])
- appbuilder.add_view(view["view"], view["name"], category=view["category"])
+ try:
+ log.debug("Adding view %s with menu", view["name"])
Review comment:
What do you think should be the behavior when the user passes "None" in the "name" key?
----------------------------------------------------------------
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 #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#issuecomment-715242311
[The Workflow run](https://github.com/apache/airflow/actions/runs/323902067) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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] shivanshs9 commented on pull request #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
shivanshs9 commented on pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#issuecomment-723453474
> Can you please rebase your PR on latest Master since we have applied [Black](https://github.com/apache/airflow/commit/4e8f9cc8d02b29c325b8a5a76b4837671bdf5f68) and [PyUpgrade](https://github.com/apache/airflow/commit/8c42cf1b00c90f0d7f11b8a3a455381de8e003c5) on Master.
>
> It will help if your squash your commits into single commit first so that there are less conflicts.
@kaxil done! :+1:
----------------------------------------------------------------
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] mik-laj commented on pull request #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#issuecomment-749611161
@shivanshs9 This change is nearing completion. We only have one open discussion - https://github.com/apache/airflow/pull/11742#discussion_r510440451 . Do you need any help from us? I am happy to help if you have any questions.
----------------------------------------------------------------
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] turbaszek commented on a change in pull request #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#discussion_r510689113
##########
File path: airflow/www/extensions/init_views.py
##########
@@ -73,8 +73,14 @@ def init_plugins(app):
appbuilder = app.appbuilder
for view in plugins_manager.flask_appbuilder_views:
- log.debug("Adding view %s", view["name"])
- appbuilder.add_view(view["view"], view["name"], category=view["category"])
+ try:
+ log.debug("Adding view %s with menu", view["name"])
+ appbuilder.add_view(view["view"], view["name"], category=view["category"])
+ except KeyError as e:
+ if e.args[0] == 'name':
Review comment:
How about:
```py
name = view.get("name")
if name:
log.debug("Adding view %s with menu", name)
appbuilder.add_view(view["view"], name, category=view["category"])
else:
# if 'name' key is missing, intent is to add view without menu
log.debug("Adding view %s without menu", str(type(view["view"])))
appbuilder.add_view_no_menu(view["view"])
```
----------------------------------------------------------------
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 #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#issuecomment-751253050
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, 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] mik-laj merged pull request #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
mik-laj merged pull request #11742:
URL: https://github.com/apache/airflow/pull/11742
----------------------------------------------------------------
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] shivanshs9 commented on a change in pull request #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
shivanshs9 commented on a change in pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#discussion_r510528017
##########
File path: airflow/www/extensions/init_views.py
##########
@@ -73,8 +73,14 @@ def init_plugins(app):
appbuilder = app.appbuilder
for view in plugins_manager.flask_appbuilder_views:
- log.debug("Adding view %s", view["name"])
- appbuilder.add_view(view["view"], view["name"], category=view["category"])
+ try:
+ log.debug("Adding view %s with menu", view["name"])
+ appbuilder.add_view(view["view"], view["name"], category=view["category"])
+ except KeyError as e:
+ if e.args[0] == 'name':
Review comment:
Fixed.
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#discussion_r510440451
##########
File path: airflow/www/extensions/init_views.py
##########
@@ -73,8 +73,14 @@ def init_plugins(app):
appbuilder = app.appbuilder
for view in plugins_manager.flask_appbuilder_views:
- log.debug("Adding view %s", view["name"])
- appbuilder.add_view(view["view"], view["name"], category=view["category"])
+ try:
+ log.debug("Adding view %s with menu", view["name"])
+ appbuilder.add_view(view["view"], view["name"], category=view["category"])
+ except KeyError as e:
+ if e.args[0] == 'name':
Review comment:
What is the expected behavior if the user does not provide the "view" key? It seems to me that in this case the exception will be silenced. It seems to me that in this case the exception will be silenced.
----------------------------------------------------------------
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] shivanshs9 commented on pull request #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
shivanshs9 commented on pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#issuecomment-752403507
> LGTM, but some static checks are sad. Can you fix it?
@mik-laj And done!
----------------------------------------------------------------
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] shivanshs9 commented on a change in pull request #11742: Fix: add support for no-menu plugin views (#10788)
Posted by GitBox <gi...@apache.org>.
shivanshs9 commented on a change in pull request #11742:
URL: https://github.com/apache/airflow/pull/11742#discussion_r510523634
##########
File path: airflow/www/extensions/init_views.py
##########
@@ -73,8 +73,14 @@ def init_plugins(app):
appbuilder = app.appbuilder
for view in plugins_manager.flask_appbuilder_views:
- log.debug("Adding view %s", view["name"])
- appbuilder.add_view(view["view"], view["name"], category=view["category"])
+ try:
+ log.debug("Adding view %s with menu", view["name"])
Review comment:
Hmm.. It's technically a valid string, I doubt the user would expect that to actually mean None.
It might be more possible to user to pass an empty string though.
----------------------------------------------------------------
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