fix(plugins): show load-failure reasons on the page the banner links to
The admin banner counts every plugin load failure, but settings/plugins.html only rendered failure reasons inside the plugin card macro -- which iterates discovered plugins. A plugin that failed BECAUSE it has no directory was therefore counted in the banner and shown nowhere on the page the banner links to. That dead end is what the operator hit: "1 plugin failed to load" on the dashboard, nothing wrong on the details page, and no way to clean it up. Folds the reason into the "Configured but not installed" rows rather than adding a competing section, and drives that list from the UNION of stored plugin.* settings and recorded load failures instead of stored settings alone. A failure with no stored row -- a plugin enabled by a DEFAULTS entry, which is exactly what plugin.http was -- would otherwise still be counted and still be invisible. Rows carry `removable`: only a plugin with a real stored row can be cleaned up, so a default-declared one shows the reason and an explanation instead of a Remove button that would delete nothing while the default reasserts it. Rows with a reason read as an error (red, "Failed to load"); rows that merely have leftover config stay a warning. A discovered plugin that failed is excluded -- its own card already shows the reason. Replaces find_orphaned_plugin_names outright rather than adding a second overlapping helper; it had one caller. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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]:
|
||||
|
||||
@@ -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"],
|
||||
|
||||
@@ -95,31 +95,50 @@
|
||||
<div style="margin-bottom:1.5rem;max-width:720px;">
|
||||
<div class="section-title" style="margin-bottom:0.4rem;">Configured but not installed</div>
|
||||
<p style="color:var(--text-muted);font-size:0.82rem;margin:0 0 0.75rem;">
|
||||
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.
|
||||
</p>
|
||||
<div style="display:grid;gap:0.6rem;">
|
||||
{% for name in orphaned_plugins %}
|
||||
{% for orphan in orphaned_plugins %}
|
||||
<div class="card" style="padding:0.85rem 1rem;display:flex;align-items:center;gap:0.75rem;">
|
||||
<span style="width:8px;height:8px;border-radius:50%;background:var(--yellow);flex-shrink:0;"
|
||||
title="Configured but not installed"></span>
|
||||
<span style="width:8px;height:8px;border-radius:50%;flex-shrink:0;
|
||||
background:{{ 'var(--red)' if orphan.reason else 'var(--yellow)' }};"
|
||||
title="{{ orphan.reason or 'Configured but not installed' }}"></span>
|
||||
<div style="flex:1;min-width:0;">
|
||||
<div style="display:flex;align-items:baseline;gap:0.5rem;flex-wrap:wrap;">
|
||||
<span style="font-weight:600;font-size:0.9rem;color:var(--text);">{{ name }}</span>
|
||||
<span style="font-weight:600;font-size:0.9rem;color:var(--text);">{{ orphan.name }}</span>
|
||||
{% if orphan.reason %}
|
||||
<span style="font-size:0.72rem;padding:0.1em 0.45em;border-radius:3px;
|
||||
background:color-mix(in srgb,var(--red) 15%,var(--bg));color:var(--red);">Failed to load</span>
|
||||
{% else %}
|
||||
<span style="font-size:0.72rem;padding:0.1em 0.45em;border-radius:3px;
|
||||
background:var(--yellow-dim);color:var(--yellow);">Not installed</span>
|
||||
{% endif %}
|
||||
</div>
|
||||
{% if orphan.reason %}
|
||||
<div style="font-size:0.77rem;color:var(--red);margin-top:0.1rem;
|
||||
white-space:nowrap;overflow:hidden;text-overflow:ellipsis;"
|
||||
title="{{ orphan.reason }}">{{ orphan.reason }}</div>
|
||||
{% endif %}
|
||||
<div style="font-size:0.8rem;color:var(--text-muted);margin-top:0.1rem;">
|
||||
Settings key <code>plugin.{{ name }}</code> has no matching plugin.
|
||||
{% if orphan.removable %}
|
||||
Settings key <code>plugin.{{ orphan.name }}</code> 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 %}
|
||||
</div>
|
||||
</div>
|
||||
<form method="post" action="/settings/plugins/orphans/{{ name }}/remove/"
|
||||
onsubmit="return confirm('Remove stored settings for "{{ name }}"? Its saved configuration, including any credentials, will be discarded. This cannot be undone.');"
|
||||
{% if orphan.removable %}
|
||||
<form method="post" action="/settings/plugins/orphans/{{ orphan.name }}/remove/"
|
||||
onsubmit="return confirm('Remove stored settings for "{{ orphan.name }}"? Its saved configuration, including any credentials, will be discarded. This cannot be undone.');"
|
||||
style="margin:0;flex-shrink:0;">
|
||||
<button type="submit" class="btn btn-danger btn-sm" style="font-size:0.78rem;">Remove</button>
|
||||
</form>
|
||||
{% endif %}
|
||||
</div>
|
||||
{% endfor %}
|
||||
</div>
|
||||
|
||||
@@ -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"}
|
||||
|
||||
Reference in New Issue
Block a user