diff --git a/steward/core/settings.py b/steward/core/settings.py index ad6a80e..3469b56 100644 --- a/steward/core/settings.py +++ b/steward/core/settings.py @@ -18,7 +18,7 @@ from __future__ import annotations import asyncio import json import logging -from collections.abc import Iterable +from collections.abc import Iterable, Mapping from datetime import datetime, timezone from typing import Any @@ -267,20 +267,42 @@ async def get_stored_plugin_names(session: AsyncSession) -> list[str]: ) -def find_orphaned_plugin_names( +def find_orphaned_plugins( stored_names: Iterable[str], installed_names: Iterable[str], -) -> list[str]: - """Stored plugin settings whose plugin is not currently installed. + failures: Mapping[str, str] | None = None, +) -> list[dict[str, Any]]: + """Plugins that are configured or failed to load, but are not installed. Pure so it can be tested without a DB or a filesystem. "Not installed" is intentionally not treated as "safe to delete" — an external plugin under /data/plugins can be missing merely because the volume isn't mounted or an install failed, so the caller surfaces these for an explicit operator decision instead of removing them automatically. + + Driven by the UNION of stored settings and load failures, not by stored + settings alone. `load_plugins` records a failure for any enabled plugin + whose directory it cannot find, and the admin banner counts those — so a + failure with no stored row (a plugin enabled by a DEFAULTS entry) would + otherwise be counted in the banner and shown nowhere, which is exactly the + dead end this section exists to close. + + Each row carries `removable`: only a plugin with a real stored row can + actually be cleaned up. Offering Remove for a default-declared plugin would + be a lie — deleting nothing, while the default reasserts it on next load. """ installed = set(installed_names) - return sorted(name for name in stored_names if name and name not in installed) + failures = dict(failures or {}) + stored = {name for name in stored_names if name} + candidates = (stored | set(failures)) - installed + return [ + { + "name": name, + "reason": failures.get(name), + "removable": name in stored, + } + for name in sorted(candidates) + ] async def get_all_settings(session: AsyncSession) -> dict[str, Any]: diff --git a/steward/settings/routes.py b/steward/settings/routes.py index 23c3e72..03b1c24 100644 --- a/steward/settings/routes.py +++ b/steward/settings/routes.py @@ -9,7 +9,7 @@ from steward.core.audit import log_audit from steward.models.users import UserRole from steward.core.settings import ( get_all_settings, set_setting, delete_setting, - get_stored_plugin_names, find_orphaned_plugin_names, + get_stored_plugin_names, find_orphaned_plugins, to_smtp_cfg, to_webhook_cfg, to_ansible_cfg, to_plugins_cfg, to_oidc_cfg, to_ldap_cfg, to_thresholds_cfg, ) @@ -607,8 +607,17 @@ async def plugins(): # explicit operator decision rather than cleaned up automatically: an external # plugin can be missing because /data/plugins isn't mounted or an install # failed, and silently dropping its row would destroy stored credentials. - orphans = find_orphaned_plugin_names( - stored_plugin_names, [p["_dir"] for p in discovered]) + # + # Load failures are folded in so every plugin counted by the admin banner has + # a row here. A plugin that failed BECAUSE it has no directory is not in + # `discovered`, so without this it would be counted in the banner and shown + # nowhere on the page that banner links to. + from steward.core.plugin_manager import get_plugin_failures + orphans = find_orphaned_plugins( + stored_plugin_names, + [p["_dir"] for p in discovered], + get_plugin_failures(), + ) return await render_template( "settings/plugins.html", capabilities=[p for p in discovered if p["_kind"] == "capability"], diff --git a/steward/templates/settings/plugins.html b/steward/templates/settings/plugins.html index 15939ea..c0bc414 100644 --- a/steward/templates/settings/plugins.html +++ b/steward/templates/settings/plugins.html @@ -95,31 +95,50 @@
Configured but not installed

- Stored settings for plugins Steward can't find. This usually means the plugin was - removed — but it also happens when an external plugin directory isn't mounted or an + Plugins Steward has settings for, or tried to load, but cannot find on disk — including + anything counted by the "failed to load" banner. This usually means the plugin was + removed, but it also happens when an external plugin directory isn't mounted or an install failed part-way, so nothing is deleted automatically. Removing an entry discards that plugin's saved configuration, including any credentials.

- {% for name in orphaned_plugins %} + {% for orphan in orphaned_plugins %}
- +
- {{ name }} + {{ orphan.name }} + {% if orphan.reason %} + Failed to load + {% else %} Not installed + {% endif %}
+ {% if orphan.reason %} +
{{ orphan.reason }}
+ {% endif %}
- Settings key plugin.{{ name }} has no matching plugin. + {% if orphan.removable %} + Settings key plugin.{{ orphan.name }} has no matching plugin. + {% else %} + Enabled by a built-in default with no stored settings to remove — this is a + packaging bug, not leftover configuration. Please report it. + {% endif %}
-
+ {% endif %}
{% endfor %}
diff --git a/tests/core/test_plugin_settings_hygiene.py b/tests/core/test_plugin_settings_hygiene.py index b5b5c64..5a55609 100644 --- a/tests/core/test_plugin_settings_hygiene.py +++ b/tests/core/test_plugin_settings_hygiene.py @@ -8,7 +8,7 @@ CI failure instead of a support question. """ from pathlib import Path -from steward.core.settings import DEFAULTS, find_orphaned_plugin_names +from steward.core.settings import DEFAULTS, find_orphaned_plugins from steward.settings.routes import CAPABILITY_PLUGINS BUNDLED_PLUGINS_DIR = Path(__file__).resolve().parents[2] / "plugins" @@ -63,26 +63,73 @@ def test_capability_plugins_all_exist(): def test_orphans_are_stored_names_with_no_installed_plugin(): - assert find_orphaned_plugin_names( - ["docker", "traefik", "ancient"], ["docker", "traefik"] - ) == ["ancient"] + rows = find_orphaned_plugins(["docker", "traefik", "ancient"], ["docker", "traefik"]) + assert [r["name"] for r in rows] == ["ancient"] + assert rows[0]["removable"] is True + assert rows[0]["reason"] is None def test_no_orphans_when_everything_is_installed(): - assert find_orphaned_plugin_names(["docker", "snmp"], ["docker", "snmp"]) == [] + assert find_orphaned_plugins(["docker", "snmp"], ["docker", "snmp"]) == [] def test_installed_plugin_without_stored_settings_is_not_an_orphan(): # Never configured is not the same as left behind. - assert find_orphaned_plugin_names([], ["docker"]) == [] + assert find_orphaned_plugins([], ["docker"]) == [] -def test_orphans_are_sorted_and_deduped_of_empties(): - assert find_orphaned_plugin_names(["zeta", "", "alpha"], []) == ["alpha", "zeta"] +def test_orphans_are_sorted_and_skip_empty_names(): + rows = find_orphaned_plugins(["zeta", "", "alpha"], []) + assert [r["name"] for r in rows] == ["alpha", "zeta"] def test_accepts_arbitrary_iterables(): # Callers pass a generator of discovered dirs, not a list. - assert find_orphaned_plugin_names( - (n for n in ["gone"]), (n for n in ["here"]) - ) == ["gone"] + rows = find_orphaned_plugins((n for n in ["gone"]), (n for n in ["here"])) + assert [r["name"] for r in rows] == ["gone"] + + +# ── load failures folded in (issue #2638) ──────────────────────────────────── + + +def test_failure_reason_is_attached_to_its_row(): + rows = find_orphaned_plugins( + ["ancient"], [], {"ancient": "Plugin directory not found in: ['/app/plugins']"}) + assert rows[0]["reason"] == "Plugin directory not found in: ['/app/plugins']" + + +def test_failure_with_no_stored_row_still_gets_a_row(): + """The banner counts it, so the page must show it — that was the dead end. + + A plugin enabled by a DEFAULTS entry has no stored row, so a stored-only + view would leave the banner pointing at a page with nothing on it. + """ + rows = find_orphaned_plugins([], [], {"http": "Plugin directory not found"}) + assert [r["name"] for r in rows] == ["http"] + assert rows[0]["reason"] == "Plugin directory not found" + + +def test_failure_with_no_stored_row_is_not_removable(): + # Offering Remove would delete nothing while the default reasserts it. + rows = find_orphaned_plugins([], [], {"http": "Plugin directory not found"}) + assert rows[0]["removable"] is False + + +def test_failure_of_an_installed_plugin_is_not_listed_here(): + # A discovered plugin that failed already shows the reason on its own card. + assert find_orphaned_plugins( + ["docker"], ["docker"], {"docker": "boom"}) == [] + + +def test_stored_orphan_and_failure_are_not_duplicated(): + rows = find_orphaned_plugins(["gone"], [], {"gone": "Plugin directory not found"}) + assert len(rows) == 1 + assert rows[0]["removable"] is True and rows[0]["reason"] + + +def test_every_undiscovered_failure_is_represented(): + # The invariant that keeps the banner count and this list in agreement. + failures = {"a": "x", "b": "y", "docker": "z"} + rows = find_orphaned_plugins([], ["docker"], failures) + listed = {r["name"] for r in rows} + assert listed == {"a", "b"}