mjpieters commented on a change in pull request #10770:
URL: https://github.com/apache/airflow/pull/10770#discussion_r491431202
##########
File path: airflow/plugins_manager.py
##########
@@ -67,6 +67,8 @@ class AirflowPluginException(Exception):
class AirflowPlugin:
"""Class used to define AirflowPlugin."""
name: Optional[str] = None
+ plugin_source: Optional[str] = None
+ plugin_path: Optional[str] = None
Review comment:
Thought: Not sure this should be part of the plugin interface. This is
the class the plugin inherits from, and has so far been necessarily the data
structure(D) that Airflow owns to track discovered plugins.
Also, the name `plugin_path` is overloaded here; it is only a path for one
type of plugin, so I'd use `plugin_location` perhaps?
##########
File path: airflow/plugins_manager.py
##########
@@ -183,6 +187,8 @@ def load_plugins_from_plugin_directory():
for mod_attr_value in (m for m in mod.__dict__.values() if
is_valid_plugin(m)):
plugin_instance = mod_attr_value()
+ plugin_instance.plugin_source = "airflow_plugins_directory"
+ plugin_instance.plugin_path = os.path.abspath(file_path)
Review comment:
Please make this relative to the Airflow plugins folder path. That way
you don't expose more information about the filesystem layout than is strictly
necessary.
You can do this with
[`os.path.relpath()`](https://docs.python.org/3/library/os.path.html#os.path.relpath),
and perhaps then prefix the path with `$AIRFLOW_HOME/plugins/` when displaying?
##########
File path: airflow/plugins_manager.py
##########
@@ -150,6 +152,8 @@ def load_entrypoint_plugins():
plugin_instance = plugin_class()
if callable(getattr(plugin_instance, 'on_load', None)):
plugin_instance.on_load()
+ plugin_instance.plugin_source = "entrypoint_plugin"
+ plugin_instance.plugin_path = str(entry_point.dist)
Review comment:
Can we include `str(entry_point)` somehow? It was part of my original FR.
##########
File path: tests/www/test_views.py
##########
@@ -301,6 +302,27 @@ def test_import_variables_success(self):
self.check_content_in_response('4 variable(s) successfully updated.',
resp)
+class PluginOperator(BaseOperator):
+ pass
+
+
+class TestPluginView(TestBase):
+ def setUp(self):
+ super().setUp()
+
+ def tearDown(self):
+ super().tearDown()
+
+ def test_should_list_plugins_on_page_with_details(self):
+ resp = self.client.get('/plugin')
+ self.check_content_in_response("test_plugin", resp)
Review comment:
Yay, tests! 😀
Does this cover both plugin sources?
----------------------------------------------------------------
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:
[email protected]
