From 2bce91764fece0c3c85b8a692fe341e582b38703 Mon Sep 17 00:00:00 2001
From: charlie4284 <yangsoo.yoon@canonical.com>
Date: Wed, 8 May 2024 11:58:37 +0800
Subject: [PATCH 01/10] apt packages from config

---
 config.yaml        |  5 +++++
 src/charm.py       |  7 +++++++
 src/charm_state.py | 20 ++++++++++++++++++++
 src/service.py     | 16 ++++++++++++++++
 4 files changed, 48 insertions(+)

diff --git a/config.yaml b/config.yaml
index 4e3bf91..04a22dc 100644
--- a/config.yaml
+++ b/config.yaml
@@ -7,3 +7,8 @@ options:
     description: |
       Comma-separated list of labels to be assigned to the agent in Jenkins. If not set it will
       default to the agents hardware identifier, e.g.: 'x86_64'
+  apt-packages:
+    type: string
+    default: ""
+    description: |
+      Comma-separated list of apt packages to install on the machine.
diff --git a/src/charm.py b/src/charm.py
index 376e6f0..8ca723a 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -59,6 +59,13 @@ def _on_install(self, _: ops.InstallEvent) -> None:
 
     def _on_config_changed(self, _: ops.ConfigChangedEvent) -> None:
         """Handle config changed event. Update the agent's label in the relation's databag."""
+        try:
+            self.jenkins_agent_service.install_apt_packages(self.state.apt_packages)
+        except service.PackageInstallError as exc:
+            logger.error("Error installing apt packages %s", exc)
+            self.unit.status = ops.BlockedStatus("Error installing apt packages: %s")
+            return
+
         if agent_relation := self.model.get_relation(AGENT_RELATION):
             relation_data = self.state.agent_meta.as_dict()
             agent_relation.data[self.unit].update(relation_data)
diff --git a/src/charm_state.py b/src/charm_state.py
index 17110a9..ef502e7 100644
--- a/src/charm_state.py
+++ b/src/charm_state.py
@@ -17,6 +17,8 @@
 # agent relation name
 AGENT_RELATION = "agent"
 
+APT_PACKAGES_CONFIG = "apt-packages"
+
 logger = logging.getLogger()
 
 
@@ -115,6 +117,21 @@ def _get_credentials_from_agent_relation(
     return Credentials(address=address, secret=secret)
 
 
+def _get_packages_to_install(charm: ops.CharmBase) -> tuple[str, ...]:
+    """Get list of apt packages to install from charm configuration.
+
+    Args:
+        charm: The charm instance.
+
+    Returns:
+        The list of apt packages to install.
+    """
+    packages = charm.config.get(APT_PACKAGES_CONFIG, "")
+    if not packages:
+        return ()
+    return tuple(package.strip() for package in packages.split(","))
+
+
 @dataclass
 class State:
     """The Jenkins agent state.
@@ -123,12 +140,14 @@ class State:
         agent_meta: The Jenkins agent metadata to register on Jenkins server.
         agent_relation_credentials: The full set of credentials from the agent relation. None if
             partial data is set or the credentials do not belong to current agent.
+        apt_packages: The list of apt packages to install on the unit.
         unit_data: Data about the current unit.
         jenkins_agent_service_name: The Jenkins agent workload container name.
     """
 
     agent_meta: AgentMeta
     agent_relation_credentials: typing.Optional[Credentials]
+    apt_packages: tuple[str, ...]
     unit_data: UnitData
     jenkins_agent_service_name: str = "jenkins-agent"
 
@@ -176,5 +195,6 @@ def from_charm(cls, charm: ops.CharmBase) -> "State":
         return cls(
             agent_meta=agent_meta,
             agent_relation_credentials=agent_relation_credentials,
+            apt_packages=_get_packages_to_install(charm),
             unit_data=unit_data,
         )
diff --git a/src/service.py b/src/service.py
index 4f8b8d4..f2a8de9 100644
--- a/src/service.py
+++ b/src/service.py
@@ -8,6 +8,7 @@
 import pwd
 import time
 from pathlib import Path
+from typing import Iterable
 
 from charms.operator_libs_linux.v0 import apt
 from charms.operator_libs_linux.v1 import systemd
@@ -123,6 +124,21 @@ def install(self) -> None:
         except (apt.PackageError, apt.PackageNotFoundError, apt.GPGKeyError) as exc:
             raise PackageInstallError("Error installing the agent package") from exc
 
+    @classmethod
+    def install_apt_packages(cls, packages: Iterable[str]):
+        """Install apt packages.
+
+        Args:
+            packages: The apt packages to install.
+        """
+        to_install = list(packages)
+        if not to_install:
+            return
+        try:
+            apt.add_package(to_install, update_cache=True)
+        except apt.PackageNotFoundError as exc:
+            raise PackageInstallError from exc
+
     def restart(self) -> None:
         """Start the agent service.
 

From 1a34a6799fb64ac637b452d9b19d9fc6793ccb6b Mon Sep 17 00:00:00 2001
From: charlie4284 <yangsoo.yoon@canonical.com>
Date: Wed, 8 May 2024 12:11:46 +0800
Subject: [PATCH 02/10] add tests

---
 tests/integration/test_agent.py | 21 ++++++++++++++++++++
 tests/unit/test_service.py      | 34 +++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/tests/integration/test_agent.py b/tests/integration/test_agent.py
index 87892e8..dce0071 100644
--- a/tests/integration/test_agent.py
+++ b/tests/integration/test_agent.py
@@ -10,6 +10,9 @@
 import jenkinsapi.jenkins
 from juju.application import Application
 from juju.model import Model
+from juju.unit import Unit
+
+from charm_state import APT_PACKAGES_CONFIG
 
 from .conftest import NUM_AGENT_UNITS, assert_job_success
 
@@ -63,3 +66,21 @@ async def test_agent_relation(
     assert len(nodes.values()) == NUM_AGENT_UNITS + 1
 
     assert_job_success(jenkins_client, jenkins_agent_application.name, "machine")
+
+
+async def test_agent_packages(
+    model: Model,
+    jenkins_agent_application: Application,
+):
+    """
+    arrange: given a jenkins agent application.
+    act: when the apt-packages configuration is set.
+    assert: the defined packages are installed.
+    """
+    await jenkins_agent_application.set_config({APT_PACKAGES_CONFIG: "bzr, iputils-ping"})
+    await model.wait_for_idle()
+
+    unit: Unit = jenkins_agent_application.units[0]
+
+    assert "/usr/bin/bzr" == await unit.ssh(["which", "bzr"])
+    assert "/usr/bin/ping" == await unit.ssh(["which", "ping"])
diff --git a/tests/unit/test_service.py b/tests/unit/test_service.py
index fa6c552..a3caa2d 100644
--- a/tests/unit/test_service.py
+++ b/tests/unit/test_service.py
@@ -17,6 +17,7 @@
 import service
 from charm import JenkinsAgentCharm
 from charm_state import AGENT_RELATION
+from service import JenkinsAgentService, PackageInstallError
 
 
 @pytest.mark.parametrize(
@@ -72,6 +73,39 @@ def test_on_install(harness: ops.testing.Harness, monkeypatch: pytest.MonkeyPatc
     assert harness.charm.unit.status.name == ops.BlockedStatus.name
 
 
+def test_on_install_packages_fail(monkeypatch: pytest.MonkeyPatch):
+    """
+    arrange: Given a monkeypatched apt lib that raises an error.
+    act: when install_apt_packages is called.
+    assert: PackageInstallError is raised.
+    """
+    monkeypatch.setattr(apt, "add_package", MagicMock(side_effect=apt.PackageNotFoundError))
+
+    with pytest.raises(PackageInstallError):
+        JenkinsAgentService.install_apt_packages(["hello", "world"])
+
+
+@pytest.mark.parametrize(
+    "packages",
+    [
+        pytest.param(tuple(), id="No packages"),
+        pytest.param(tuple("hello", "world"), id="Has packages"),
+    ],
+)
+def test_on_install_packages(monkeypatch: pytest.MonkeyPatch, packages: tuple[str, ...]):
+    """
+    arrange: Given a monkeypatched apt lib and list of packages to install.
+    act: when install_apt_packages is called.
+    assert: package install call is made.
+    """
+    monkeypatch.setattr(apt, "add_package", (apt_mock := MagicMock()))
+
+    JenkinsAgentService.install_apt_packages(packages)
+
+    if packages:
+        apt_mock.assert_has_calls()
+
+
 def test_restart_service(
     harness: ops.testing.Harness,
     monkeypatch: pytest.MonkeyPatch,

From 71fb7e683a8cf35989a58b7eca4e3969c0023c7f Mon Sep 17 00:00:00 2001
From: charlie4284 <yangsoo.yoon@canonical.com>
Date: Wed, 8 May 2024 12:44:31 +0800
Subject: [PATCH 03/10] make lint happy

---
 src/charm.py               |  4 +++-
 src/service.py             |  7 +++++--
 tests/unit/test_charm.py   | 25 +++++++++++++++++++++++++
 tests/unit/test_service.py |  4 ++--
 tests/unit/test_state.py   | 26 ++++++++++++++++++++++++++
 5 files changed, 61 insertions(+), 5 deletions(-)

diff --git a/src/charm.py b/src/charm.py
index 8ca723a..46bf2f9 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -59,11 +59,13 @@ def _on_install(self, _: ops.InstallEvent) -> None:
 
     def _on_config_changed(self, _: ops.ConfigChangedEvent) -> None:
         """Handle config changed event. Update the agent's label in the relation's databag."""
+        logger.info("CONFIG CHANGED")
+        logger.info("%s", self.state.apt_packages)
         try:
             self.jenkins_agent_service.install_apt_packages(self.state.apt_packages)
         except service.PackageInstallError as exc:
             logger.error("Error installing apt packages %s", exc)
-            self.unit.status = ops.BlockedStatus("Error installing apt packages: %s")
+            self.model.unit.status = ops.BlockedStatus("Error installing apt packages: %s")
             return
 
         if agent_relation := self.model.get_relation(AGENT_RELATION):
diff --git a/src/service.py b/src/service.py
index f2a8de9..93f8de8 100644
--- a/src/service.py
+++ b/src/service.py
@@ -125,18 +125,21 @@ def install(self) -> None:
             raise PackageInstallError("Error installing the agent package") from exc
 
     @classmethod
-    def install_apt_packages(cls, packages: Iterable[str]):
+    def install_apt_packages(cls, packages: Iterable[str]) -> None:
         """Install apt packages.
 
         Args:
             packages: The apt packages to install.
+
+        Raises:
+            PackageInstallError: If there was an error installing the package.
         """
         to_install = list(packages)
         if not to_install:
             return
         try:
             apt.add_package(to_install, update_cache=True)
-        except apt.PackageNotFoundError as exc:
+        except apt.Error as exc:
             raise PackageInstallError from exc
 
     def restart(self) -> None:
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
index f17cd27..f70cf6a 100644
--- a/tests/unit/test_charm.py
+++ b/tests/unit/test_charm.py
@@ -5,16 +5,21 @@
 
 """Test for charm hooks."""
 
+# Need access to protected functions for testing
+# pylint:disable=protected-access
+
 from unittest.mock import MagicMock, PropertyMock
 
 import ops
 import ops.testing
 import pytest
+from charms.operator_libs_linux.v0 import apt
 from charms.operator_libs_linux.v1 import systemd
 
 import charm_state
 import service
 from charm import JenkinsAgentCharm
+from charm_state import APT_PACKAGES_CONFIG
 
 
 def test___init___invalid_state(harness: ops.testing.Harness, monkeypatch: pytest.MonkeyPatch):
@@ -53,6 +58,26 @@ def test__on_upgrade_charm(harness: ops.testing.Harness, monkeypatch: pytest.Mon
     assert charm.unit.status.name == ops.BlockedStatus.name
 
 
+def test__on_config_changed_apt_install_fail(
+    harness: ops.testing.Harness, monkeypatch: pytest.MonkeyPatch
+):
+    """
+    arrange: given a charm with patched relation.
+    act: when _on_config_changed is called.
+    assert: The charm correctly updates the relation databag.
+    """
+    harness.update_config({APT_PACKAGES_CONFIG: "hello,world"})
+    harness.begin()
+    get_relation_mock = MagicMock()
+    monkeypatch.setattr(apt, "add_package", MagicMock(side_effect=apt.PackageNotFoundError))
+    monkeypatch.setattr(ops.Model, "get_relation", get_relation_mock)
+
+    charm: JenkinsAgentCharm = harness.charm
+    charm._on_config_changed(MagicMock())
+
+    assert charm.unit.status.name == ops.BlockedStatus.name
+
+
 def test__on_config_changed(harness: ops.testing.Harness, monkeypatch: pytest.MonkeyPatch):
     """
     arrange: given a charm with patched relation.
diff --git a/tests/unit/test_service.py b/tests/unit/test_service.py
index a3caa2d..2e1ee70 100644
--- a/tests/unit/test_service.py
+++ b/tests/unit/test_service.py
@@ -89,7 +89,7 @@ def test_on_install_packages_fail(monkeypatch: pytest.MonkeyPatch):
     "packages",
     [
         pytest.param(tuple(), id="No packages"),
-        pytest.param(tuple("hello", "world"), id="Has packages"),
+        pytest.param(("hello", "world"), id="Has packages"),
     ],
 )
 def test_on_install_packages(monkeypatch: pytest.MonkeyPatch, packages: tuple[str, ...]):
@@ -103,7 +103,7 @@ def test_on_install_packages(monkeypatch: pytest.MonkeyPatch, packages: tuple[st
     JenkinsAgentService.install_apt_packages(packages)
 
     if packages:
-        apt_mock.assert_has_calls()
+        apt_mock.assert_called_once()
 
 
 def test_restart_service(
diff --git a/tests/unit/test_state.py b/tests/unit/test_state.py
index 49d5e08..edfc30c 100644
--- a/tests/unit/test_state.py
+++ b/tests/unit/test_state.py
@@ -5,6 +5,9 @@
 
 """Test for charm state."""
 
+# Need access to protected functions for testing
+# pylint:disable=protected-access
+
 import os
 from unittest.mock import MagicMock
 
@@ -14,6 +17,7 @@
 
 import charm_state
 from charm import JenkinsAgentCharm
+from charm_state import APT_PACKAGES_CONFIG
 
 
 def test_from_charm_invalid_metadata(
@@ -30,3 +34,25 @@ def test_from_charm_invalid_metadata(
 
     with pytest.raises(charm_state.InvalidStateError, match="Invalid executor state."):
         charm_state.State.from_charm(charm=charm)
+
+
+@pytest.mark.parametrize(
+    "packages, expected",
+    [
+        pytest.param("", tuple(), id="empty"),
+        pytest.param("git,bzr", ("git", "bzr"), id="has packages"),
+    ],
+)
+def test__get_packages_to_install(
+    harness: ops.testing.Harness, packages: str, expected: tuple[str, ...]
+):
+    """
+    arrange: given charm apt-packages config.
+    act: when _get_packages_to_install is called.
+    assert: packages are correctly parsed.
+    """
+    harness.begin()
+    harness.update_config({APT_PACKAGES_CONFIG: packages})
+    charm: JenkinsAgentCharm = harness.charm
+
+    assert charm_state._get_packages_to_install(charm=charm) == expected

From 1f803e09962bbbe8ad361d1a658d122cee673d56 Mon Sep 17 00:00:00 2001
From: charlie4284 <yangsoo.yoon@canonical.com>
Date: Wed, 8 May 2024 13:25:04 +0800
Subject: [PATCH 04/10] debug

---
 .github/workflows/integration_test.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.github/workflows/integration_test.yaml b/.github/workflows/integration_test.yaml
index e12cd6e..2aebed7 100644
--- a/.github/workflows/integration_test.yaml
+++ b/.github/workflows/integration_test.yaml
@@ -17,3 +17,4 @@ jobs:
         ./tests/integration/pre_run_script.sh"
       juju-channel: 3.1/stable
       channel: 1.27-strict/stable
+      tmate-ssh-debug: true

From de66eb64cc17e444a48d88d10de4b8768a913931 Mon Sep 17 00:00:00 2001
From: charlie4284 <yangsoo.yoon@canonical.com>
Date: Wed, 8 May 2024 13:51:23 +0800
Subject: [PATCH 05/10] debug

---
 .github/workflows/integration_test.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.github/workflows/integration_test.yaml b/.github/workflows/integration_test.yaml
index 2aebed7..5053aea 100644
--- a/.github/workflows/integration_test.yaml
+++ b/.github/workflows/integration_test.yaml
@@ -17,4 +17,4 @@ jobs:
         ./tests/integration/pre_run_script.sh"
       juju-channel: 3.1/stable
       channel: 1.27-strict/stable
-      tmate-ssh-debug: true
+      tmate-debug: true

From 2b658a585c0fa1c2c9ce8ae6d07965a11f76e67b Mon Sep 17 00:00:00 2001
From: charlie4284 <yangsoo.yoon@canonical.com>
Date: Wed, 8 May 2024 14:19:05 +0800
Subject: [PATCH 06/10] fix typing

---
 src/charm_state.py         | 4 ++--
 tests/unit/test_service.py | 3 ++-
 tests/unit/test_state.py   | 3 ++-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/charm_state.py b/src/charm_state.py
index ef502e7..3959c8b 100644
--- a/src/charm_state.py
+++ b/src/charm_state.py
@@ -117,7 +117,7 @@ def _get_credentials_from_agent_relation(
     return Credentials(address=address, secret=secret)
 
 
-def _get_packages_to_install(charm: ops.CharmBase) -> tuple[str, ...]:
+def _get_packages_to_install(charm: ops.CharmBase) -> typing.Tuple[str, ...]:
     """Get list of apt packages to install from charm configuration.
 
     Args:
@@ -147,7 +147,7 @@ class State:
 
     agent_meta: AgentMeta
     agent_relation_credentials: typing.Optional[Credentials]
-    apt_packages: tuple[str, ...]
+    apt_packages: typing.Tuple[str, ...]
     unit_data: UnitData
     jenkins_agent_service_name: str = "jenkins-agent"
 
diff --git a/tests/unit/test_service.py b/tests/unit/test_service.py
index 2e1ee70..418837c 100644
--- a/tests/unit/test_service.py
+++ b/tests/unit/test_service.py
@@ -6,6 +6,7 @@
 """Test for service interaction."""
 
 import os
+import typing
 from pathlib import Path
 from unittest.mock import MagicMock
 
@@ -92,7 +93,7 @@ def test_on_install_packages_fail(monkeypatch: pytest.MonkeyPatch):
         pytest.param(("hello", "world"), id="Has packages"),
     ],
 )
-def test_on_install_packages(monkeypatch: pytest.MonkeyPatch, packages: tuple[str, ...]):
+def test_on_install_packages(monkeypatch: pytest.MonkeyPatch, packages: typing.Tuple[str, ...]):
     """
     arrange: Given a monkeypatched apt lib and list of packages to install.
     act: when install_apt_packages is called.
diff --git a/tests/unit/test_state.py b/tests/unit/test_state.py
index edfc30c..d4ea44e 100644
--- a/tests/unit/test_state.py
+++ b/tests/unit/test_state.py
@@ -9,6 +9,7 @@
 # pylint:disable=protected-access
 
 import os
+import typing
 from unittest.mock import MagicMock
 
 import ops
@@ -44,7 +45,7 @@ def test_from_charm_invalid_metadata(
     ],
 )
 def test__get_packages_to_install(
-    harness: ops.testing.Harness, packages: str, expected: tuple[str, ...]
+    harness: ops.testing.Harness, packages: str, expected: typing.Tuple[str, ...]
 ):
     """
     arrange: given charm apt-packages config.

From df785712d0bad2239cbe3b7f6cf476b31609e972 Mon Sep 17 00:00:00 2001
From: charlie4284 <yangsoo.yoon@canonical.com>
Date: Wed, 8 May 2024 22:43:03 +0800
Subject: [PATCH 07/10] fix ssh command

---
 tests/integration/test_agent.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/integration/test_agent.py b/tests/integration/test_agent.py
index dce0071..dfd8a2a 100644
--- a/tests/integration/test_agent.py
+++ b/tests/integration/test_agent.py
@@ -82,5 +82,5 @@ async def test_agent_packages(
 
     unit: Unit = jenkins_agent_application.units[0]
 
-    assert "/usr/bin/bzr" == await unit.ssh(["which", "bzr"])
-    assert "/usr/bin/ping" == await unit.ssh(["which", "ping"])
+    assert "/usr/bin/bzr" == await unit.ssh("which bzr")
+    assert "/usr/bin/ping" == await unit.ssh("which ping")

From 1b356841b3e656be275fd31ebcd291f676aa253b Mon Sep 17 00:00:00 2001
From: charlie4284 <yangsoo.yoon@canonical.com>
Date: Thu, 9 May 2024 09:19:26 +0800
Subject: [PATCH 08/10] add newline

---
 tests/integration/test_agent.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/integration/test_agent.py b/tests/integration/test_agent.py
index dfd8a2a..6ababab 100644
--- a/tests/integration/test_agent.py
+++ b/tests/integration/test_agent.py
@@ -82,5 +82,5 @@ async def test_agent_packages(
 
     unit: Unit = jenkins_agent_application.units[0]
 
-    assert "/usr/bin/bzr" == await unit.ssh("which bzr")
-    assert "/usr/bin/ping" == await unit.ssh("which ping")
+    assert "/usr/bin/bzr\n" == await unit.ssh("which bzr")
+    assert "/usr/bin/ping\n" == await unit.ssh("which ping")

From 23f53ef194fca7be2c05cb4ad0de059e9a547946 Mon Sep 17 00:00:00 2001
From: charlie4284 <yangsoo.yoon@canonical.com>
Date: Thu, 9 May 2024 10:26:11 +0800
Subject: [PATCH 09/10] revert debug

---
 .github/workflows/integration_test.yaml | 1 -
 1 file changed, 1 deletion(-)

diff --git a/.github/workflows/integration_test.yaml b/.github/workflows/integration_test.yaml
index 5053aea..e12cd6e 100644
--- a/.github/workflows/integration_test.yaml
+++ b/.github/workflows/integration_test.yaml
@@ -17,4 +17,3 @@ jobs:
         ./tests/integration/pre_run_script.sh"
       juju-channel: 3.1/stable
       channel: 1.27-strict/stable
-      tmate-debug: true

From 442c9bd252eef52f80bd91b41c662d48d46a8343 Mon Sep 17 00:00:00 2001
From: charlie4284 <yangsoo.yoon@canonical.com>
Date: Thu, 9 May 2024 14:08:59 +0800
Subject: [PATCH 10/10] remove debug logs

---
 src/charm.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/charm.py b/src/charm.py
index 46bf2f9..92cebfa 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -59,8 +59,6 @@ def _on_install(self, _: ops.InstallEvent) -> None:
 
     def _on_config_changed(self, _: ops.ConfigChangedEvent) -> None:
         """Handle config changed event. Update the agent's label in the relation's databag."""
-        logger.info("CONFIG CHANGED")
-        logger.info("%s", self.state.apt_packages)
         try:
             self.jenkins_agent_service.install_apt_packages(self.state.apt_packages)
         except service.PackageInstallError as exc: