refactor(agent-charm): move methods to charm utilities

agent/charms/testflinger-agent-host-charm/src/charm.py

@@ -7,15 +7,15 @@
 
 import logging
 import os
-import shutil
 import sys
 from pathlib import Path
 
+import charm_utils
 import ops
 import supervisord
 import testflinger_client
 import testflinger_source
-from charmlibs import apt, passwd
+from charmlibs import apt
 from charms.grafana_agent.v0.cos_agent import COSAgentProvider
 from common import copy_ssh_keys, run_with_logged_errors, update_charm_scripts
 from config import TestflingerAgentConfig
@@ -24,7 +24,6 @@
     LOCAL_TESTFLINGER_PATH,
     VIRTUAL_ENV_PATH,
 )
-from git import GitCommandError, Repo
 from jinja2 import Template
 
 logger = logging.getLogger(__name__)
@@ -63,13 +62,14 @@ def __init__(self, *args):
     def on_install(self, _):
         """Install hook."""
         self.install_dependencies()
-        self.setup_docker()
+        charm_utils.setup_docker()
         self.update_tf_cmd_scripts()
         self.update_testflinger_repo()
         try:
-            self.update_config_files()
-        except ValueError:
-            self._block("config-repo and config-dir must be set")
+            charm_utils.update_config_files(self.typed_config)
+        except (RuntimeError, OSError) as err:
+            logger.error(err)
+            self._block("Failed to update or config files")
             return
 
     def install_dependencies(self):
@@ -78,19 +78,12 @@ def install_dependencies(self):
         # maas cli comes from maas snap now
         run_with_logged_errors(["snap", "install", "maas"])
 
-        self.install_apt_packages(
-            [
-                "python3-pip",
-                "python3-virtualenv",
-                "pipx",
-                "docker.io",
-                "git",
-                "openssh-client",
-                "sshpass",
-                "snmp",
-                "supervisor",
-            ]
-        )
+        try:
+            charm_utils.install_agent_packages()
+        except (apt.PackageNotFoundError, apt.PackageError):
+            self.logger.error("An error occured while installing dependencies")
+            self._block("Failed to install dependencies")
+            return
         run_with_logged_errors(["pipx", "install", "uv"])
 
     def update_testflinger_repo(self, branch: str | None = None):
@@ -105,37 +98,6 @@ def update_testflinger_repo(self, branch: str | None = None):
         else:
             testflinger_source.clone_repo(LOCAL_TESTFLINGER_PATH)
 
-    def update_config_files(self):
-        """
-        Clone the config files from the repo and swap it in for whatever is
-        in AGENT_CONFIGS_PATH.
-        """
-        if (
-            not self.typed_config.config_repo
-            or not self.typed_config.config_dir
-        ):
-            logger.error("config-repo and config-dir must be set")
-            raise ValueError("config-repo and config-dir must be set")
-        tmp_repo_path = Path("/srv/tmp-agent-configs")
-        repo_path = Path(AGENT_CONFIGS_PATH)
-        if tmp_repo_path.exists():
-            shutil.rmtree(tmp_repo_path, ignore_errors=True)
-        try:
-            Repo.clone_from(
-                url=self.typed_config.config_repo,
-                branch=self.typed_config.config_branch,
-                to_path=tmp_repo_path,
-                depth=1,
-            )
-        except GitCommandError:
-            logger.exception("Failed to update config files")
-            self._block("Failed to update or config files")
-            sys.exit(1)
-
-        if repo_path.exists():
-            shutil.rmtree(repo_path, ignore_errors=True)
-        shutil.move(tmp_repo_path, repo_path)
-
     def write_supervisor_service_files(self):
         """
         Generate supervisord service files for all agents.
@@ -216,10 +178,6 @@ def write_supervisor_service_files(self):
             ) as agent_file:
                 agent_file.write(rendered)
 
-    def setup_docker(self):
-        passwd.add_group("docker")
-        passwd.add_user_to_group("ubuntu", "docker")
-
     def update_tf_cmd_scripts(self):
         """Update tf-cmd-scripts."""
         self.unit.status = ops.MaintenanceStatus("Installing tf-cmd-scripts")
@@ -320,10 +278,12 @@ def on_config_changed(self, _):
             "Handling config_changed hook"
         )
         try:
-            self.update_config_files()
-        except ValueError:
-            self._block("config-repo and config-dir must be set")
+            charm_utils.update_config_files(self.typed_config)
+        except (RuntimeError, OSError) as err:
+            logger.error(err)
+            self._block("Failed to update or config files")
             return
+
         copy_ssh_keys(self.typed_config)
         self.update_tf_cmd_scripts()
         self.write_supervisor_service_files()
@@ -333,20 +293,6 @@ def on_config_changed(self, _):
             return
         self.unit.status = ops.ActiveStatus()
 
-    def install_apt_packages(self, packages: list):
-        """Wrap 'apt-get install -y."""
-        try:
-            apt.update()
-            apt.add_package(packages)
-        except apt.PackageNotFoundError:
-            logger.error(
-                "a specified package not found in package cache or on system"
-            )
-            self._block("Failed to install packages")
-        except apt.PackageError:
-            logger.error("could not install package")
-            self._block("Failed to install packages")
-
     def on_update_testflinger_action(self, event: ops.ActionEvent):
         """Update Testflinger agent code."""
         self.unit.status = ops.MaintenanceStatus(
@@ -363,10 +309,12 @@ def on_update_configs_action(self, event: ops.ActionEvent):
             "Updating Testflinger Agent Configs"
         )
         try:
-            self.update_config_files()
-        except ValueError:
-            self._block("config-repo and config-dir must be set")
+            charm_utils.update_config_files(self.typed_config)
+        except (RuntimeError, OSError) as err:
+            logger.error(err)
+            self._block("Failed to update or config files")
             return
+
         self.write_supervisor_service_files()
         supervisord.supervisor_update()
         supervisord.restart_agents()

agent/charms/testflinger-agent-host-charm/src/charm_utils.py

@@ -0,0 +1,64 @@
+# Copyright 2026 Canonical
+# See LICENSE file for licensing details.
+
+import logging
+import shutil
+from pathlib import Path
+
+from charmlibs import apt, passwd
+from config import TestflingerAgentConfig
+from defaults import AGENT_CONFIGS_PATH
+from git import GitCommandError, Repo
+
+logger = logging.getLogger(__name__)
+
+AGENT_PACKAGES = [
+    "python3-pip",
+    "python3-virtualenv",
+    "pipx",
+    "docker.io",
+    "git",
+    "openssh-client",
+    "sshpass",
+    "snmp",
+    "supervisor",
+]
+
+CHARM_USER = "ubuntu"
+DOCKER_GROUP = "docker"
+
+
+def install_agent_packages() -> None:
+    """Install necessary packages for the agents."""
+    apt.update()
+    apt.add_package(AGENT_PACKAGES)
+
+
+def setup_docker():
+    """Set up docker group and add charm user to the docker group."""
+    passwd.add_group(DOCKER_GROUP)
+    passwd.add_user_to_group(CHARM_USER, DOCKER_GROUP)
+
+
+def update_config_files(config: TestflingerAgentConfig):
+    """
+    Clone the config files from the repo and swap it in for whatever is
+    in AGENT_CONFIGS_PATH.
+    """
+    tmp_repo_path = Path("/srv/tmp-agent-configs")
+    repo_path = Path(AGENT_CONFIGS_PATH)
+    if tmp_repo_path.exists():
+        shutil.rmtree(tmp_repo_path)
+    try:
+        Repo.clone_from(
+            url=config.config_repo,
+            branch=config.config_branch,
+            to_path=tmp_repo_path,
+            depth=1,
+        )
+    except GitCommandError as err:
+        logger.error("Failed to update config files: %s", err)
+        raise RuntimeError("Failed to update or config files") from err
+    if repo_path.exists():
+        shutil.rmtree(repo_path)
+    shutil.move(tmp_repo_path, repo_path)

agent/charms/testflinger-agent-host-charm/src/config.py

@@ -35,3 +35,11 @@ def validate_server(cls, value):
                 "testflinger_server must include protocol (http:// or https://)"
             )
         return value
+
+    @pydantic.field_validator("config_repo", "config_dir")
+    @classmethod
+    def validate_config_paths(cls, value):
+        """Validate that both config_repo and config_dir have values."""
+        if not value:
+            raise ValueError("config_repo and config_dir must be set")
+        return value

agent/charms/testflinger-agent-host-charm/tests/unit/conftest.py

@@ -7,6 +7,7 @@
 
 import pytest
 from charm import TestflingerAgentHostCharm
+from config import TestflingerAgentConfig
 from ops import testing
 
 
@@ -24,6 +25,15 @@ def secret() -> testing.Secret:
     )
 
 
+@pytest.fixture
+def config() -> TestflingerAgentConfig:
+    """Fixture to provide default config values for testing."""
+    return TestflingerAgentConfig(
+        config_repo="some-repo",
+        config_dir="agent-configs",
+    )
+
+
 @pytest.fixture
 def state_in() -> Callable[..., testing.State]:
     """Create a testing state with configurable config and secrets."""

agent/charms/testflinger-agent-host-charm/tests/unit/test_charm.py

@@ -6,33 +6,34 @@
 
 from unittest.mock import patch
 
+import pytest
 from ops import testing
 
 
 def test_blocked_on_no_config_repo(ctx, state_in):
-    """Test blocked status when config-repo is not set."""
+    """Test charm aborts when config-repo is not set."""
+    # Pydantic validation runs in __init__ via load_config(errors="blocked"),
+    # which sets BlockedStatus and raises _Abort before the hook handler runs.
     state = state_in(config={"config-repo": ""})
-    state_out = ctx.run(ctx.on.config_changed(), state=state)
-    assert state_out.unit_status == testing.BlockedStatus(
-        "config-repo and config-dir must be set"
-    )
+    with pytest.raises(testing.errors.UncaughtCharmError):
+        ctx.run(ctx.on.config_changed(), state=state)
 
 
 def test_blocked_on_no_config_dir(ctx, state_in):
-    """Test blocked status when config-dir is not set."""
+    """Test charm aborts when config-dir is not set."""
+    # Pydantic validation runs in __init__ via load_config(errors="blocked"),
+    # which sets BlockedStatus and raises _Abort before the hook handler runs.
     state = state_in(config={"config-dir": ""})
-    state_out = ctx.run(ctx.on.config_changed(), state=state)
-    assert state_out.unit_status == testing.BlockedStatus(
-        "config-repo and config-dir must be set"
-    )
+    with pytest.raises(testing.errors.UncaughtCharmError):
+        ctx.run(ctx.on.config_changed(), state=state)
 
 
 @patch("charm.update_charm_scripts")
 @patch("charm.copy_ssh_keys")
 @patch("charm.supervisord.supervisor_update")
 @patch("charm.TestflingerAgentHostCharm.write_supervisor_service_files")
 @patch("charm.supervisord.restart_agents")
-@patch("charm.TestflingerAgentHostCharm.update_config_files")
+@patch("charm.charm_utils.update_config_files")
 def test_update_tf_cmd_scripts_on_config_changed(
     mock_update_config,
     mock_restart,
@@ -126,7 +127,7 @@ def test_start_active_on_successful_authentication(
 @patch("charm.TestflingerAgentHostCharm.write_supervisor_service_files")
 @patch("charm.update_charm_scripts")
 @patch("charm.copy_ssh_keys")
-@patch("charm.TestflingerAgentHostCharm.update_config_files")
+@patch("charm.charm_utils.update_config_files")
 def test_config_changed_blocked_on_authentication_failure(
     mock_update_config,
     mock_copy_ssh,
@@ -169,3 +170,63 @@ def test_authentication_triggered_on_update_status(
     """Test that authentication is triggered on update_status event."""
     ctx.run(ctx.on.update_status(), state=state_in())
     mock_authenticate_with_server.assert_called_once()
+
+
+@patch("charm.charm_utils.update_config_files")
+@patch("charm.TestflingerAgentHostCharm.update_testflinger_repo")
+@patch("charm.TestflingerAgentHostCharm.update_tf_cmd_scripts")
+@patch("charm.charm_utils.setup_docker")
+@patch("charm.TestflingerAgentHostCharm.install_dependencies")
+def test_on_install_methods_called(
+    mock_install_deps,
+    mock_setup_docker,
+    mock_update_tf_cmd_scripts,
+    mock_update_testflinger_repo,
+    mock_update_config_files,
+    ctx,
+    state_in,
+):
+    """Test that all expected methods are called during install."""
+    ctx.run(ctx.on.install(), state=state_in())
+
+    mock_install_deps.assert_called_once()
+    mock_setup_docker.assert_called_once()
+    mock_update_tf_cmd_scripts.assert_called_once()
+    mock_update_testflinger_repo.assert_called_once()
+    mock_update_config_files.assert_called_once()
+
+
+def test_on_install_blocked_on_missing_config(ctx, state_in):
+    """Test that install is blocked when required config is missing."""
+    # Pydantic validation runs in __init__ via load_config(errors="blocked"),
+    # which sets BlockedStatus and raises _Abort before the hook handler runs.
+    state = state_in(config={"config-repo": ""})
+    with pytest.raises(testing.errors.UncaughtCharmError):
+        ctx.run(ctx.on.install(), state=state)
+
+
+@patch("charm.supervisord.restart_agents")
+@patch("charm.TestflingerAgentHostCharm.update_testflinger_repo")
+def test_on_update_testflinger_action_with_branch(
+    mock_update_testflinger_repo,
+    mock_restart,
+    ctx,
+    state_in,
+):
+    """Test action call update testflinger code with the correct branch."""
+    state_out = ctx.run(
+        ctx.on.action("update-testflinger", params={"branch": "test-branch"}),
+        state=state_in(),
+    )
+    mock_update_testflinger_repo.assert_called_once_with("test-branch")
+    mock_restart.assert_called_once()
+    assert state_out.unit_status == testing.ActiveStatus()
+
+
+def test_update_configs_action_blocked_on_missing_config(ctx, state_in):
+    """Test update-configs action is blocked when config is missing."""
+    # Pydantic validation runs in __init__ via load_config(errors="blocked"),
+    # which sets BlockedStatus and raises _Abort before the hook handler runs.
+    state = state_in(config={"config-repo": ""})
+    with pytest.raises(testing.errors.UncaughtCharmError):
+        ctx.run(ctx.on.action("update-configs"), state=state)