pierrejeambrun commented on code in PR #55262:
URL: https://github.com/apache/airflow/pull/55262#discussion_r2322127730
##########
airflow-core/src/airflow/plugins_manager.py:
##########
@@ -635,6 +637,9 @@ def get_plugin_info(attrs_to_dump: Iterable[str] | None =
None) -> list[dict[str
plugins_info = []
if plugins:
for plugin in plugins:
+ if plugin.name in import_errors:
+ log.warning("Skipping plugin %s because it has an invalid
'url_prefix'", plugin.name)
+ continue
Review Comment:
I don't think we should to that. To comply with how things were done in AF2
we allow extra properties and custom plugins object to be loaded.
The `get_plugin_info` endpoint should return plugins information 'as if'.
Even if some plugins attributes can't be interpreted or won't do anything, they
are still returned.
I would be in favor of remothing changes to the `plugins_manager` file, so
the endpoint keeps sending us all the information about 'loaded' endpoints. And
the code above will take care of not executing/registering invalid objects as
an application.
##########
airflow-core/src/airflow/api_fastapi/app.py:
##########
@@ -178,13 +178,18 @@ def init_plugins(app: FastAPI) -> None:
for subapp_dict in cast("list", plugins_manager.fastapi_apps):
name = subapp_dict.get("name")
subapp = subapp_dict.get("app")
+ plugin_name = subapp_dict.get("plugin_name")
if subapp is None:
log.error("'app' key is missing for the fastapi app: %s", name)
continue
url_prefix = subapp_dict.get("url_prefix")
if url_prefix is None:
log.error("'url_prefix' key is missing for the fastapi app: %s",
name)
continue
+ if url_prefix == "":
+ log.error("'url_prefix' key is empty string for the fastapi app:
%s", name)
+ plugins_manager.import_errors[plugin_name] = "'url_prefix' key is
empty"
Review Comment:
As long as plugins are loaded in memory (Any objects are allowed) they are
considered 'imported') If we compare to other checks in that function (or at
registration time), we can see that they are not considered 'import errors').
I see why we would like to do this, but to keep things consistent I wouldn't
create an `import error` for this.
```suggestion
```
--
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]
