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/04/15 18:40:07 UTC

[GitHub] [airflow] ShakaibKhan opened a new pull request #15385: Added table to view providers in airflow ui under admin tab

ShakaibKhan opened a new pull request #15385:
URL: https://github.com/apache/airflow/pull/15385


   Added page in Airflow UI for provider package info
   ![Screen Shot 2021-04-15 at 2 05 59 PM](https://user-images.githubusercontent.com/10384742/114921125-33b6cb80-9df8-11eb-9d56-5024d76c56ce.png)
   
   


-- 
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] ShakaibKhan commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
ShakaibKhan commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-898104022


   Failing the static check "Don't use safe in templates". I was wondering if there was another way to render html passed to a template?


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] boring-cyborg[bot] commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-902091256


   Awesome work, congrats on your first merged pull request!
   


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] jedcunningham commented on a change in pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#discussion_r688641009



##########
File path: tests/www/views/test_views.py
##########
@@ -93,6 +95,28 @@ def test_plugin_endpoint_should_not_be_unauthenticated(app):
     check_content_in_response("Sign In - Airflow", resp)
 
 
+def test_should_list_providers_on_page_with_details(admin_client):
+    pm = ProvidersManager()
+    resp = admin_client.get('/provider')
+
+    for pi in pm.providers.values():
+        check_content_in_response("<td>" + pi.version + "</td>", resp)
+        check_content_in_response("<td>" + pi[1]["package-name"] + "</td>", resp)
+        cd = re.sub(
+            ">",
+            "\">[site]</a>",
+            re.sub("<", "<a href=\"", re.sub("[`_]", "", pi[1]["description"].strip(" \n.").strip("\""))),
+        )

Review comment:
       I'm thinking instead of duplicating this logic, maybe we test a specific providers stuff is built properly with static values here? Thoughts?

##########
File path: airflow/www/views.py
##########
@@ -3339,6 +3339,58 @@ def list(self):
         )
 
 
+class ProviderView(AirflowBaseView):
+    """View to show Airflow Providers"""
+
+    default_view = 'list'
+
+    class_permission_name = permissions.RESOURCE_PROVIDER
+
+    method_permission_name = {
+        'list': 'read',
+    }
+
+    base_permissions = [
+        permissions.ACTION_CAN_READ,
+        permissions.ACTION_CAN_ACCESS_MENU,
+    ]
+
+    @expose('/provider')
+    @auth.has_access(
+        [
+            (permissions.ACTION_CAN_READ, permissions.RESOURCE_PROVIDER),
+        ]
+    )
+    def list(self):
+        """List providers."""
+        providers_manager = ProvidersManager()
+
+        providers = []
+        for pi in providers_manager.providers.values():
+            provider_info = pi[1]
+            provider_data = {
+                "package_name": provider_info["package-name"],
+                "description": self._clean_description(provider_info["description"]),
+                "version": pi[0],
+            }
+            providers.append(provider_data)
+
+        title = "Providers"
+        doc_url = get_docs_url("apache-airflow-providers/index.html")
+        return self.render_template(
+            'airflow/providers.html',
+            providers=providers,
+            title=title,
+            doc_url=doc_url,
+        )
+
+    def _clean_description(self, description):
+        cd = re.sub("[`_]", "", description.strip(" \n.").strip("\""))
+        cd = re.sub("<", "<a href=\"", cd)
+        cd = re.sub(">", "\">[site]</a>", cd)
+        return cd

Review comment:
       ```suggestion
           import markupsafe  # Should be moved to the top of the file
   
           def _build_link(matchobj):
               # These were already escaped, so we can use them as-is
               text = matchobj.group(1)
               url = matchobj.group(2)
               return markupsafe.Markup(f'<a href="{url}">{text}</a>')
   
           safe = markupsafe.escape(description)
           safe = re.sub(r"`(.*) +&lt;(.*)&gt;`__", _build_link, safe)
           safe = re.sub(r"\n", r"<br>", safe)
           return markupsafe.Markup(safe)
   ```
   
   This will allow you to remove the `safe` filter, plus I think it renders the links more nicely, e.g:
   ![Screen Shot 2021-08-13 at 10 27 35 AM](https://user-images.githubusercontent.com/66968678/129390631-e7e2f343-aeea-4486-893a-4a61f54ad1c2.png)
   
   ![Screen Shot 2021-08-13 at 10 27 45 AM](https://user-images.githubusercontent.com/66968678/129390706-7cc80b4c-2736-4388-a992-993e0d7c7ef3.png)
   
   
   




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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] jedcunningham commented on a change in pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#discussion_r686214152



##########
File path: airflow/www/views.py
##########
@@ -3271,6 +3271,58 @@ def list(self):
         )
 
 
+class ProviderView(AirflowBaseView):
+    """View to show Airflow Providers"""
+
+    default_view = 'list'
+
+    class_permission_name = permissions.RESOURCE_PROVIDER
+
+    method_permission_name = {
+        'list': 'read',
+    }
+
+    base_permissions = [
+        permissions.ACTION_CAN_READ,
+        permissions.ACTION_CAN_ACCESS_MENU,
+    ]
+
+    @expose('/provider')
+    @auth.has_access(
+        [
+            (permissions.ACTION_CAN_READ, permissions.RESOURCE_PROVIDER),
+        ]
+    )
+    def list(self):
+        """List providers."""
+        providers_manager = ProvidersManager()
+
+        providers = []
+        for pi in providers_manager.providers.values():
+            provider_info = pi[1]
+            provider_data = {
+                "package_name": provider_info["package-name"],
+                "description": self._clean_description(provider_info["description"]),
+                "version": pi[0],
+            }
+            providers.append(provider_data)
+
+        title = "Providers"
+        doc_url = get_docs_url("apache-airflow-providers/index.html")
+        return self.render_template(
+            'airflow/providers.html',
+            providers=providers,
+            title=title,
+            doc_url=doc_url,
+        )
+
+    def _clean_description(self, description):
+        cd = re.sub("[`_]", "", description.strip(" \n.").strip("\""))
+        cd = re.sub("<", "<a href=\"", cd)
+        cd = re.sub(">", "\">[site]<a/>", cd)

Review comment:
       ```suggestion
           cd = re.sub(">", "\">[site]</a>", cd)
   ```
   
   Simple typo but it really breaks the table by merging rows together :)
   
   Probably worth adding a test to make sure the links are rendered properly.




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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-902095202


   ❤️ 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.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] github-actions[bot] commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-842175542


   [The Workflow run](https://github.com/apache/airflow/actions/runs/849193584) 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 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] jhtimmins commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-839360528


   @ShakaibKhan It looks like this needs another rebase on master, since it's currently showing a lot of changes from other commits. 


-- 
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 #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-842175542


   [The Workflow run](https://github.com/apache/airflow/actions/runs/849193584) 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 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] jedcunningham commented on a change in pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#discussion_r681159977



##########
File path: airflow/www/views.py
##########
@@ -3271,6 +3271,58 @@ def list(self):
         )
 
 
+class ProvidersView(AirflowBaseView):

Review comment:
       ```suggestion
   class ProviderView(AirflowBaseView):
   ```
   
   I think this should be singular.

##########
File path: airflow/www/extensions/init_views.py
##########
@@ -91,6 +91,9 @@ def init_appbuilder_views(app):
     appbuilder.add_view(
         views.PluginView, permissions.RESOURCE_PLUGIN, category=permissions.RESOURCE_ADMIN_MENU
     )
+    appbuilder.add_view(
+        views.ProvidersView, permissions.RESOURCE_PROVIDER, category=permissions.RESOURCE_ADMIN_MENU

Review comment:
       ```suggestion
           views.ProviderView, permissions.RESOURCE_PROVIDER, category=permissions.RESOURCE_ADMIN_MENU
   ```

##########
File path: airflow/www/views.py
##########
@@ -3271,6 +3271,58 @@ def list(self):
         )
 
 
+class ProvidersView(AirflowBaseView):
+    """View to show Airflow Providers"""
+
+    default_view = 'list'
+
+    class_permission_name = permissions.RESOURCE_PROVIDER
+
+    method_permission_name = {
+        'list': 'read',
+    }
+
+    base_permissions = [
+        permissions.ACTION_CAN_READ,
+        permissions.ACTION_CAN_ACCESS_MENU,
+    ]
+
+    @expose('/providers')

Review comment:
       ```suggestion
       @expose('/provider')
   ```




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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] jedcunningham commented on a change in pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#discussion_r688750350



##########
File path: airflow/www/views.py
##########
@@ -3339,6 +3339,58 @@ def list(self):
         )
 
 
+class ProviderView(AirflowBaseView):
+    """View to show Airflow Providers"""
+
+    default_view = 'list'
+
+    class_permission_name = permissions.RESOURCE_PROVIDER
+
+    method_permission_name = {
+        'list': 'read',
+    }
+
+    base_permissions = [
+        permissions.ACTION_CAN_READ,
+        permissions.ACTION_CAN_ACCESS_MENU,
+    ]
+
+    @expose('/provider')
+    @auth.has_access(
+        [
+            (permissions.ACTION_CAN_READ, permissions.RESOURCE_PROVIDER),
+        ]
+    )
+    def list(self):
+        """List providers."""
+        providers_manager = ProvidersManager()
+
+        providers = []
+        for pi in providers_manager.providers.values():
+            provider_info = pi[1]
+            provider_data = {
+                "package_name": provider_info["package-name"],
+                "description": self._clean_description(provider_info["description"]),
+                "version": pi[0],
+            }
+            providers.append(provider_data)
+
+        title = "Providers"
+        doc_url = get_docs_url("apache-airflow-providers/index.html")
+        return self.render_template(
+            'airflow/providers.html',
+            providers=providers,
+            title=title,
+            doc_url=doc_url,
+        )
+
+    def _clean_description(self, description):
+        cd = re.sub("[`_]", "", description.strip(" \n.").strip("\""))
+        cd = re.sub("<", "<a href=\"", cd)
+        cd = re.sub(">", "\">[site]</a>", cd)
+        return cd

Review comment:
       Fwiw, this might need a little more effort. 1 providers link doesn't work right with this, and I thought it was the provider itself but after looking closer I dont think thats the case.




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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil merged pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #15385:
URL: https://github.com/apache/airflow/pull/15385


   


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ShakaibKhan commented on pull request #15385: [WIP] Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
ShakaibKhan commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-828107361


   > Is there a way that we could parse the descriptions to convert the URLs (that most of them contain) to actual links?
   
   Tried implementing this with adding some anchor tags around links but having issues with jinja adding quotes to multiline descriptions


-- 
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] ShakaibKhan commented on pull request #15385: [WIP] Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
ShakaibKhan commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-832207378


   Made readability changes and rebased but am uncertain how to check for permissions in the views_test.py


-- 
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 #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-902091605


   Thank you for your contribution @ShakaibKhan 


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ShakaibKhan commented on a change in pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
ShakaibKhan commented on a change in pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#discussion_r688718313



##########
File path: tests/www/views/test_views.py
##########
@@ -93,6 +95,28 @@ def test_plugin_endpoint_should_not_be_unauthenticated(app):
     check_content_in_response("Sign In - Airflow", resp)
 
 
+def test_should_list_providers_on_page_with_details(admin_client):
+    pm = ProvidersManager()
+    resp = admin_client.get('/provider')
+
+    for pi in pm.providers.values():
+        check_content_in_response("<td>" + pi.version + "</td>", resp)
+        check_content_in_response("<td>" + pi[1]["package-name"] + "</td>", resp)
+        cd = re.sub(
+            ">",
+            "\">[site]</a>",
+            re.sub("<", "<a href=\"", re.sub("[`_]", "", pi[1]["description"].strip(" \n.").strip("\""))),
+        )

Review comment:
       Yeah I agree that I probably shouldnt repeat the logic here. Correct me if I am wrong but should the test look more like checking for a provider that we know is provided and check it is rendered on the page (w/ static values)?




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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] github-actions[bot] commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-833744416


   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] xinbinhuang commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-820804802


   @ShakaibKhan Thanks for the contribution! Love this. 
   
   Can you rebase to master, please :)


-- 
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] jedcunningham commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-899779951


   @ShakaibKhan, some static check failures again, otherwise this LGTM!


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] github-actions[bot] commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-835212074


   [The Workflow run](https://github.com/apache/airflow/actions/runs/822696102) 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 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] potiuk commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-900204199


   There was a refactor in provider's manager so you will have to rebase and fix conflict. I also recommend you to use `pre-commit` and squash everything to single commit during the rebase -  pre-commit will automatically fix most of the static code problems for you https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil commented on a change in pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#discussion_r692126612



##########
File path: tests/www/views/test_views.py
##########
@@ -93,6 +93,24 @@ def test_plugin_endpoint_should_not_be_unauthenticated(app):
     check_content_in_response("Sign In - Airflow", resp)
 
 
+# cd = re.sub("[`_]", "", description.strip(" \n.").strip("\""))
+# cd = re.sub("<", "<a href=\"", cd)
+# cd = re.sub(">", "\">[site]</a>", cd)

Review comment:
       Is this intentionally added or just left after debugging




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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] mik-laj commented on a change in pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#discussion_r614773231



##########
File path: airflow/www/views.py
##########
@@ -71,6 +71,7 @@
     set_dag_run_state_to_failed,
     set_dag_run_state_to_success,
 )
+from airflow.cli.commands.provider_command import _remove_rst_syntax

Review comment:
       This import looks weird. I think, we should move this function to utils. 




-- 
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] potiuk closed pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #15385:
URL: https://github.com/apache/airflow/pull/15385


   


-- 
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] jhtimmins commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-834027380


   @ShakaibKhan Looks like a test is failing since you added another view that adds an additional query. If you can just increment the number of expected queries in the test that should fix 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] jhtimmins commented on pull request #15385: [WIP] Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-833237576


   > Made readability changes and rebased but am uncertain how to check for permissions in the views_test.py
   
   Thanks @ShakaibKhan. You can call `.create_user_and_login` (as seen [here](https://github.com/apache/airflow/blob/master/tests/www/test_views.py#L2162)) to create a user with specific permissions. So if you can call that in `test_should_list_providers_on_page_with_details` to give them the necessary permissions, and then add another test where you create a user but don't give them any permissions, so they've authenticated but not authorized.


-- 
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] jedcunningham commented on a change in pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#discussion_r688745364



##########
File path: tests/www/views/test_views.py
##########
@@ -93,6 +95,28 @@ def test_plugin_endpoint_should_not_be_unauthenticated(app):
     check_content_in_response("Sign In - Airflow", resp)
 
 
+def test_should_list_providers_on_page_with_details(admin_client):
+    pm = ProvidersManager()
+    resp = admin_client.get('/provider')
+
+    for pi in pm.providers.values():
+        check_content_in_response("<td>" + pi.version + "</td>", resp)
+        check_content_in_response("<td>" + pi[1]["package-name"] + "</td>", resp)
+        cd = re.sub(
+            ">",
+            "\">[site]</a>",
+            re.sub("<", "<a href=\"", re.sub("[`_]", "", pi[1]["description"].strip(" \n.").strip("\""))),
+        )

Review comment:
       Yeah, that's what I was envisioning.




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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-875051551


   Can you please rebase this one to latest main @ShakaibKhan ?


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-842149077






-- 
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] ShakaibKhan commented on a change in pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
ShakaibKhan commented on a change in pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#discussion_r688717069



##########
File path: airflow/www/views.py
##########
@@ -3339,6 +3339,58 @@ def list(self):
         )
 
 
+class ProviderView(AirflowBaseView):
+    """View to show Airflow Providers"""
+
+    default_view = 'list'
+
+    class_permission_name = permissions.RESOURCE_PROVIDER
+
+    method_permission_name = {
+        'list': 'read',
+    }
+
+    base_permissions = [
+        permissions.ACTION_CAN_READ,
+        permissions.ACTION_CAN_ACCESS_MENU,
+    ]
+
+    @expose('/provider')
+    @auth.has_access(
+        [
+            (permissions.ACTION_CAN_READ, permissions.RESOURCE_PROVIDER),
+        ]
+    )
+    def list(self):
+        """List providers."""
+        providers_manager = ProvidersManager()
+
+        providers = []
+        for pi in providers_manager.providers.values():
+            provider_info = pi[1]
+            provider_data = {
+                "package_name": provider_info["package-name"],
+                "description": self._clean_description(provider_info["description"]),
+                "version": pi[0],
+            }
+            providers.append(provider_data)
+
+        title = "Providers"
+        doc_url = get_docs_url("apache-airflow-providers/index.html")
+        return self.render_template(
+            'airflow/providers.html',
+            providers=providers,
+            title=title,
+            doc_url=doc_url,
+        )
+
+    def _clean_description(self, description):
+        cd = re.sub("[`_]", "", description.strip(" \n.").strip("\""))
+        cd = re.sub("<", "<a href=\"", cd)
+        cd = re.sub(">", "\">[site]</a>", cd)
+        return cd

Review comment:
       Agree that this looks cleaner and safer than safe in a template 👍 




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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ShakaibKhan commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
ShakaibKhan commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-895403091


   @jedcunningham 
   
   Was wondering what would be the best way to do this? Would another rebase be required to main or would I need to manually remove the tests I previously pulled in? Thanks!


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-842149077


   Closing/reopening as CI seems more stable.


-- 
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] ShakaibKhan commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
ShakaibKhan commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-900638820


   Looks like I re-commited all my commits. Although pre-commit and statickcheck passed locally


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk closed pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #15385:
URL: https://github.com/apache/airflow/pull/15385


   


-- 
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 a change in pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#discussion_r685937377



##########
File path: airflow/security/permissions.py
##########
@@ -41,6 +41,7 @@
 RESOURCE_PLUGIN = "Plugins"
 RESOURCE_PROVIDER = "Providers"
 RESOURCE_ROLE = "Roles"
+RESOURCE_PROVIDER = "Providers"

Review comment:
       There is already a `RESOURCE_PROVIDER` on Line 42




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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ShakaibKhan commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
ShakaibKhan commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-900638820


   Looks like I re-commited all my commits. Although pre-commit and statickcheck passed locally


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] jedcunningham commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-901574489


   @ShakaibKhan, looks like `tests/www/views/test_views_base.py::test_index` is failing due to query count. Your +1 to it probably got lost during your last rebase.


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-841025654


   And this time we have some conflicting files - can you please rebase again @ShakaibKhan ?


-- 
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 #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-835211797


   [The Workflow run](https://github.com/apache/airflow/actions/runs/822695772) 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 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 #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-821205579


   Is there a way that we could parse the descriptions to convert the URLs (that most of them contain) to actual links?


-- 
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] ShakaibKhan commented on a change in pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
ShakaibKhan commented on a change in pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#discussion_r692210588



##########
File path: tests/www/views/test_views.py
##########
@@ -93,6 +93,24 @@ def test_plugin_endpoint_should_not_be_unauthenticated(app):
     check_content_in_response("Sign In - Airflow", resp)
 
 
+# cd = re.sub("[`_]", "", description.strip(" \n.").strip("\""))
+# cd = re.sub("<", "<a href=\"", cd)
+# cd = re.sub(">", "\">[site]</a>", cd)

Review comment:
       accidentally left in 




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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] xinbinhuang commented on a change in pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#discussion_r614505521



##########
File path: airflow/www/views.py
##########
@@ -3017,6 +3018,51 @@ def list(self):
         )
 
 
+class ProvidersModelView(AirflowBaseView):

Review comment:
       Can you also add some tests to `test_vews.py` for this new 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] potiuk commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-842174162


   Agree. That can wait for post 2.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] jhtimmins commented on a change in pull request #15385: [WIP] Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#discussion_r625488542



##########
File path: airflow/security/permissions.py
##########
@@ -32,6 +32,7 @@
 RESOURCE_JOB = "Jobs"
 RESOURCE_POOL = "Pools"
 RESOURCE_PLUGIN = "Plugins"
+RESOURCE_PROVIDERS = "Providers"

Review comment:
       ```suggestion
   RESOURCE_PROVIDER = "Providers"
   ```
   Just for consistency (I know some of the naming conventions are annoying. It's a work in progress :)). 

##########
File path: airflow/www/templates/airflow/providers.html
##########
@@ -0,0 +1,49 @@
+{#
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied.  See the License for the
+specific language governing permissions and limitations
+under the License.
+#}
+
+{% extends base_template %}
+
+{% block title %}
+  {{ title }}
+{% endblock %}
+
+{% block content %}
+  <h2>{{ title }}</h2>
+  {% if providers|length == 0 %}
+    <p>No providers loaded. Learn more in the
+      <a href="{{ doc_url }}" target="_blank">provider packages documentation</a>.
+    </p>
+  {% endif %}
+
+  <table class="table table-striped table-bordered">
+    <tr>
+      <th>Package Name</th>
+      <th>Version</th>
+      <th>Description</th>
+    </tr>
+
+    {% for provider in providers %}
+      <tr>
+          <td>{{ provider["package_name"] }}</td>
+          <td>{{ provider["version"] }}</td>
+          <td>{{ provider["description"] | safe}}</td>

Review comment:
       ```suggestion
             <td>{{ provider["description"]|safe}}</td>
   ```
   Doesn't _really_ matter, but this makes it consistent with line 28.

##########
File path: tests/www/test_views.py
##########
@@ -368,6 +369,19 @@ def test_endpoint_should_not_be_unauthenticated(self):
         self.check_content_in_response("Sign In - Airflow", resp)
 
 
+class TestProvidersView(TestBase):
+    def test_should_list_providers_on_page_with_details(self):
+        resp = self.client.get('/providers')
+        self.check_content_in_response("Providers", resp)

Review comment:
       Can you validate that the necessary permissions are included?

##########
File path: airflow/www/views.py
##########
@@ -3017,6 +3018,57 @@ def list(self):
         )
 
 
+class ProvidersView(AirflowBaseView):
+    """View to show Airflow Providers"""
+
+    default_view = 'list'
+
+    class_permission_name = permissions.RESOURCE_PROVIDERS
+
+    method_permission_name = {
+        'list': 'read',
+    }
+
+    base_permissions = [
+        permissions.ACTION_CAN_READ,
+        permissions.ACTION_CAN_ACCESS_MENU,
+    ]
+
+    @expose('/providers')
+    @auth.has_access(
+        [
+            (permissions.ACTION_CAN_READ, permissions.RESOURCE_PLUGIN),
+        ]
+    )
+    def list(self):
+        """List providers."""
+        providers_manager = ProvidersManager()
+
+        providers = []
+        for pi in providers_manager.providers.values():
+            provider_data = {
+                "package_name": pi[1]["package-name"],

Review comment:
       It may help to assign `pi[1]` to a variable to indicate what it is.

##########
File path: tests/www/test_views.py
##########
@@ -53,6 +53,7 @@
 from airflow.operators.bash import BashOperator
 from airflow.operators.dummy import DummyOperator
 from airflow.plugins_manager import AirflowPlugin, EntryPointSource
+from airflow.providers_manager import ProvidersManager

Review comment:
       Is this method in use?

##########
File path: airflow/www/views.py
##########
@@ -3017,6 +3018,57 @@ def list(self):
         )
 
 
+class ProvidersView(AirflowBaseView):
+    """View to show Airflow Providers"""
+
+    default_view = 'list'
+
+    class_permission_name = permissions.RESOURCE_PROVIDERS
+
+    method_permission_name = {
+        'list': 'read',
+    }
+
+    base_permissions = [
+        permissions.ACTION_CAN_READ,
+        permissions.ACTION_CAN_ACCESS_MENU,
+    ]
+
+    @expose('/providers')
+    @auth.has_access(
+        [
+            (permissions.ACTION_CAN_READ, permissions.RESOURCE_PLUGIN),

Review comment:
       ```suggestion
               (permissions.ACTION_CAN_READ, permissions.RESOURCE_PROVIDER),
   ```

##########
File path: airflow/www/views.py
##########
@@ -3017,6 +3018,57 @@ def list(self):
         )
 
 
+class ProvidersView(AirflowBaseView):
+    """View to show Airflow Providers"""
+
+    default_view = 'list'
+
+    class_permission_name = permissions.RESOURCE_PROVIDERS
+
+    method_permission_name = {
+        'list': 'read',
+    }
+
+    base_permissions = [
+        permissions.ACTION_CAN_READ,
+        permissions.ACTION_CAN_ACCESS_MENU,
+    ]
+
+    @expose('/providers')
+    @auth.has_access(
+        [
+            (permissions.ACTION_CAN_READ, permissions.RESOURCE_PLUGIN),
+        ]
+    )
+    def list(self):
+        """List providers."""
+        providers_manager = ProvidersManager()
+
+        providers = []
+        for pi in providers_manager.providers.values():
+            provider_data = {
+                "package_name": pi[1]["package-name"],
+                "description": self.clean_description(pi[1]["description"]),
+                "version": pi[0],
+            }
+            providers.append(provider_data)
+
+        title = "Providers"
+        doc_url = get_docs_url("apache-airflow-providers/index.html")
+        return self.render_template(
+            'airflow/providers.html',
+            providers=providers,
+            title=title,
+            doc_url=doc_url,
+        )
+
+    def clean_description(self, description):

Review comment:
       ```suggestion
       def _clean_description(self, description):
   ```
   This can probably be an internal method.




-- 
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] jedcunningham commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-895446826


   @ShakaibKhan I'd move just the new tests you wrote (I'm assuming it was all new tests, not changes to existing ones) to a new file (or an existing one if it makes sense), then delete the monolithic `test_views.py` in a new commit. That's the easiest path forward I believe.


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] jedcunningham commented on pull request #15385: Added table to view providers in airflow ui under admin tab

Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #15385:
URL: https://github.com/apache/airflow/pull/15385#issuecomment-897927155


   @ShakaibKhan, looks there is both a static check failure and an import error in your test. Can you take a look at those? Thanks.


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org