mirror of
https://github.com/dredx/prole.git
synced 2026-09-23 11:03:59 +00:00
fix(cfg): refuse to serialize non-string widget values into knoe.cfg
Add _validate_cfg_values() to knoe/ui/screens/cfg.py — called before _filter_cfg_values_for_persistence so MagicMock (or any non-str) widget values raise TypeError instead of being silently str()-coerced into conf/<mode>.cfg. Also re-raise TypeError/ValueError from _save_knoe_cfg so the error escapes the outer broad except-Exception handler. New test: tests/installer/test_cfg_save_refuses_mock_values.py - test_save_knoe_cfg_refuses_non_string_widget_values: MagicMock vars → TypeError - test_save_knoe_cfg_real_strings_produce_clean_cfg: real _Var stubs → clean cfg Fixes TODO-1 / tracked in docs/completed/todo-1-cfg-save-path-bug.md. conf/k3d.cfg and conf/k3s.cfg still contain stale MagicMock values from before this fix and must be regenerated before committing. Co-authored-by: Junie <junie@jetbrains.com>
This commit is contained in:
parent
b5f17ffa72
commit
ef20c8a598
@ -34,6 +34,22 @@ from knoe.core.env import (
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _validate_cfg_values(name: str, mapping: dict) -> None:
|
||||||
|
"""Raise TypeError if any value in *mapping* is not a plain string.
|
||||||
|
|
||||||
|
Prevents MagicMock (or other non-string) reprs from being silently
|
||||||
|
coerced and written into conf/<mode>.cfg by _save_knoe_cfg.
|
||||||
|
"""
|
||||||
|
for k, v in mapping.items():
|
||||||
|
if not isinstance(v, str):
|
||||||
|
raise TypeError(
|
||||||
|
f"cfg {name!r}: field {k!r} has non-string value "
|
||||||
|
f"{type(v).__name__} ({v!r}). Refusing to serialize. "
|
||||||
|
"This usually means a Tk widget var wasn't properly initialized "
|
||||||
|
"(headless env, partial mocking, etc.)."
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
class ConfigMixin:
|
class ConfigMixin:
|
||||||
"""knoe.cfg persistence, input snapshot collection and port-forward management."""
|
"""knoe.cfg persistence, input snapshot collection and port-forward management."""
|
||||||
|
|
||||||
@ -189,6 +205,8 @@ class ConfigMixin:
|
|||||||
if explicit_service_namespace:
|
if explicit_service_namespace:
|
||||||
explicit_global_keys.add("SERVICE_NAMESPACE")
|
explicit_global_keys.add("SERVICE_NAMESPACE")
|
||||||
|
|
||||||
|
_validate_cfg_values("globals", globals_to_save)
|
||||||
|
|
||||||
globals_to_save = _filter_cfg_values_for_persistence(
|
globals_to_save = _filter_cfg_values_for_persistence(
|
||||||
"Global",
|
"Global",
|
||||||
globals_to_save,
|
globals_to_save,
|
||||||
@ -278,6 +296,8 @@ class ConfigMixin:
|
|||||||
cfg_path.write_text(cfg_text)
|
cfg_path.write_text(cfg_text)
|
||||||
print(f"[DEBUG] knoe.cfg saved to {cfg_path}")
|
print(f"[DEBUG] knoe.cfg saved to {cfg_path}")
|
||||||
|
|
||||||
|
except (TypeError, ValueError):
|
||||||
|
raise
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
print(f"[ERROR] Failed to save knoe.cfg: {e}")
|
print(f"[ERROR] Failed to save knoe.cfg: {e}")
|
||||||
|
|
||||||
|
|||||||
177
tests/installer/test_cfg_save_refuses_mock_values.py
Normal file
177
tests/installer/test_cfg_save_refuses_mock_values.py
Normal file
@ -0,0 +1,177 @@
|
|||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from pathlib import Path
|
||||||
|
from unittest.mock import MagicMock
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
from knoe.ui.screens.cfg import ConfigMixin
|
||||||
|
|
||||||
|
|
||||||
|
class _DummyCfgAppWithMockVar(ConfigMixin):
|
||||||
|
"""Worst-case input: every widget var is a bare MagicMock.
|
||||||
|
|
||||||
|
This is what happens when an interactive `./install.py -c conf/k3d.cfg`
|
||||||
|
run launches the TUI in a context where Tk variables aren't real
|
||||||
|
StringVars (headless env, partial mock setup, etc.).
|
||||||
|
"""
|
||||||
|
|
||||||
|
def __init__(self, conf_dir: Path):
|
||||||
|
self._conf_dir = conf_dir
|
||||||
|
self._cfg_path_override = conf_dir / "knoe.cfg"
|
||||||
|
self._mode = "k3d"
|
||||||
|
self._resolved_service_namespace = "knoe-system"
|
||||||
|
# Every var is a MagicMock — `var.get()` returns MagicMock,
|
||||||
|
# `.strip()` returns MagicMock, `str(...)` returns a repr.
|
||||||
|
for name in (
|
||||||
|
"cluster_env",
|
||||||
|
"selected_kubectx",
|
||||||
|
"db_username",
|
||||||
|
"db_password",
|
||||||
|
"db_namespace",
|
||||||
|
"cnpg_cluster_name",
|
||||||
|
"db_host_port",
|
||||||
|
"supabase_pv_node",
|
||||||
|
"supabase_pv_base_dir",
|
||||||
|
"gitops_node_selector",
|
||||||
|
"argocd_node_selector",
|
||||||
|
"k3s_server_url",
|
||||||
|
"k3s_token",
|
||||||
|
"prod_artifacts_path",
|
||||||
|
):
|
||||||
|
setattr(self, name, MagicMock())
|
||||||
|
|
||||||
|
self.knoe_cfg_data = {
|
||||||
|
"Global": {},
|
||||||
|
"Initialize Cluster": {},
|
||||||
|
"Dev Cluster (k3d)": {},
|
||||||
|
"Service Cluster (k3s)": {},
|
||||||
|
"Prod Cluster (k8s)": {},
|
||||||
|
}
|
||||||
|
|
||||||
|
def _deployment_mode(self) -> str:
|
||||||
|
return self._mode
|
||||||
|
|
||||||
|
def _get_service_namespace(self) -> str:
|
||||||
|
return self._resolved_service_namespace
|
||||||
|
|
||||||
|
def _secret_cfg_value(self, *_args, **_kwargs):
|
||||||
|
return ""
|
||||||
|
|
||||||
|
def _save_ansible_knoe_vault(self, *_args, **_kwargs):
|
||||||
|
return None
|
||||||
|
|
||||||
|
def _sanitize_sections_for_cfg(self, sections: dict) -> dict:
|
||||||
|
return sections
|
||||||
|
|
||||||
|
def _sync_port_forward_mappings(self):
|
||||||
|
return None
|
||||||
|
|
||||||
|
def _collect_input_snapshot(self) -> dict:
|
||||||
|
return {}
|
||||||
|
|
||||||
|
|
||||||
|
def test_save_knoe_cfg_refuses_non_string_widget_values(tmp_path, monkeypatch):
|
||||||
|
"""The save path must not serialize MagicMock (or any non-str) values
|
||||||
|
into the cfg file. Either raise a clear TypeError, or skip the
|
||||||
|
offending field. Silent str()-coercion-to-repr is forbidden.
|
||||||
|
"""
|
||||||
|
import knoe.ui.screens.cfg as cfg_mod
|
||||||
|
|
||||||
|
monkeypatch.setattr(cfg_mod.knoe_conf, "activate_environment", lambda *_a, **_k: None)
|
||||||
|
|
||||||
|
app = _DummyCfgAppWithMockVar(tmp_path)
|
||||||
|
cfg_path = tmp_path / "knoe.cfg"
|
||||||
|
|
||||||
|
with pytest.raises((TypeError, ValueError)) as excinfo:
|
||||||
|
app._save_knoe_cfg()
|
||||||
|
|
||||||
|
# The error must name the offending field/var so the engineer can
|
||||||
|
# diagnose without grepping mock-reprs out of files.
|
||||||
|
assert "MagicMock" in str(excinfo.value) or "non-string" in str(excinfo.value).lower()
|
||||||
|
|
||||||
|
# The cfg file must NOT contain any MagicMock string-reprs even if
|
||||||
|
# save partially succeeded before raising.
|
||||||
|
if cfg_path.exists():
|
||||||
|
text = cfg_path.read_text()
|
||||||
|
assert "<MagicMock" not in text, f"cfg leaked MagicMock repr:\n{text}"
|
||||||
|
|
||||||
|
|
||||||
|
class _Var:
|
||||||
|
def __init__(self, value: str):
|
||||||
|
self._value = value
|
||||||
|
|
||||||
|
def get(self) -> str:
|
||||||
|
return self._value
|
||||||
|
|
||||||
|
|
||||||
|
class _DummyCfgApp(ConfigMixin):
|
||||||
|
"""Real-string variant — save must succeed and produce a clean cfg."""
|
||||||
|
|
||||||
|
def __init__(self, conf_dir: Path):
|
||||||
|
self._conf_dir = conf_dir
|
||||||
|
self._cfg_path_override = conf_dir / "knoe.cfg"
|
||||||
|
self._mode = "k3d"
|
||||||
|
self._resolved_service_namespace = "knoe-system"
|
||||||
|
|
||||||
|
self.cluster_env = _Var("dev")
|
||||||
|
self.selected_kubectx = _Var("k3d-knoe-system")
|
||||||
|
self.db_username = _Var("knoe-db")
|
||||||
|
self.db_password = _Var("secret")
|
||||||
|
self.db_namespace = _Var("knoe-system")
|
||||||
|
self.cnpg_cluster_name = _Var("knoe-db")
|
||||||
|
self.db_host_port = _Var("5432")
|
||||||
|
self.supabase_pv_node = _Var("")
|
||||||
|
self.supabase_pv_base_dir = _Var("")
|
||||||
|
self.gitops_node_selector = _Var("")
|
||||||
|
self.argocd_node_selector = _Var("")
|
||||||
|
self.k3s_server_url = _Var("")
|
||||||
|
self.k3s_token = _Var("")
|
||||||
|
self.prod_artifacts_path = _Var(str(conf_dir))
|
||||||
|
self.service_namespace = _Var("knoe-system")
|
||||||
|
|
||||||
|
self.knoe_cfg_data = {
|
||||||
|
"Global": {},
|
||||||
|
"Initialize Cluster": {},
|
||||||
|
"Dev Cluster (k3d)": {},
|
||||||
|
"Service Cluster (k3s)": {},
|
||||||
|
"Prod Cluster (k8s)": {},
|
||||||
|
}
|
||||||
|
|
||||||
|
def _deployment_mode(self) -> str:
|
||||||
|
return self._mode
|
||||||
|
|
||||||
|
def _get_service_namespace(self) -> str:
|
||||||
|
return self._resolved_service_namespace
|
||||||
|
|
||||||
|
def _secret_cfg_value(self, _section, _key, value, *_args) -> str:
|
||||||
|
return value
|
||||||
|
|
||||||
|
def _save_ansible_knoe_vault(self, *_args, **_kwargs):
|
||||||
|
return None
|
||||||
|
|
||||||
|
def _sanitize_sections_for_cfg(self, sections: dict) -> dict:
|
||||||
|
return sections
|
||||||
|
|
||||||
|
def _sync_port_forward_mappings(self):
|
||||||
|
return None
|
||||||
|
|
||||||
|
def _collect_input_snapshot(self) -> dict:
|
||||||
|
return {}
|
||||||
|
|
||||||
|
|
||||||
|
def test_save_knoe_cfg_real_strings_produce_clean_cfg(tmp_path, monkeypatch):
|
||||||
|
"""When all widget vars return real strings, save must succeed without
|
||||||
|
any MagicMock reprs in the output file."""
|
||||||
|
import knoe.ui.screens.cfg as cfg_mod
|
||||||
|
|
||||||
|
monkeypatch.setattr(cfg_mod.knoe_conf, "activate_environment", lambda *_a, **_k: None)
|
||||||
|
|
||||||
|
app = _DummyCfgApp(tmp_path)
|
||||||
|
app._save_knoe_cfg()
|
||||||
|
|
||||||
|
cfg_path = tmp_path / "knoe.cfg"
|
||||||
|
assert cfg_path.exists(), "cfg file was not written"
|
||||||
|
text = cfg_path.read_text()
|
||||||
|
assert "<MagicMock" not in text
|
||||||
|
assert "KUBECONTEXT" in text
|
||||||
Loading…
Reference in New Issue
Block a user