pierrejeambrun commented on code in PR #51714:
URL: https://github.com/apache/airflow/pull/51714#discussion_r2152280196
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/config.py:
##########
@@ -35,3 +36,4 @@ class ConfigResponse(BaseModel):
dashboard_alert: list[UIAlert]
show_external_log_redirect: bool
external_log_name: str | None = None
+ plugins_extra_menu_items: list[AppBuilderMenuItemResponse] | None = None
Review Comment:
This can never be None.
Once the plugin manager is initialized all its `None` attributes will be
replaced with an empty list.
##########
airflow-core/src/airflow/ui/src/pages/Dashboard/Stats/PluginImportErrors.tsx:
##########
@@ -30,7 +30,9 @@ import { PluginImportErrorsModal } from
"./PluginImportErrorsModal";
export const PluginImportErrors = ({ iconOnly = false }: { readonly iconOnly?:
boolean }) => {
const { onClose, onOpen, open } = useDisclosure();
const { t: translate } = useTranslation("admin");
- const { data, error, isLoading } = usePluginServiceImportErrors();
+ const { data, error, isLoading } = usePluginServiceImportErrors(undefined, {
+ retry: false,
+ });
Review Comment:
Why is that necesssary?
##########
airflow-core/src/airflow/ui/src/layouts/Nav/PluginMenus.tsx:
##########
@@ -21,36 +21,39 @@ import { useTranslation } from "react-i18next";
import { FiChevronRight } from "react-icons/fi";
import { LuPlug } from "react-icons/lu";
-import { usePluginServiceGetPlugins } from "openapi/queries";
import type { AppBuilderMenuItemResponse } from "openapi/requests/types.gen";
import { Menu } from "src/components/ui";
+import { useConfig } from "src/queries/useConfig";
import { NavButton } from "./NavButton";
export const PluginMenus = () => {
const { t: translate } = useTranslation("common");
- const { data } = usePluginServiceGetPlugins();
+ const menuItems = useConfig("plugins_extra_menu_items");
- const menuPlugins = data?.plugins.filter((plugin) =>
plugin.appbuilder_menu_items.length > 0);
-
- if (data === undefined || menuPlugins === undefined) {
+ if (
+ !Array.isArray(menuItems) ||
+ !menuItems.every(
+ (item): item is AppBuilderMenuItemResponse => "name" in item && "href"
in item && "category" in item,
+ )
Review Comment:
You'll be able to simplify this check once the backend always returns a list
for `menuItems`
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_config.py:
##########
@@ -19,8 +19,10 @@
import pytest
from tests_common.test_utils.config import conf_vars
+from tests_common.test_utils.mock_plugins import mock_plugin_manager
-pytestmark = pytest.mark.db_test
+# pytestmark = pytest.mark.db_test
Review Comment:
```suggestion
```
##########
airflow-core/src/airflow/ui/src/pages/Dashboard/Stats/Stats.tsx:
##########
@@ -59,7 +60,7 @@ export const Stats = () => {
<DAGImportErrors />
- <PluginImportErrors />
+ {authLinks?.authorized_menu_items.includes("Plugins") ?
<PluginImportErrors /> : null}
Review Comment:
Nice
##########
devel-common/src/tests_common/pytest_plugin.py:
##########
@@ -410,7 +410,9 @@ def initialize_airflow_tests(request):
def _initialize_airflow_db(force_db_init: bool, airflow_home: str | Path):
- db_init_lock_file = Path(airflow_home).joinpath(".airflow_db_initialised")
+ airflow_home = Path(airflow_home)
+ airflow_home.mkdir(parents=True, exist_ok=True)
+ db_init_lock_file = airflow_home.joinpath(".airflow_db_initialised")
Review Comment:
I'm surprised that we need that.
Did you experience any trouble running tests?
--
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]
