pierrejeambrun commented on code in PR #55212:
URL: https://github.com/apache/airflow/pull/55212#discussion_r2445275332
##########
airflow-core/src/airflow/plugins_manager.py:
##########
@@ -298,10 +298,33 @@ def load_plugins_from_plugin_directory():
sys.modules[spec.name] = mod
loader.exec_module(mod)
- for mod_attr_value in (m for m in mod.__dict__.values() if
is_valid_plugin(m)):
- plugin_instance = mod_attr_value()
- plugin_instance.source = PluginsDirectorySource(file_path)
- register_plugin(plugin_instance)
+ # First pass: collect all plugin classes in this file
+ plugin_classes = [m for m in mod.__dict__.values() if
is_valid_plugin(m)]
+
+ # Check for duplicate plugin names within the same file
+ plugin_names = {}
+ for plugin_class in plugin_classes:
+ plugin_instance = plugin_class()
+ plugin_name = plugin_instance.name
+ if not plugin_name:
+ raise AirflowPluginException(
Review Comment:
We probably want to register an `import_errors`. This will also most likely
break registration of other plugins. `for plugin_class in plugin_classes` will
stop there without processing further plugin_class.
I see most function using `register_plugin` are catching exceptions and
doing that import error registration (so we can see it nicely in the UI). So
basically raising directly in `register_plugin` should do the trick.
##########
airflow-core/src/airflow/plugins_manager.py:
##########
@@ -298,10 +298,33 @@ def load_plugins_from_plugin_directory():
sys.modules[spec.name] = mod
loader.exec_module(mod)
- for mod_attr_value in (m for m in mod.__dict__.values() if
is_valid_plugin(m)):
- plugin_instance = mod_attr_value()
- plugin_instance.source = PluginsDirectorySource(file_path)
- register_plugin(plugin_instance)
+ # First pass: collect all plugin classes in this file
+ plugin_classes = [m for m in mod.__dict__.values() if
is_valid_plugin(m)]
+
+ # Check for duplicate plugin names within the same file
+ plugin_names = {}
+ for plugin_class in plugin_classes:
+ plugin_instance = plugin_class()
+ plugin_name = plugin_instance.name
+ if not plugin_name:
+ raise AirflowPluginException(
+ f"Plugin in {file_path} has no name. "
+ f"Please set the 'name' attribute for the plugin
class."
+ )
+ if plugin_name in plugin_names:
+ raise AirflowPluginException(f"Duplicate plugin name
found in {file_path}. ")
+ plugin_names[plugin_name] = plugin_class
Review Comment:
I don't think we need two loops.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
