GRM-19: refactor: resolve_runner returns gitea_url, add --url CLI option, force remove improvements, code quality fixes

- _resolve_runner now returns gitea_url from registry so disable/remove can reuse the URL stored at install time without requiring env vars. - Added --url option to install, disable, and remove CLI commands. - remove(force=True) no longer requires gitea_url or token. - Moved _parse_status outside the for loop in list_runners. - Updated all translations and tests to match.
2026-06-19 03:12:04 +02:00
parent a9adb71a08
commit 3d943b57dc
5 changed files with 214 additions and 37 deletions
@@ -76,6 +76,11 @@ def cli() -> None:
    default="docker",
    help=_("Gitea Runner deployment mode (default: docker)"),
 )
+@click.option(
+    "--url",
+    default=lambda: os.getenv("GITEA_URL", ""),
+    help=_("Gitea URL (env: GITEA_URL)"),
+)
@click.option(
    "--admin-token",
    "-a",
@@ -101,6 +106,7 @@ def install(
    name: str | None,
    token: str | None,
    mode: str,
+    url: str,
    admin_token: str | None,
    integration_retries: int,
    ask_become_pass: bool,
@@ -113,7 +119,7 @@ def install(
            key=key,
            name=name,
            token=token,
-            gitea_url=os.getenv("GITEA_URL", ""),
+            gitea_url=url,
            mode=mode,
            admin_token=admin_token,
            integration_retries=integration_retries,
@@ -251,6 +257,11 @@ def enable(
    type=click.Choice(["docker", "binary"]),
    help=_("Override deployment mode from registry"),
 )
+@click.option(
+    "--url",
+    default=lambda: os.getenv("GITEA_URL", ""),
+    help=_("Gitea URL (env: GITEA_URL)"),
+)
@_handle_errors("Disable failed: {error}")
 def disable(
    runner_name: str,
@@ -259,6 +270,7 @@ def disable(
    key: str | None,
    token: str | None,
    mode: str | None,
+    url: str,
    ask_become_pass: bool,
 ) -> None:
    manager = RunnerManager()
@@ -268,7 +280,7 @@ def disable(
        user=user,
        key=key,
        token=token,
-        gitea_url=os.getenv("GITEA_URL", ""),
+        gitea_url=url,
        mode=mode,
        ask_become_pass=ask_become_pass,
    )
@@ -318,6 +330,11 @@ def status(
    type=click.Choice(["docker", "binary"]),
    help=_("Override deployment mode from registry"),
 )
+@click.option(
+    "--url",
+    default=lambda: os.getenv("GITEA_URL", ""),
+    help=_("Gitea URL (env: GITEA_URL)"),
+)
@click.option(
    "--force",
    "-f",
@@ -332,6 +349,7 @@ def remove(
    key: str | None,
    token: str | None,
    mode: str | None,
+    url: str,
    force: bool,
    ask_become_pass: bool,
 ) -> None:
@@ -342,7 +360,7 @@ def remove(
        user=user,
        key=key,
        token=token,
-        gitea_url=os.getenv("GITEA_URL", ""),
+        gitea_url=url,
        mode=mode,
        ask_become_pass=ask_become_pass,
        force=force,
@@ -114,6 +114,13 @@ TRANSLATIONS: dict[str, dict[str, str]] = {
        "ru": "Токен регистрации (env: GITEA_REGISTRATION_TOKEN)",
        "zh": "注册令牌（环境变量: GITEA_REGISTRATION_TOKEN）",
    },
+    "Gitea URL (env: GITEA_URL)": {
+        "en": "Gitea URL (env: GITEA_URL)",
+        "bg": "Gitea URL (env: GITEA_URL)",
+        "de": "Gitea-URL (env: GITEA_URL)",
+        "ru": "URL Gitea (env: GITEA_URL)",
+        "zh": "Gitea URL（环境变量: GITEA_URL）",
+    },
    "Gitea Runner deployment mode (default: docker)": {
        "en": "Gitea Runner deployment mode (default: docker)",
        "bg": "Режим на разполагане на Gitea Runner (по подразбиране: docker)",
@@ -96,9 +96,15 @@ class RunnerManager:
        user: str | None = None,
        key: str | None = None,
        mode: str | None = None,
-    ) -> tuple[str, str, str | None, str]:
-        """Look up runner metadata from registry, applying CLI overrides."""
+    ) -> tuple[str, str, str | None, str, str]:
+        """Look up runner metadata from registry, applying CLI overrides.
+
+        Returns ``(host, user, key, mode, gitea_url)`` where *gitea_url* is
+        taken from the registry when available, allowing ``disable`` and
+        ``remove`` to reuse the value stored at install time.
+        """
        info = self._registry.get(name)
+        actual_gitea_url = info.get("gitea_url", "") if info else ""
        if host and user:
            # Explicit connection details — bypass registry
            actual_host = host
@@ -117,7 +123,7 @@ class RunnerManager:
                    name=name,
                )
            )
-        return actual_host, actual_user, actual_key, actual_mode
+        return actual_host, actual_user, actual_key, actual_mode, actual_gitea_url

    def start(
        self,
@@ -129,7 +135,9 @@ class RunnerManager:
        ask_become_pass: bool = False,
    ) -> None:
        """Start a runner instance on a remote host."""
-        actual_host, actual_user, actual_key, actual_mode = self._resolve_runner(name, host, user, key, mode)
+        actual_host, actual_user, actual_key, actual_mode, _gitea_url = self._resolve_runner(
+            name, host, user, key, mode
+        )
        with track_steps() as tracker:
            tracker.begin(_("Starting Gitea Runner {name} on {host}", name=name, host=actual_host))
            extra_vars = f"runner_name={name} runner_mode={actual_mode}"
@@ -148,7 +156,7 @@ class RunnerManager:
        ask_become_pass: bool = False,
    ) -> None:
        """Stop a runner instance on a remote host."""
-        actual_host, actual_user, actual_key, _mode = self._resolve_runner(name, host, user, key)
+        actual_host, actual_user, actual_key, _mode, _gitea_url = self._resolve_runner(name, host, user, key)
        with track_steps() as tracker:
            tracker.begin(_("Stopping Gitea Runner {name} on {host}", name=name, host=actual_host))
            extra_vars = f"runner_name={name}"
@@ -167,7 +175,7 @@ class RunnerManager:
        ask_become_pass: bool = False,
    ) -> None:
        """Enable a runner instance to start on boot."""
-        actual_host, actual_user, actual_key, _mode = self._resolve_runner(name, host, user, key)
+        actual_host, actual_user, actual_key, _mode, _gitea_url = self._resolve_runner(name, host, user, key)
        with track_steps() as tracker:
            tracker.begin(_("Enabling Gitea Runner {name} on {host}", name=name, host=actual_host))
            extra_vars = f"runner_name={name}"
@@ -191,15 +199,19 @@ class RunnerManager:
        ask_become_pass: bool = False,
    ) -> None:
        """Disable and deregister a runner instance."""
-        if not gitea_url:
+        actual_host, actual_user, actual_key, actual_mode, registry_gitea_url = self._resolve_runner(
+            name, host, user, key, mode
+        )
+        resolved_gitea_url = gitea_url or registry_gitea_url
+        if not resolved_gitea_url:
            raise AnsibleError(_("GITEA_URL must be set (or pass --url)"))
-        actual_host, actual_user, actual_key, actual_mode = self._resolve_runner(name, host, user, key, mode)
        if not token:
            raise AnsibleError(_("GITEA_REGISTRATION_TOKEN must be set (or pass --token)"))
        with track_steps() as tracker:
            tracker.begin(_("Disabling Gitea Runner {name} on {host}", name=name, host=actual_host))
            extra_vars = (
-                f"runner_name={name} registration_token={token} gitea_url={gitea_url} runner_mode={actual_mode}"
+                f"runner_name={name} registration_token={token}"
+                f" gitea_url={resolved_gitea_url} runner_mode={actual_mode}"
            )
            cmd = self._build_cmd(
                "disable-runner.yml", actual_host, actual_user, extra_vars, actual_key, ask_become_pass
@@ -219,7 +231,9 @@ class RunnerManager:
        ask_become_pass: bool = False,
    ) -> None:
        """Check the status of a runner instance."""
-        actual_host, actual_user, actual_key, actual_mode = self._resolve_runner(name, host, user, key, mode)
+        actual_host, actual_user, actual_key, actual_mode, _gitea_url = self._resolve_runner(
+            name, host, user, key, mode
+        )
        with track_steps() as tracker:
            tracker.begin(_("Checking status of Gitea Runner {name} on {host}", name=name, host=actual_host))
            extra_vars = f"runner_name={name} runner_mode={actual_mode}"
@@ -249,16 +263,22 @@ class RunnerManager:
        only remove the local registry entry. Use this when the remote
        host is already gone or unreachable.
        """
-        if not gitea_url:
-            raise AnsibleError(_("GITEA_URL must be set (or pass --url)"))
-        actual_host, actual_user, actual_key, actual_mode = self._resolve_runner(name, host, user, key, mode)
-        if not token:
-            raise AnsibleError(_("GITEA_REGISTRATION_TOKEN must be set (or pass --token)"))
+        actual_host, actual_user, actual_key, actual_mode, registry_gitea_url = self._resolve_runner(
+            name, host, user, key, mode
+        )
+        resolved_gitea_url = gitea_url or registry_gitea_url
+        resolved_token = token
+        if not force:
+            if not resolved_gitea_url:
+                raise AnsibleError(_("GITEA_URL must be set (or pass --url)"))
+            if not resolved_token:
+                raise AnsibleError(_("GITEA_REGISTRATION_TOKEN must be set (or pass --token)"))
        with track_steps() as tracker:
            tracker.begin(_("Removing Gitea Runner {name} from {host}", name=name, host=actual_host))
            if not force:
                extra_vars = (
-                    f"runner_name={name} registration_token={token} gitea_url={gitea_url} runner_mode={actual_mode}"
+                    f"runner_name={name} registration_token={resolved_token}"
+                    f" gitea_url={resolved_gitea_url} runner_mode={actual_mode}"
                )
                cmd = self._build_cmd(
                    "remove-runner.yml", actual_host, actual_user, extra_vars, actual_key, ask_become_pass
@@ -290,11 +310,6 @@ class RunnerManager:
                )
            )

-            def _parse_status(stdout: str) -> str:
-                ansible_noise = (" | CHANGED | ", " | FAILED | ", " | UNREACHABLE | ", "[WARNING]", "ssh:", ">>")
-                lines = [ln for ln in stdout.splitlines() if ln.strip() and not any(p in ln for p in ansible_noise)]
-                return lines[-1].strip() if lines else "unknown"
-
            service_status = "unknown"
            if mode == "docker":
                # Try expected container name first, then host-based fallback for legacy installs.
@@ -317,7 +332,7 @@ class RunnerManager:
                        )
                    except Exception:
                        continue
-                    status = _parse_status(stdout)
+                    status = self._parse_status(stdout)
                    if status != "unknown":
                        docker_map = {"running": "active", "exited": "inactive", "dead": "failed"}
                        service_status = docker_map.get(status, "unknown")
@@ -335,7 +350,7 @@ class RunnerManager:
                            ask_become_pass=True,
                            check=False,
                        )
-                        service_status = _parse_status(stdout)
+                        service_status = self._parse_status(stdout)
                    except Exception:
                        service_status = "unknown"
            else:
@@ -350,7 +365,7 @@ class RunnerManager:
                        ask_become_pass=True,
                        check=False,
                    )
-                    service_status = _parse_status(stdout)
+                    service_status = self._parse_status(stdout)
                except Exception:
                    service_status = "unknown"
            # Translate known status values
@@ -368,6 +383,12 @@ class RunnerManager:
            )
        return result

+    @staticmethod
+    def _parse_status(stdout: str) -> str:
+        ansible_noise = (" | CHANGED | ", " | FAILED | ", " | UNREACHABLE | ", "[WARNING]", "ssh:", ">>")
+        lines = [ln for ln in stdout.splitlines() if ln.strip() and not any(p in ln for p in ansible_noise)]
+        return lines[-1].strip() if lines else "unknown"
+
    def _build_cmd(
        self,
        playbook_name: str,
@@ -70,6 +70,31 @@ class TestCLI:
            assert result.exit_code != 0
            assert "GITEA_URL must be set" in result.output

+    @patch("gitea_runner_manager.cli.RunnerManager")
+    def test_install_with_url_flag(self, mock_manager_class: MagicMock) -> None:
+        mock_manager = MagicMock()
+        mock_manager_class.return_value = mock_manager
+
+        with patch.dict("os.environ", {"GITEA_REGISTRATION_TOKEN": "tok"}, clear=True):
+            runner = CliRunner()
+            result = runner.invoke(
+                cli,
+                ["install", "host1", "--user", "ubuntu", "--token", "tok", "--url", "https://git.example.com"],
+            )
+            assert result.exit_code == 0
+            mock_manager.install.assert_called_once_with(
+                host="host1",
+                user="ubuntu",
+                key=None,
+                name=None,
+                token="tok",
+                gitea_url="https://git.example.com",
+                mode="docker",
+                admin_token=None,
+                integration_retries=3,
+                ask_become_pass=True,
+            )
+
    @patch("gitea_runner_manager.cli.RunnerManager")
    def test_install_missing_token(self, mock_manager_class: MagicMock) -> None:
        mock_manager = MagicMock()
@@ -349,6 +374,26 @@ class TestCLI:
        assert result.exit_code != 0
        assert "GITEA_URL must be set" in result.output

+    @patch("gitea_runner_manager.cli.RunnerManager")
+    def test_disable_with_url_flag(self, mock_manager_class: MagicMock) -> None:
+        mock_manager = MagicMock()
+        mock_manager_class.return_value = mock_manager
+
+        with patch.dict("os.environ", {"GITEA_REGISTRATION_TOKEN": "tok"}, clear=True):
+            runner = CliRunner()
+            result = runner.invoke(cli, ["disable", "r1", "--token", "tok", "--url", "https://git.example.com"])
+        assert result.exit_code == 0
+        mock_manager.disable.assert_called_once_with(
+            name="r1",
+            host=None,
+            user=None,
+            key=None,
+            token="tok",
+            gitea_url="https://git.example.com",
+            mode=None,
+            ask_become_pass=True,
+        )
+
    @patch("gitea_runner_manager.cli.RunnerManager")
    def test_disable_error(self, mock_manager_class: MagicMock) -> None:
        mock_manager = MagicMock()
@@ -395,8 +440,8 @@ class TestCLI:
            token="tok",
            gitea_url="https://git.example.com",
            mode=None,
-            ask_become_pass=True,
            force=False,
+            ask_become_pass=True,
        )

    @patch("gitea_runner_manager.cli.RunnerManager")
@@ -415,8 +460,8 @@ class TestCLI:
            token="tok",
            gitea_url="https://git.example.com",
            mode=None,
-            ask_become_pass=True,
            force=True,
+            ask_become_pass=True,
        )

    @patch("gitea_runner_manager.cli.RunnerManager")
@@ -485,6 +530,27 @@ class TestCLI:
        assert result.exit_code != 0
        assert "GITEA_URL must be set" in result.output

+    @patch("gitea_runner_manager.cli.RunnerManager")
+    def test_remove_with_url_flag(self, mock_manager_class: MagicMock) -> None:
+        mock_manager = MagicMock()
+        mock_manager_class.return_value = mock_manager
+
+        with patch.dict("os.environ", {"GITEA_REGISTRATION_TOKEN": "tok"}, clear=True):
+            runner = CliRunner()
+            result = runner.invoke(cli, ["remove", "r1", "--token", "tok", "--url", "https://git.example.com"])
+        assert result.exit_code == 0
+        mock_manager.remove.assert_called_once_with(
+            name="r1",
+            host=None,
+            user=None,
+            key=None,
+            token="tok",
+            gitea_url="https://git.example.com",
+            mode=None,
+            force=False,
+            ask_become_pass=True,
+        )
+
    @patch("gitea_runner_manager.cli.RunnerManager")
    def test_remove_error(self, mock_manager_class: MagicMock) -> None:
        mock_manager = MagicMock()
@@ -186,34 +186,51 @@ class TestRunnerManager:

    def test_resolve_runner_from_registry(self) -> None:
        mock_registry = MagicMock()
-        mock_registry.get.return_value = {"host": "10.0.0.1", "user": "ubuntu", "key": "/key", "mode": "docker"}
+        mock_registry.get.return_value = {
+            "host": "10.0.0.1",
+            "user": "ubuntu",
+            "key": "/key",
+            "mode": "docker",
+            "gitea_url": "https://git.example.com",
+        }
        manager = RunnerManager(registry=mock_registry)
-        host, user, key, mode = manager._resolve_runner("r1")
+        host, user, key, mode, gitea_url = manager._resolve_runner("r1")
        assert host == "10.0.0.1"
        assert user == "ubuntu"
        assert key == "/key"
        assert mode == "docker"
+        assert gitea_url == "https://git.example.com"
        mock_registry.get.assert_called_once_with("r1")

    def test_resolve_runner_explicit_host_user(self) -> None:
        mock_registry = MagicMock()
        mock_registry.get.return_value = None
        manager = RunnerManager(registry=mock_registry)
-        host, user, key, mode = manager._resolve_runner("r1", host="10.0.0.2", user="root")
+        host, user, key, mode, gitea_url = manager._resolve_runner("r1", host="10.0.0.2", user="root")
        assert host == "10.0.0.2"
        assert user == "root"
        assert key is None
        assert mode == "docker"
+        assert gitea_url == ""

    def test_resolve_runner_override(self) -> None:
        mock_registry = MagicMock()
-        mock_registry.get.return_value = {"host": "10.0.0.1", "user": "ubuntu", "key": "/key", "mode": "docker"}
+        mock_registry.get.return_value = {
+            "host": "10.0.0.1",
+            "user": "ubuntu",
+            "key": "/key",
+            "mode": "docker",
+            "gitea_url": "https://git.example.com",
+        }
        manager = RunnerManager(registry=mock_registry)
-        host, user, key, mode = manager._resolve_runner("r1", host="10.0.0.2", user="root", key="/new", mode="binary")
+        host, user, key, mode, gitea_url = manager._resolve_runner(
+            "r1", host="10.0.0.2", user="root", key="/new", mode="binary"
+        )
        assert host == "10.0.0.2"
        assert user == "root"
        assert key == "/new"
        assert mode == "binary"
+        assert gitea_url == "https://git.example.com"

    def test_resolve_runner_not_found(self) -> None:
        mock_registry = MagicMock()
@@ -293,7 +310,13 @@ class TestRunnerManager:

    def test_disable(self) -> None:
        mock_registry = MagicMock()
-        mock_registry.get.return_value = {"host": "host", "user": "user", "key": None, "mode": "docker"}
+        mock_registry.get.return_value = {
+            "host": "host",
+            "user": "user",
+            "key": None,
+            "mode": "docker",
+            "gitea_url": "",
+        }
        manager = RunnerManager(registry=mock_registry)
        mock_executor = MagicMock()
        manager._executor = mock_executor
@@ -308,6 +331,24 @@ class TestRunnerManager:
        assert "runner_mode=docker" in cmd_str
        assert "Disabling Gitea Runner r1 on host" in mock_executor.run.call_args.kwargs["description"]

+    def test_disable_uses_registry_gitea_url(self) -> None:
+        mock_registry = MagicMock()
+        mock_registry.get.return_value = {
+            "host": "host",
+            "user": "user",
+            "key": None,
+            "mode": "docker",
+            "gitea_url": "https://registry.example.com",
+        }
+        manager = RunnerManager(registry=mock_registry)
+        mock_executor = MagicMock()
+        manager._executor = mock_executor
+
+        manager.disable("r1", token="tok")
+        cmd = mock_executor.run.call_args.args[0]
+        cmd_str = " ".join(cmd)
+        assert "gitea_url=https://registry.example.com" in cmd_str
+
    def test_disable_missing_token(self) -> None:
        mock_registry = MagicMock()
        mock_registry.get.return_value = {"host": "host", "user": "user"}
@@ -339,7 +380,13 @@ class TestRunnerManager:

    def test_remove(self) -> None:
        mock_registry = MagicMock()
-        mock_registry.get.return_value = {"host": "host", "user": "user", "key": None, "mode": "docker"}
+        mock_registry.get.return_value = {
+            "host": "host",
+            "user": "user",
+            "key": None,
+            "mode": "docker",
+            "gitea_url": "",
+        }
        manager = RunnerManager(registry=mock_registry)
        mock_executor = MagicMock()
        manager._executor = mock_executor
@@ -383,7 +430,13 @@ class TestRunnerManager:

    def test_remove_force_skips_playbook(self) -> None:
        mock_registry = MagicMock()
-        mock_registry.get.return_value = {"host": "host", "user": "user", "key": None, "mode": "docker"}
+        mock_registry.get.return_value = {
+            "host": "host",
+            "user": "user",
+            "key": None,
+            "mode": "docker",
+            "gitea_url": "",
+        }
        manager = RunnerManager(registry=mock_registry)
        mock_executor = MagicMock()
        manager._executor = mock_executor
@@ -392,6 +445,18 @@ class TestRunnerManager:
        mock_executor.run.assert_not_called()
        mock_registry.remove.assert_called_once_with("r1")

+    def test_remove_force_without_token_or_url(self) -> None:
+        """Force removal should not require token or gitea_url."""
+        mock_registry = MagicMock()
+        mock_registry.get.return_value = {"host": "host", "user": "user"}
+        manager = RunnerManager(registry=mock_registry)
+        mock_executor = MagicMock()
+        manager._executor = mock_executor
+
+        manager.remove("r1", force=True)
+        mock_executor.run.assert_not_called()
+        mock_registry.remove.assert_called_once_with("r1")
+
    def test_list_runners_docker(self) -> None:
        mock_registry = MagicMock()
        mock_registry.list.return_value = {