jscheffl commented on code in PR #51889:
URL: https://github.com/apache/airflow/pull/51889#discussion_r2160034781
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/plugins.py:
##########
@@ -65,20 +65,22 @@ class AppBuilderMenuItemResponse(BaseModel):
model_config = ConfigDict(extra="allow")
name: str
- href: str | None = None
+ href: str
category: str | None = None
-class IFrameViewsResponse(BaseModel):
+class ExternalViewResponse(BaseModel):
"""Serializer for IFrame Plugin responses."""
model_config = ConfigDict(extra="allow")
name: str
- src: str
+ href: str
Review Comment:
`href` is required but only used if `url_route` is not used. Should we
somehow make this clear that at least a placeholder value needs to be added?
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/plugins.py:
##########
@@ -65,20 +65,22 @@ class AppBuilderMenuItemResponse(BaseModel):
model_config = ConfigDict(extra="allow")
name: str
- href: str | None = None
+ href: str
category: str | None = None
-class IFrameViewsResponse(BaseModel):
+class ExternalViewResponse(BaseModel):
"""Serializer for IFrame Plugin responses."""
model_config = ConfigDict(extra="allow")
name: str
- src: str
+ href: str
icon: str | None = None
+ dark_mode_icon: str | None = None
Review Comment:
Reading multiple times I feel a "dark mode" should rather be a suffic for
the property, would bring the benefit that a sorting lists light and dark mode
together
```suggestion
icon_dark_mode: str | None = None
```
##########
airflow-core/docs/administration-and-deployment/plugins.rst:
##########
@@ -195,18 +195,24 @@ definitions in Airflow.
}
# Creating a iframe view that will be rendered in the Airflow UI.
- iframe_view_with_metadata = {
+ external_view_with_metadata = {
"name": "Name of the Iframe View as displayed in the UI",
- # Source URL of the iframe. This URL can be templated using context
variables, depending on the location where the iframe is rendered
- # the context variables available will be different, i.e a subset of
(DAG_ID, RUN_ID, TASK_ID, MAP_INDEX)
- "src": "https://example.com/{DAG_ID}/{RUN_ID}/{TASK_ID}";,
+ # Source URL of the external view. This URL can be templated using
context variables, depending on the location where the external view is rendered
+ # the context variables available will be different, i.e a subset of
(DAG_ID, RUN_ID, TASK_ID, MAP_INDEX).
+ "href": "https://example.com/{DAG_ID}/{RUN_ID}/{TASK_ID}";,
# Destination of the iframe view. This is used to determine where the
iframe will be loaded in the UI.
- # Supported locations are Literal["nav", "dag", "dag_run", "task",
"task_instance"]
+ # Supported locations are Literal["nav", "dag", "dag_run", "task",
"task_instance"], default to "nav".
"destination": "dag_run",
# Optional icon, url to an svg file.
"icon": "https://example.com/icon.svg";,
- # Optional parameters, relative URL location when opening the iframe
- "url_route": "/my_iframe_view",
+ # Optional dark icon for the dark theme, url to an svg file. If not
provided, "icon" will be used for both light and dark themes.
+ "dark_mode_icon": "https://example.com/dark_icon.svg";,
+ # Optional parameters, relative URL location for the iframe rendering.
If not provided, external view will be rendeded as an external link. Should
+ # not contain a leading slash.
+ "url_route": "my_iframe_view",
Review Comment:
It is not explicitly stated but I assume the same templating like with
`href` is applied to `url_route` correct?
##########
airflow-core/tests/unit/plugins/test_plugin.py:
##########
@@ -102,12 +102,14 @@ async def dispatch(self, request, call_next):
"name": "Name of the Middleware",
}
-iframe_view_with_metadata = {
+external_view_with_metadata = {
"name": "Test IFrame Plugin",
- "src": "https://www.google.com";,
+ "href": "https://www.google.com";,
"icon": "https://example.com/icon.svg";,
- "url_route": "/test_iframe_plugin",
+ "dark_mode_icon": "https://example.com/dark_icon.svg";,
+ "url_route": "test_iframe_plugin",
"destination": "nav",
+ "category": "browse",
}
Review Comment:
I realized that Google blocks (probably because of security?) to embed the
homepage into an iFrame. Therefore I tested (positive) with these parameters:
```
external_view_with_metadata = {
"name": "Test IFrame Airflow Docs",
"href": "https://airflow.apache.org/";,
"icon":
"https://raw.githubusercontent.com/lucide-icons/lucide/refs/heads/main/icons/plug.svg";,
"url_route": "test_iframe_plugin",
"destination": "nav",
"category": "browse",
}
```
Did not find a comparable dark mode icon but maybe it would be best to
move-away from google as example.
--
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]
![https://example.com/icon.svg"](https://example.com/icon.svg"){kind=link}
![https://example.com/dark_icon.svg"](https://example.com/dark_icon.svg"){kind=link}
![https://raw.githubusercontent.com/lucide-icons/lucide/refs/heads/main/icons/plug.svg"](https://raw.githubusercontent.com/lucide-icons/lucide/refs/heads/main/icons/plug.svg"){kind=link}