diff --git a/controller/api/schemas.py b/controller/api/schemas.py index 06c7c8cb..3d32a284 100644 --- a/controller/api/schemas.py +++ b/controller/api/schemas.py @@ -4,7 +4,7 @@ Pydantic models for Controller API requests and responses. from typing import Optional -from pydantic import BaseModel, Field +from pydantic import BaseModel, ConfigDict, Field # ============================================================================ @@ -495,6 +495,9 @@ class ScriptOutputResponse(BaseModel): class ScriptStartRequest(BaseModel): """Request model for starting a script.""" + # Parameters belong in `params`; any other key is a mistake, not something to ignore. + model_config = ConfigDict(extra="forbid") + params: Optional[dict[str, str]] = Field(default=None, description="Parameter values (e.g., {'branch': 'release'})") diff --git a/controller/lib/scripts.py b/controller/lib/scripts.py index 0a0be151..a530e280 100644 --- a/controller/lib/scripts.py +++ b/controller/lib/scripts.py @@ -239,17 +239,30 @@ def start_script(name: str, params: Optional[dict] = None) -> dict: script_config = AVAILABLE_SCRIPTS[name] service = script_config["service_name"] + parameters = script_config.get("parameters", []) - # Write parameters to env file if script has one configured + # A misspelled parameter would otherwise be dropped, and the script run with its default. + unknown = sorted(set(params or {}) - {i["name"] for i in parameters}) + if unknown: + return { + "success": False, + "error": f"Unknown parameters for script '{name}': {', '.join(unknown)}", + } + + # Always write the env file: the service cannot start without it, and a file left by an interrupted run must + # not choose the parameters. env_file = script_config.get("env_file") - if env_file and params: + if env_file: + values = {key: value for key, value in (params or {}).items() if value} + for parameter in parameters: + if parameter["type"] == "branch" and not values.get(parameter["name"]): + # upgrade.sh upgrades the current branch when it is not given one. + values[parameter["name"]] = get_current_branch() or "release" try: with open(env_file, "w") as f: - for key, value in params.items(): - if value: # Only write non-empty values - # Convert param name to uppercase for env var - env_name = key.upper() - f.write(f"{env_name}={value}\n") + for key, value in values.items(): + # Convert param name to uppercase for env var + f.write(f"{key.upper()}={value}\n") except OSError as e: return { "success": False, diff --git a/controller/test/test_scripts.py b/controller/test/test_scripts.py index 36bbcf39..a02ea452 100644 --- a/controller/test/test_scripts.py +++ b/controller/test/test_scripts.py @@ -240,6 +240,46 @@ class TestStartScript: assert "wrolpi-repair.service" in call_args + def _start_upgrade(self, tmp_path, params, current_branch="master"): + """Start the upgrade script with its env file in `tmp_path`. Returns (result, env file, Popen mock).""" + env_file = tmp_path / "wrolpi-upgrade.env" + with mock.patch.dict(AVAILABLE_SCRIPTS["upgrade"], {"env_file": str(env_file)}), \ + mock.patch("controller.lib.scripts.is_docker_mode", return_value=False), \ + mock.patch("controller.lib.scripts.get_script_status", return_value={"running": False}), \ + mock.patch("controller.lib.scripts.get_current_branch", return_value=current_branch), \ + mock.patch("subprocess.Popen") as popen: + result = start_script("upgrade", params=params) + return result, env_file, popen + + def test_upgrade_without_branch_writes_current_branch(self, tmp_path): + """The upgrade service reads BRANCH from its env file, and fails to start without the file; upgrade.sh + removes it when it finishes. With no branch, upgrade the current one, as upgrade.sh does.""" + for params in (None, {}, {"branch": ""}): + result, env_file, popen = self._start_upgrade(tmp_path, params) + assert result["success"] is True, params + assert env_file.read_text() == "BRANCH=master\n", params + popen.assert_called_once() + env_file.unlink() + + def test_upgrade_replaces_stale_env_file(self, tmp_path): + """A file left by an interrupted upgrade does not choose the branch.""" + (tmp_path / "wrolpi-upgrade.env").write_text("BRANCH=old\n") + result, env_file, _ = self._start_upgrade(tmp_path, None, current_branch="release") + assert result["success"] is True + assert env_file.read_text() == "BRANCH=release\n" + + result, env_file, _ = self._start_upgrade(tmp_path, {"branch": "master"}, current_branch="release") + assert env_file.read_text() == "BRANCH=master\n" + + def test_unknown_param_is_rejected(self, tmp_path): + """A misspelled parameter is an error, not silently ignored.""" + result, env_file, popen = self._start_upgrade(tmp_path, {"brnch": "master"}) + assert result["success"] is False + assert "brnch" in result["error"] + assert not env_file.exists() + popen.assert_not_called() + + class TestGetScriptOutput: """Tests for get_script_output function.""" diff --git a/controller/test/test_scripts_api.py b/controller/test/test_scripts_api.py index 41a07951..c03b9f09 100644 --- a/controller/test/test_scripts_api.py +++ b/controller/test/test_scripts_api.py @@ -101,6 +101,16 @@ class TestScriptsStartEndpoint: assert response.status_code == 400 assert "already running" in response.json()["detail"].lower() + def test_scripts_start_rejects_unknown_body_keys(self, test_client): + """Parameters go in `params`; a body like {"branch": ...} is an error, not an upgrade of some other + branch.""" + with mock.patch("controller.lib.scripts.is_docker_mode", return_value=False), \ + mock.patch("controller.lib.scripts.get_script_status", return_value={"running": False}), \ + mock.patch("subprocess.Popen") as popen: + response = test_client.post("/api/scripts/upgrade/start", json={"branch": "master"}) + assert response.status_code == 422 + popen.assert_not_called() + class TestScriptsOutputEndpoint: """Tests for /api/scripts/{name}/output endpoint."""