From a5533d70ffe86ec7fb1471b171dbbb323a2e31ba Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Fri, 12 Dec 2025 18:53:07 -0500 Subject: [PATCH 01/19] Do not set worker info in worker service filename and add HOME env var --- src/charm.py | 2 -- src/git_ubuntu.py | 15 +++++++-------- src/importer_node.py | 33 ++++++++++++--------------------- 3 files changed, 19 insertions(+), 31 deletions(-) diff --git a/src/charm.py b/src/charm.py index e6f12c3..92da1db 100755 --- a/src/charm.py +++ b/src/charm.py @@ -318,8 +318,6 @@ def _refresh_importer_node(self) -> None: if not node.setup_secondary_node( GIT_UBUNTU_USER_HOME_DIR, - self._node_id, - self._num_workers, GIT_UBUNTU_SYSTEM_USER_USERNAME, self._is_publishing_active, self._controller_port, diff --git a/src/git_ubuntu.py b/src/git_ubuntu.py index 5a9a6ac..6cf1aa2 100644 --- a/src/git_ubuntu.py +++ b/src/git_ubuntu.py @@ -223,23 +223,21 @@ def setup_poller_service( def setup_worker_service( - local_folder: str, + home_dir: str, user: str, group: str, - worker_name: str = "", push_to_lp: bool = True, broker_ip: str = "127.0.0.1", broker_port: int = 1692, lp_credentials_filename: str = "", https_proxy: str = "", ) -> bool: - """Set up worker systemd file with designated worker name. + """Set up worker systemd file. Args: - local_folder: The local folder to store the service in. + home_dir: The home directory of the user. user: The user to run the service as. group: The permissions group to run the service as. - worker_name: The unique worker ID to add to the service filename. push_to_lp: True if publishing repositories to Launchpad. broker_ip: The IP address of the broker process' node. broker_port: The network port that the broker provides tasks on. @@ -249,13 +247,13 @@ def setup_worker_service( Returns: True if setup succeeded, False otherwise. """ - filename = f"git-ubuntu-importer-service-worker{worker_name}.service" + filename = "git-ubuntu-importer-service-worker@.service" publish_arg = " --no-push" if not push_to_lp else "" broker_url = f"tcp://{broker_ip}:{broker_port}" exec_start = f"/snap/bin/git-ubuntu importer-service-worker{publish_arg} %i {broker_url}" - environment = "PYTHONUNBUFFERED=1" + environment = f"HOME={home_dir} PYTHONUNBUFFERED=1" if lp_credentials_filename != "": environment = f"LP_CREDENTIALS_FILE={lp_credentials_filename} " + environment @@ -279,7 +277,8 @@ def setup_worker_service( wanted_by="multi-user.target", ) - return create_systemd_service_file(filename, local_folder, service_string) + services_folder = pathops.LocalPath(home_dir, "services") + return create_systemd_service_file(filename, services_folder.as_posix(), service_string) def start_services(service_folder: str) -> bool: diff --git a/src/importer_node.py b/src/importer_node.py index d52625b..4e955da 100644 --- a/src/importer_node.py +++ b/src/importer_node.py @@ -15,8 +15,6 @@ def setup_secondary_node( git_ubuntu_user_home: str, - node_id: int, - num_workers: int, system_user: str, push_to_lp: bool, primary_port: int, @@ -28,8 +26,6 @@ def setup_secondary_node( Args: git_ubuntu_user_home: The home directory of the git-ubuntu user. - node_id: The unique ID of this node. - num_workers: The number of worker instances to set up. system_user: The user + group to run the services as. push_to_lp: True if publishing repositories to Launchpad. primary_port: The network port used for worker assignments. @@ -40,23 +36,18 @@ def setup_secondary_node( Returns: True if installation succeeded, False otherwise. """ - services_folder = pathops.LocalPath(git_ubuntu_user_home, "services") - - for i in range(num_workers): - worker_name = f"{node_id}_{i}" - if not git_ubuntu.setup_worker_service( - services_folder.as_posix(), - system_user, - system_user, - worker_name, - push_to_lp, - primary_ip, - primary_port, - lp_credentials_filename, - https_proxy, - ): - logger.error("Failed to setup worker %s service.", worker_name) - return False + if not git_ubuntu.setup_worker_service( + git_ubuntu_user_home, + system_user, + system_user, + push_to_lp, + primary_ip, + primary_port, + lp_credentials_filename, + https_proxy, + ): + logger.error("Failed to setup worker service file.") + return False return True From 9f3dd03dd579c6f88759951575d7b26fb10e2770 Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Fri, 12 Dec 2025 18:58:47 -0500 Subject: [PATCH 02/19] Handle services folder consistently with workers --- src/git_ubuntu.py | 14 ++++++++------ src/importer_node.py | 6 ++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/git_ubuntu.py b/src/git_ubuntu.py index 6cf1aa2..92d0654 100644 --- a/src/git_ubuntu.py +++ b/src/git_ubuntu.py @@ -139,7 +139,7 @@ def generate_systemd_service_string( def setup_broker_service( - local_folder: str, + home_dir: str, user: str, group: str, broker_port: int = 1692, @@ -147,7 +147,7 @@ def setup_broker_service( """Set up broker systemd service file. Args: - local_folder: The local folder to store the service in. + home_dir: The home directory of the user. user: The user to run the service as. group: The permissions group to run the service as. broker_port: The network port to provide tasks to workers on. @@ -170,11 +170,12 @@ def setup_broker_service( wanted_by="multi-user.target", ) - return create_systemd_service_file(filename, local_folder, service_string) + services_folder = pathops.LocalPath(home_dir, "services") + return create_systemd_service_file(filename, services_folder.as_posix(), service_string) def setup_poller_service( - local_folder: str, + home_dir: str, user: str, group: str, denylist: str, @@ -184,7 +185,7 @@ def setup_poller_service( """Set up poller systemd service file. Args: - local_folder: The local folder to store the service in. + home_dir: The home directory of the user. user: The user to run the service as. group: The permissions group to run the service as. denylist: The location of the package denylist. @@ -219,7 +220,8 @@ def setup_poller_service( wanted_by="multi-user.target", ) - return create_systemd_service_file(filename, local_folder, service_string) + services_folder = pathops.LocalPath(home_dir, "services") + return create_systemd_service_file(filename, services_folder.as_posix(), service_string) def setup_worker_service( diff --git a/src/importer_node.py b/src/importer_node.py index 4e955da..dcf4122 100644 --- a/src/importer_node.py +++ b/src/importer_node.py @@ -71,11 +71,9 @@ def setup_primary_node( Returns: True if installation succeeded, False otherwise. """ - services_folder = pathops.LocalPath(git_ubuntu_user_home, "services") - # Setup broker service. if not git_ubuntu.setup_broker_service( - services_folder.as_posix(), + git_ubuntu_user_home, system_user, system_user, primary_port, @@ -90,7 +88,7 @@ def setup_primary_node( # Setup poller service. if not git_ubuntu.setup_poller_service( - services_folder.as_posix(), + git_ubuntu_user_home, system_user, system_user, denylist.as_posix(), From 891af908a4d05410f35158ae8e86d8f3d825da12 Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Fri, 12 Dec 2025 19:28:01 -0500 Subject: [PATCH 03/19] Expand worker services as units of template service --- src/charm.py | 2 +- src/git_ubuntu.py | 36 +++++++++++++++++++++++++++++++++++- src/importer_node.py | 6 ++++-- 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/charm.py b/src/charm.py index 92da1db..e675012 100755 --- a/src/charm.py +++ b/src/charm.py @@ -333,7 +333,7 @@ def _refresh_importer_node(self) -> None: def _start_services(self) -> None: """Start the services and note the result through status.""" - if node.start(GIT_UBUNTU_USER_HOME_DIR): + if node.start(GIT_UBUNTU_USER_HOME_DIR, self._node_id, self._num_workers): node_type_str = "primary" if self._is_primary else "secondary" self.unit.status = ops.ActiveStatus( f"Running git-ubuntu importer {node_type_str} node." diff --git a/src/git_ubuntu.py b/src/git_ubuntu.py index 92d0654..ed7faea 100644 --- a/src/git_ubuntu.py +++ b/src/git_ubuntu.py @@ -53,6 +53,34 @@ def _get_services_list(service_folder: str) -> list[str] | None: return service_list +def _expand_service_list_for_workers( + base_service_list: list[str], + node_id: int, + num_workers: int, +) -> list[str]: + """Expand the base service list to include worker instances. + + Args: + base_service_list: The base list of service names. + node_id: The unique ID of this node. + num_workers: The number of worker instances to set up. + + Returns: + The expanded list of service names including worker instances. + """ + expanded_service_list = [] + + for service in base_service_list: + if "@.service" in service: + for worker_id in range(num_workers): + service_name = service.replace("@.service", f"@{node_id}-{worker_id}") + expanded_service_list.append(service_name) + else: + expanded_service_list.append(service) + + return expanded_service_list + + def generate_systemd_service_string( description: str, service_user: str, @@ -283,7 +311,7 @@ def setup_worker_service( return create_systemd_service_file(filename, services_folder.as_posix(), service_string) -def start_services(service_folder: str) -> bool: +def start_services(service_folder: str, node_id: int, num_workers: int) -> bool: """Start all git-ubuntu services and wait for startup to complete. Args: @@ -297,6 +325,8 @@ def start_services(service_folder: str) -> bool: if service_list is None: return False + service_list = _expand_service_list_for_workers(service_list, node_id, num_workers) + services_started = True # Start services @@ -338,6 +368,10 @@ def stop_services(service_folder: str) -> bool: services_stopped = True for service in service_list: + # Glob worker services + if "@.service" in service: + service = f"'{service.replace('@.service', '@*')}'" + if stop_service(service): logger.info("Stopped service %s", service) else: diff --git a/src/importer_node.py b/src/importer_node.py index dcf4122..4e9a815 100644 --- a/src/importer_node.py +++ b/src/importer_node.py @@ -101,18 +101,20 @@ def setup_primary_node( return True -def start(git_ubuntu_user_home: str) -> bool: +def start(git_ubuntu_user_home: str, node_id: int, num_workers: int) -> bool: """Start all git-ubuntu services and wait for their startups to complete. Args: git_ubuntu_user_home: The home directory of the git-ubuntu user. + node_id: The node ID of this node. + num_workers: The number of worker services to start if secondary. Returns: True if all services were started successfully, False otherwise. """ services_folder = pathops.LocalPath(git_ubuntu_user_home, "services") - if not git_ubuntu.start_services(services_folder.as_posix()): + if not git_ubuntu.start_services(services_folder.as_posix(), node_id, num_workers): logger.error("Failed to start all services.") return False From 02fa4e55cfd4c7ac0b6646b4d1cc52bf1625f258 Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Fri, 12 Dec 2025 19:32:19 -0500 Subject: [PATCH 04/19] Update integration test with new worker style --- tests/integration/test_charm.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 8689810..635d148 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -70,14 +70,14 @@ def get_services_dict(unit_name: str, juju: jubilant.Juju) -> dict[str, dict[str else: node_id = int(unit_name.split("/")[-1]) - assert services[f"git-ubuntu-importer-service-worker{node_id}_0.service"]["active"] + assert services[f"git-ubuntu-importer-service-worker@{node_id}-0.service"]["active"] assert ( - services[f"git-ubuntu-importer-service-worker{node_id}_0.service"]["description"] + services[f"git-ubuntu-importer-service-worker@{node_id}-0.service"]["description"] == "git-ubuntu importer service worker" ) - assert services[f"git-ubuntu-importer-service-worker{node_id}_1.service"]["active"] + assert services[f"git-ubuntu-importer-service-worker@{node_id}-1.service"]["active"] assert ( - services[f"git-ubuntu-importer-service-worker{node_id}_1.service"]["description"] + services[f"git-ubuntu-importer-service-worker@{node_id}-1.service"]["description"] == "git-ubuntu importer service worker" ) From 8f337b4af7d985f7ea767bc5dd0e72d26b18eb1a Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Fri, 12 Dec 2025 19:43:41 -0500 Subject: [PATCH 05/19] Fix unit tests and lint --- src/git_ubuntu.py | 2 ++ tests/unit/test_git_ubuntu.py | 26 ++++++++++++-------------- tests/unit/test_importer_node.py | 12 ++++++------ 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/git_ubuntu.py b/src/git_ubuntu.py index ed7faea..d04031d 100644 --- a/src/git_ubuntu.py +++ b/src/git_ubuntu.py @@ -316,6 +316,8 @@ def start_services(service_folder: str, node_id: int, num_workers: int) -> bool: Args: service_folder: The name of the folder containing the service files. + node_id: The unique ID of this node. + num_workers: The number of worker instances to start if secondary. Returns: True if all services were started successfully, False otherwise. diff --git a/tests/unit/test_git_ubuntu.py b/tests/unit/test_git_ubuntu.py index 2b700d8..78992e8 100644 --- a/tests/unit/test_git_ubuntu.py +++ b/tests/unit/test_git_ubuntu.py @@ -478,7 +478,7 @@ def test_git_ubuntu_broker_setup_success(mock_create_file): """Test GitUbuntuBroker setup with default parameters.""" mock_create_file.return_value = True - result = setup_broker_service("test_folder", "ubuntu", "testgroup") + result = setup_broker_service("test_home", "ubuntu", "testgroup") assert result is True mock_create_file.assert_called_once() @@ -489,7 +489,7 @@ def test_git_ubuntu_broker_setup_custom_port(mock_create_file): """Test GitUbuntuBroker setup with custom broker port.""" mock_create_file.return_value = True - result = setup_broker_service("test_folder", "ubuntu", "testgroup", broker_port=8080) + result = setup_broker_service("test_home", "ubuntu", "testgroup", broker_port=8080) assert result is True mock_create_file.assert_called_once() @@ -500,7 +500,7 @@ def test_git_ubuntu_broker_setup_failure(mock_create_file): """Test GitUbuntuBroker setup when file creation fails.""" mock_create_file.return_value = False - result = setup_broker_service("test_folder", "ubuntu", "testgroup") + result = setup_broker_service("test_home", "ubuntu", "testgroup") assert result is False @@ -510,7 +510,7 @@ def test_git_ubuntu_poller_setup_success(mock_create_file): """Test GitUbuntuPoller setup with default parameters.""" mock_create_file.return_value = True - result = setup_poller_service("test_folder", "ubuntu", "testgroup", "denylist.txt") + result = setup_poller_service("test_home", "ubuntu", "testgroup", "denylist.txt") assert result is True mock_create_file.assert_called_once() @@ -522,7 +522,7 @@ def test_git_ubuntu_poller_setup_with_http_proxy(mock_create_file): mock_create_file.return_value = True result = setup_poller_service( - "test_folder", + "test_home", "ubuntu", "testgroup", "denylist.txt", @@ -539,7 +539,7 @@ def test_git_ubuntu_poller_setup_with_https_proxy(mock_create_file): mock_create_file.return_value = True result = setup_poller_service( - "test_folder", + "test_home", "ubuntu", "testgroup", "denylist.txt", @@ -556,7 +556,7 @@ def test_git_ubuntu_poller_setup_with_full_proxy(mock_create_file): mock_create_file.return_value = True result = setup_poller_service( - "test_folder", + "test_home", "ubuntu", "testgroup", "denylist.txt", @@ -573,7 +573,7 @@ def test_git_ubuntu_poller_setup_failure(mock_create_file): """Test GitUbuntuPoller setup when file creation fails.""" mock_create_file.return_value = False - result = setup_poller_service("test_folder", "ubuntu", "testgroup", "denylist.txt") + result = setup_poller_service("test_home", "ubuntu", "testgroup", "denylist.txt") assert result is False @@ -583,7 +583,7 @@ def test_git_ubuntu_worker_setup_success(mock_create_file): """Test GitUbuntuWorker setup with default parameters.""" mock_create_file.return_value = True - result = setup_worker_service("test_folder", "ubuntu", "testgroup") + result = setup_worker_service("test_home", "ubuntu", "testgroup") assert result is True mock_create_file.assert_called_once() @@ -595,10 +595,9 @@ def test_git_ubuntu_worker_setup_custom_params(mock_create_file): mock_create_file.return_value = True result = setup_worker_service( - "test_folder", + "test_home", "ubuntu", "testgroup", - worker_name="1_2", broker_ip="192.168.1.100", broker_port=9000, ) @@ -613,10 +612,9 @@ def test_git_ubuntu_worker_setup_https_proxy(mock_create_file): mock_create_file.return_value = True result = setup_worker_service( - "test_folder", + "test_home", "ubuntu", "testgroup", - worker_name="1_2", broker_ip="192.168.1.100", broker_port=9000, https_proxy="http://proxy.example.com:8080", @@ -631,6 +629,6 @@ def test_git_ubuntu_worker_setup_failure(mock_create_file): """Test GitUbuntuWorker setup when file creation fails.""" mock_create_file.return_value = False - result = setup_worker_service("test_folder", "ubuntu", "testgroup") + result = setup_worker_service("test_home", "ubuntu", "testgroup") assert result is False diff --git a/tests/unit/test_importer_node.py b/tests/unit/test_importer_node.py index ee39618..f1c2bc5 100644 --- a/tests/unit/test_importer_node.py +++ b/tests/unit/test_importer_node.py @@ -16,11 +16,11 @@ def test_setup_secondary_node_success(mock_setup_worker): mock_setup_worker.return_value = True result = importer_node.setup_secondary_node( - "/var/local/git-ubuntu", 1, 2, "git-ubuntu", True, 1692, "192.168.1.1" + "/var/local/git-ubuntu", "git-ubuntu", True, 1692, "192.168.1.1" ) assert result is True - assert mock_setup_worker.call_count == 2 + assert mock_setup_worker.call_count == 1 @patch("importer_node.git_ubuntu.setup_worker_service") @@ -29,7 +29,7 @@ def test_setup_secondary_node_failure(mock_setup_worker): mock_setup_worker.return_value = False result = importer_node.setup_secondary_node( - "/var/local/git-ubuntu", 1, 1, "git-ubuntu", True, 1692, "192.168.1.1" + "/var/local/git-ubuntu", "git-ubuntu", True, 1692, "192.168.1.1" ) assert result is False @@ -90,10 +90,10 @@ def test_start_success(mock_start_services): """Test successful service start.""" mock_start_services.return_value = True - result = importer_node.start("/var/local/git-ubuntu") + result = importer_node.start("/var/local/git-ubuntu", 1, 2) assert result is True - mock_start_services.assert_called_once_with("/var/local/git-ubuntu/services") + mock_start_services.assert_called_once_with("/var/local/git-ubuntu/services", 1, 2) @patch("importer_node.git_ubuntu.start_services") @@ -101,7 +101,7 @@ def test_start_failure(mock_start_services): """Test service start failure.""" mock_start_services.return_value = False - result = importer_node.start("/var/local/git-ubuntu") + result = importer_node.start("/var/local/git-ubuntu", 1, 2) assert result is False From d5bd2f97066f6e65b56e0a6911a9e9a6fb9d05d1 Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Mon, 15 Dec 2025 08:16:17 -0500 Subject: [PATCH 06/19] Use subprocess instead of system --- src/user_management.py | 40 ++++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/src/user_management.py b/src/user_management.py index 4887e84..f11b1d8 100644 --- a/src/user_management.py +++ b/src/user_management.py @@ -5,7 +5,7 @@ """Machine user management functions.""" import logging -from os import system +import subprocess from charmlibs import pathops from charms.operator_libs_linux.v0 import passwd @@ -23,10 +23,29 @@ def _run_command_as_user(user: str, command: str) -> bool: Returns: True if the command was run successfully, False otherwise. """ - command_result = system(f'su - {user} -s /bin/bash -c "{command}"') - if command_result != 0: - logger.error("Command %s exited with result %d.", command, command_result) + sudo_command = ["sudo", "-u", user, "--", "/bin/bash", "-c", f'"{command}"'] + + try: + result = subprocess.run( + sudo_command, + capture_output=True, + text=True, + check=False, + ) + except OSError: + logger.exception("Failed to execute command %s as user %s", command, user) + return False + + if result.returncode != 0: + logger.error( + "Command %s exited with result %d - stdout: %s - stderr: %s", + command, + result.returncode, + result.stdout, + result.stderr, + ) return False + return True @@ -290,10 +309,19 @@ def set_snap_homedirs(home_dir: str) -> bool: True if the homedirs update succeeded, False otherwise. """ homedirs_entry = pathops.LocalPath(home_dir).parent.as_posix() - command_result = system(f"snap set system homedirs={homedirs_entry}") - if command_result != 0: + + try: + command_result = subprocess.run( + ["snap", "set", "system", f"homedirs={homedirs_entry}"], check=False + ) + except OSError: + logger.exception("Failed to execute snap homedir setting command.") + return False + + if command_result.returncode != 0: logger.error("snap homedir setting exited with result %d.", command_result) return False + return True From 87c4f3fd92618f69c22e1a86f837f304a57f08de Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Mon, 15 Dec 2025 08:16:40 -0500 Subject: [PATCH 07/19] Confirm git-ubuntu source is cloned in integration --- tests/integration/test_charm.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 635d148..fe6ffa6 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -137,6 +137,25 @@ def test_installed_dump_files(app: str, juju: jubilant.Juju): assert debian_keyring_status == "0" +def test_git_ubuntu_source_denylist_exists(app: str, juju: jubilant.Juju): + """Test that the git-ubuntu source repo has been cloned and contains the denylist. + + Args: + app: The app in charge of this unit. + juju: The juju model in charge of the app. + """ + juju.wait(jubilant.all_active) + + debian_keyring_status = juju.ssh( + f"{app}/leader", + "test -f " + + "/var/local/git-ubuntu/live-allowlist-denylist-source/gitubuntu/" + + "source-package-denylist.txt | echo $?", + "", + ).strip() + assert debian_keyring_status == "0" + + def test_controller_port_open(app: str, juju: jubilant.Juju): """Confirm that the git-ubuntu controller leader network port opens. From 2bf64ecb2210ab043a974a1e985e794ad5c91ac0 Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Mon, 15 Dec 2025 08:51:01 -0500 Subject: [PATCH 08/19] Test service status last --- tests/integration/test_charm.py | 134 ++++++++++++++++---------------- 1 file changed, 67 insertions(+), 67 deletions(-) diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index fe6ffa6..3d0394d 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -15,73 +15,6 @@ logger = logging.getLogger(__name__) -def test_service_status(app: str, juju: jubilant.Juju): - """Test necessary files exist after startup. - - Args: - app: The app in charge of this unit. - juju: The juju model in charge of the app. - """ - # Wait until machine is ready, then wait an extra 60 seconds for services to fully activate. - juju.wait(jubilant.all_active) - sleep(90) - - def get_services_dict(unit_name: str, juju: jubilant.Juju) -> dict[str, dict[str, bool | str]]: - """Get a dictionary of running systemd services on the app's unit. - - Args: - unit_name: The name of the unit to check. - juju: The juju model in charge of the app. - - Returns: - A dict mapping unit name to if the service is active and its description. - """ - service_output = juju.ssh( - unit_name, - "systemctl list-units --type service --full --all --output json --no-pager | cat -v", - "", - ) - service_json = json.loads(service_output) - service_dict = dict() - - for service_entry in service_json: - service_dict[service_entry["unit"]] = { - "active": service_entry["active"] == "active", - "description": service_entry["description"], - } - - return service_dict - - for unit_name, unit in juju.status().get_units(app).items(): - services = get_services_dict(unit_name, juju) - - if unit.leader: - assert services["git-ubuntu-importer-service-broker.service"]["active"] - assert ( - services["git-ubuntu-importer-service-broker.service"]["description"] - == "git-ubuntu importer service broker" - ) - assert services["git-ubuntu-importer-service-poller.service"]["active"] - assert ( - services["git-ubuntu-importer-service-poller.service"]["description"] - == "git-ubuntu importer service poller" - ) - - else: - node_id = int(unit_name.split("/")[-1]) - - assert services[f"git-ubuntu-importer-service-worker@{node_id}-0.service"]["active"] - assert ( - services[f"git-ubuntu-importer-service-worker@{node_id}-0.service"]["description"] - == "git-ubuntu importer service worker" - ) - assert services[f"git-ubuntu-importer-service-worker@{node_id}-1.service"]["active"] - assert ( - services[f"git-ubuntu-importer-service-worker@{node_id}-1.service"]["description"] - == "git-ubuntu importer service worker" - ) - - def test_installed_apps(app: str, juju: jubilant.Juju): """Test that all required applications are installed. @@ -187,3 +120,70 @@ def is_port_open(host: str, port: int) -> bool: assert address is not None assert is_port_open(address, 1692) + + +def test_service_status(app: str, juju: jubilant.Juju): + """Test necessary files exist after startup. + + Args: + app: The app in charge of this unit. + juju: The juju model in charge of the app. + """ + # Wait until machine is ready, then wait an extra 60 seconds for services to fully activate. + juju.wait(jubilant.all_active) + sleep(60) + + def get_services_dict(unit_name: str, juju: jubilant.Juju) -> dict[str, dict[str, bool | str]]: + """Get a dictionary of running systemd services on the app's unit. + + Args: + unit_name: The name of the unit to check. + juju: The juju model in charge of the app. + + Returns: + A dict mapping unit name to if the service is active and its description. + """ + service_output = juju.ssh( + unit_name, + "systemctl list-units --type service --full --all --output json --no-pager | cat -v", + "", + ) + service_json = json.loads(service_output) + service_dict = dict() + + for service_entry in service_json: + service_dict[service_entry["unit"]] = { + "active": service_entry["active"] == "active", + "description": service_entry["description"], + } + + return service_dict + + for unit_name, unit in juju.status().get_units(app).items(): + services = get_services_dict(unit_name, juju) + + if unit.leader: + assert services["git-ubuntu-importer-service-broker.service"]["active"] + assert ( + services["git-ubuntu-importer-service-broker.service"]["description"] + == "git-ubuntu importer service broker" + ) + assert services["git-ubuntu-importer-service-poller.service"]["active"] + assert ( + services["git-ubuntu-importer-service-poller.service"]["description"] + == "git-ubuntu importer service poller" + ) + + else: + node_id = int(unit_name.split("/")[-1]) + + assert services[f"git-ubuntu-importer-service-worker@{node_id}-0.service"]["active"] + assert ( + services[f"git-ubuntu-importer-service-worker@{node_id}-0.service"]["description"] + == "git-ubuntu importer service worker" + ) + assert services[f"git-ubuntu-importer-service-worker@{node_id}-1.service"]["active"] + assert ( + services[f"git-ubuntu-importer-service-worker@{node_id}-1.service"]["description"] + == "git-ubuntu importer service worker" + ) From d8ee696cc7ca123efdd25ad1d102d48b3585a6e9 Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Mon, 15 Dec 2025 08:55:38 -0500 Subject: [PATCH 09/19] Add w to new worker service name to match ps5 style --- src/git_ubuntu.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/git_ubuntu.py b/src/git_ubuntu.py index d04031d..cc83b8f 100644 --- a/src/git_ubuntu.py +++ b/src/git_ubuntu.py @@ -73,7 +73,7 @@ def _expand_service_list_for_workers( for service in base_service_list: if "@.service" in service: for worker_id in range(num_workers): - service_name = service.replace("@.service", f"@{node_id}-{worker_id}") + service_name = service.replace("@.service", f"@w{node_id}-{worker_id}") expanded_service_list.append(service_name) else: expanded_service_list.append(service) From b9dcf44c26cb412f2dcc86be3f804a81810f1fe7 Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Mon, 15 Dec 2025 09:18:27 -0500 Subject: [PATCH 10/19] Use w in tests and remove sleep --- tests/integration/test_charm.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 3d0394d..dcea522 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -8,7 +8,6 @@ import json import logging import socket -from time import sleep import jubilant @@ -131,7 +130,6 @@ def test_service_status(app: str, juju: jubilant.Juju): """ # Wait until machine is ready, then wait an extra 60 seconds for services to fully activate. juju.wait(jubilant.all_active) - sleep(60) def get_services_dict(unit_name: str, juju: jubilant.Juju) -> dict[str, dict[str, bool | str]]: """Get a dictionary of running systemd services on the app's unit. @@ -177,13 +175,13 @@ def get_services_dict(unit_name: str, juju: jubilant.Juju) -> dict[str, dict[str else: node_id = int(unit_name.split("/")[-1]) - assert services[f"git-ubuntu-importer-service-worker@{node_id}-0.service"]["active"] + assert services[f"git-ubuntu-importer-service-worker@w{node_id}-0.service"]["active"] assert ( - services[f"git-ubuntu-importer-service-worker@{node_id}-0.service"]["description"] + services[f"git-ubuntu-importer-service-worker@w{node_id}-0.service"]["description"] == "git-ubuntu importer service worker" ) - assert services[f"git-ubuntu-importer-service-worker@{node_id}-1.service"]["active"] + assert services[f"git-ubuntu-importer-service-worker@w{node_id}-1.service"]["active"] assert ( - services[f"git-ubuntu-importer-service-worker@{node_id}-1.service"]["description"] + services[f"git-ubuntu-importer-service-worker@w{node_id}-1.service"]["description"] == "git-ubuntu importer service worker" ) From cfe27f0a331e88bef0abc379dec17ef905ad9b87 Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Mon, 15 Dec 2025 09:49:29 -0500 Subject: [PATCH 11/19] Swap back to su --- src/user_management.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/user_management.py b/src/user_management.py index f11b1d8..81c374f 100644 --- a/src/user_management.py +++ b/src/user_management.py @@ -23,11 +23,11 @@ def _run_command_as_user(user: str, command: str) -> bool: Returns: True if the command was run successfully, False otherwise. """ - sudo_command = ["sudo", "-u", user, "--", "/bin/bash", "-c", f'"{command}"'] + su_command = ["su", "-", user, "-s", "/bin/bash", "-c", f'"{command}"'] try: result = subprocess.run( - sudo_command, + su_command, capture_output=True, text=True, check=False, From 64421e6d4beefa4e9258f66569a03231d83c8187 Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Mon, 15 Dec 2025 10:14:29 -0500 Subject: [PATCH 12/19] Move integration test sub-functions to helpers file --- tests/integration/helpers.py | 72 +++++++++++++++++++++++++++++++++ tests/integration/test_charm.py | 60 +-------------------------- 2 files changed, 73 insertions(+), 59 deletions(-) create mode 100644 tests/integration/helpers.py diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py new file mode 100644 index 0000000..667085f --- /dev/null +++ b/tests/integration/helpers.py @@ -0,0 +1,72 @@ +#!/usr/bin/env python3 +# Copyright 2025 Canonical Ltd. +# See LICENSE file for licensing details. + + +"""Integration test utility functions.""" + +import json +import socket + +import jubilant + + +def check_deb_installed(app: str, unit: int, juju: jubilant.Juju, package_name: str) -> bool: + """Check if a deb pkg is installed on a specific unit. + + Args: + app: The app in charge of this unit. + unit: The unit number to check. + juju: The juju model in charge of the app. + package_name: The name of the deb package. + + Returns: + True if the package is installed, False otherwise. + """ + install_status = juju.ssh( + f"{app}/{unit}", f"dpkg-query --show --showformat='${{Status}}' {package_name}" + ) + return "installed" in install_status + + +def get_services_dict(unit_name: str, juju: jubilant.Juju) -> dict[str, dict[str, bool | str]]: + """Get a dictionary of running systemd services on the app's unit. + + Args: + unit_name: The name of the unit to check. + juju: The juju model in charge of the app. + + Returns: + A dict mapping unit name to if the service is active and its description. + """ + service_output = juju.ssh( + unit_name, + "systemctl list-units --type service --full --all --output json --no-pager | cat -v", + "", + ) + service_json = json.loads(service_output) + service_dict = dict() + + for service_entry in service_json: + service_dict[service_entry["unit"]] = { + "active": service_entry["active"] == "active", + "description": service_entry["description"], + } + + return service_dict + + +def is_port_open(host: str, port: int) -> bool: + """Check if a port is opened in a particular host. + + Args: + host: The host network location. + port: The port number to check. + + Returns: True if the port is open, False otherwise. + """ + try: + with socket.create_connection((host, port), timeout=5): + return True + except (ConnectionRefusedError, TimeoutError): + return False diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index dcea522..cb3fd18 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -7,9 +7,9 @@ import json import logging -import socket import jubilant +from helpers import check_deb_installed, get_services_dict, is_port_open logger = logging.getLogger(__name__) @@ -23,23 +23,6 @@ def test_installed_apps(app: str, juju: jubilant.Juju): """ juju.wait(jubilant.all_active) - def check_deb_installed(app: str, unit: int, juju: jubilant.Juju, package_name: str) -> bool: - """Check if a deb pkg is installed on a specific unit. - - Args: - app: The app in charge of this unit. - unit: The unit number to check. - juju: The juju model in charge of the app. - package_name: The name of the deb package. - - Returns: - True if the package is installed, False otherwise. - """ - install_status = juju.ssh( - f"{app}/{unit}", f"dpkg-query --show --showformat='${{Status}}' {package_name}" - ) - return "installed" in install_status - assert check_deb_installed(app, 0, juju, "git") assert check_deb_installed(app, 0, juju, "sqlite3") assert check_deb_installed(app, 1, juju, "git") @@ -97,21 +80,6 @@ def test_controller_port_open(app: str, juju: jubilant.Juju): """ juju.wait(jubilant.all_active) - def is_port_open(host: str, port: int) -> bool: - """Check if a port is opened in a particular host. - - Args: - host: The host network location. - port: The port number to check. - - Returns: True if the port is open, False otherwise. - """ - try: - with socket.create_connection((host, port), timeout=5): - return True - except (ConnectionRefusedError, TimeoutError): - return False - address = None for unit in juju.status().get_units(app).values(): if unit.leader: @@ -131,32 +99,6 @@ def test_service_status(app: str, juju: jubilant.Juju): # Wait until machine is ready, then wait an extra 60 seconds for services to fully activate. juju.wait(jubilant.all_active) - def get_services_dict(unit_name: str, juju: jubilant.Juju) -> dict[str, dict[str, bool | str]]: - """Get a dictionary of running systemd services on the app's unit. - - Args: - unit_name: The name of the unit to check. - juju: The juju model in charge of the app. - - Returns: - A dict mapping unit name to if the service is active and its description. - """ - service_output = juju.ssh( - unit_name, - "systemctl list-units --type service --full --all --output json --no-pager | cat -v", - "", - ) - service_json = json.loads(service_output) - service_dict = dict() - - for service_entry in service_json: - service_dict[service_entry["unit"]] = { - "active": service_entry["active"] == "active", - "description": service_entry["description"], - } - - return service_dict - for unit_name, unit in juju.status().get_units(app).items(): services = get_services_dict(unit_name, juju) From 8700f96eea9c6aa884d0af752a01114883944c63 Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Mon, 15 Dec 2025 11:24:01 -0500 Subject: [PATCH 13/19] Use shell=True in subprocess --- src/user_management.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/user_management.py b/src/user_management.py index 81c374f..1d8308a 100644 --- a/src/user_management.py +++ b/src/user_management.py @@ -31,6 +31,7 @@ def _run_command_as_user(user: str, command: str) -> bool: capture_output=True, text=True, check=False, + shell=True, ) except OSError: logger.exception("Failed to execute command %s as user %s", command, user) From 8bcac2c5c9b2a5beba5845407f338e7fc2c6b0e0 Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Mon, 15 Dec 2025 12:37:11 -0500 Subject: [PATCH 14/19] Remove json import --- tests/integration/test_charm.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index cb3fd18..c88e611 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -5,7 +5,6 @@ """Integration tests.""" -import json import logging import jubilant From 43398cd98f0a3396b4b339f099f8259e2fbf8fac Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Mon, 15 Dec 2025 12:48:18 -0500 Subject: [PATCH 15/19] Properly wait for all units to be running --- tests/integration/helpers.py | 16 ++++++++++++++++ tests/integration/test_charm.py | 17 +++++++++++------ 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index 667085f..c2db4b4 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -70,3 +70,19 @@ def is_port_open(host: str, port: int) -> bool: return True except (ConnectionRefusedError, TimeoutError): return False + + +def wait_for_all_units_running(app: str, juju: jubilant.Juju) -> None: + """Wait until all units of an application are active and contain the correct running message. + + Args: + app: The application name. + juju: The juju model in charge of the app. + """ + juju.wait_for( + lambda status: all( + unit_status.is_active + and "Running git-ubuntu importer" in unit_status.juju_status.message + for unit_status in status.apps[app].app_status.units.values() + ) + ) diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index c88e611..c41bef6 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -8,7 +8,12 @@ import logging import jubilant -from helpers import check_deb_installed, get_services_dict, is_port_open +from helpers import ( + check_deb_installed, + get_services_dict, + is_port_open, + wait_for_all_units_running, +) logger = logging.getLogger(__name__) @@ -20,7 +25,7 @@ def test_installed_apps(app: str, juju: jubilant.Juju): app: The app in charge of this unit. juju: The juju model in charge of the app. """ - juju.wait(jubilant.all_active) + wait_for_all_units_running(juju, app) assert check_deb_installed(app, 0, juju, "git") assert check_deb_installed(app, 0, juju, "sqlite3") @@ -43,7 +48,7 @@ def test_installed_dump_files(app: str, juju: jubilant.Juju): app: The app in charge of this unit. juju: The juju model in charge of the app. """ - juju.wait(jubilant.all_active) + wait_for_all_units_running(juju, app) debian_keyring_status = juju.ssh( f"{app}/leader", "test -f /etc/git-ubuntu/debian-archive-keyring.gpg | echo $?", "" @@ -58,7 +63,7 @@ def test_git_ubuntu_source_denylist_exists(app: str, juju: jubilant.Juju): app: The app in charge of this unit. juju: The juju model in charge of the app. """ - juju.wait(jubilant.all_active) + wait_for_all_units_running(juju, app) debian_keyring_status = juju.ssh( f"{app}/leader", @@ -77,7 +82,7 @@ def test_controller_port_open(app: str, juju: jubilant.Juju): app: The app in charge of this unit. juju: The juju model in charge of the app. """ - juju.wait(jubilant.all_active) + wait_for_all_units_running(juju, app) address = None for unit in juju.status().get_units(app).values(): @@ -96,7 +101,7 @@ def test_service_status(app: str, juju: jubilant.Juju): juju: The juju model in charge of the app. """ # Wait until machine is ready, then wait an extra 60 seconds for services to fully activate. - juju.wait(jubilant.all_active) + wait_for_all_units_running(juju, app) for unit_name, unit in juju.status().get_units(app).items(): services = get_services_dict(unit_name, juju) From 11fb4b7f53685ff79be1838c8024c91ddc7d3c9d Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Mon, 15 Dec 2025 15:46:49 -0500 Subject: [PATCH 16/19] Use user arg for subprocess --- src/user_management.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/user_management.py b/src/user_management.py index 1d8308a..d4948e2 100644 --- a/src/user_management.py +++ b/src/user_management.py @@ -23,11 +23,10 @@ def _run_command_as_user(user: str, command: str) -> bool: Returns: True if the command was run successfully, False otherwise. """ - su_command = ["su", "-", user, "-s", "/bin/bash", "-c", f'"{command}"'] - try: result = subprocess.run( - su_command, + command, + user=user, capture_output=True, text=True, check=False, From 65c89b814ce80b25ee775a780375a62d128b2d5f Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Mon, 15 Dec 2025 16:06:11 -0500 Subject: [PATCH 17/19] Fix integration wait --- tests/integration/helpers.py | 4 ++-- tests/integration/test_charm.py | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index c2db4b4..15bb2e6 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -79,10 +79,10 @@ def wait_for_all_units_running(app: str, juju: jubilant.Juju) -> None: app: The application name. juju: The juju model in charge of the app. """ - juju.wait_for( + juju.wait( lambda status: all( unit_status.is_active and "Running git-ubuntu importer" in unit_status.juju_status.message - for unit_status in status.apps[app].app_status.units.values() + for unit_status in status.apps[app].units.values() ) ) diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index c41bef6..bfa35e2 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -25,7 +25,7 @@ def test_installed_apps(app: str, juju: jubilant.Juju): app: The app in charge of this unit. juju: The juju model in charge of the app. """ - wait_for_all_units_running(juju, app) + wait_for_all_units_running(app, juju) assert check_deb_installed(app, 0, juju, "git") assert check_deb_installed(app, 0, juju, "sqlite3") @@ -48,7 +48,7 @@ def test_installed_dump_files(app: str, juju: jubilant.Juju): app: The app in charge of this unit. juju: The juju model in charge of the app. """ - wait_for_all_units_running(juju, app) + wait_for_all_units_running(app, juju) debian_keyring_status = juju.ssh( f"{app}/leader", "test -f /etc/git-ubuntu/debian-archive-keyring.gpg | echo $?", "" @@ -63,7 +63,7 @@ def test_git_ubuntu_source_denylist_exists(app: str, juju: jubilant.Juju): app: The app in charge of this unit. juju: The juju model in charge of the app. """ - wait_for_all_units_running(juju, app) + wait_for_all_units_running(app, juju) debian_keyring_status = juju.ssh( f"{app}/leader", @@ -82,7 +82,7 @@ def test_controller_port_open(app: str, juju: jubilant.Juju): app: The app in charge of this unit. juju: The juju model in charge of the app. """ - wait_for_all_units_running(juju, app) + wait_for_all_units_running(app, juju) address = None for unit in juju.status().get_units(app).values(): @@ -101,7 +101,7 @@ def test_service_status(app: str, juju: jubilant.Juju): juju: The juju model in charge of the app. """ # Wait until machine is ready, then wait an extra 60 seconds for services to fully activate. - wait_for_all_units_running(juju, app) + wait_for_all_units_running(app, juju) for unit_name, unit in juju.status().get_units(app).items(): services = get_services_dict(unit_name, juju) From b838a501e5e586ee2e5d85a5a4d60d3c30692e60 Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Mon, 15 Dec 2025 16:58:46 -0500 Subject: [PATCH 18/19] Include home directory as env variable --- src/charm.py | 10 +++++--- src/user_management.py | 41 ++++++++++++++++++++---------- tests/unit/test_user_management.py | 24 ++++++++--------- 3 files changed, 46 insertions(+), 29 deletions(-) diff --git a/src/charm.py b/src/charm.py index e675012..11935ba 100755 --- a/src/charm.py +++ b/src/charm.py @@ -350,8 +350,10 @@ def _update_git_user_config(self) -> bool: self.unit.status = ops.MaintenanceStatus("Updating git config for git-ubuntu user.") if not usr.update_git_user_name( - GIT_UBUNTU_SYSTEM_USER_USERNAME, GIT_UBUNTU_GIT_USER_NAME - ) or not usr.update_git_email(GIT_UBUNTU_SYSTEM_USER_USERNAME, GIT_UBUNTU_GIT_EMAIL): + GIT_UBUNTU_SYSTEM_USER_USERNAME, GIT_UBUNTU_GIT_USER_NAME, GIT_UBUNTU_USER_HOME_DIR + ) or not usr.update_git_email( + GIT_UBUNTU_SYSTEM_USER_USERNAME, GIT_UBUNTU_GIT_EMAIL, GIT_UBUNTU_USER_HOME_DIR + ): self.unit.status = ops.BlockedStatus("Failed to set git user config.") return False return True @@ -361,7 +363,9 @@ def _update_lpuser_config(self) -> bool: self.unit.status = ops.MaintenanceStatus("Updating lpuser entry for git-ubuntu user.") lpuser = self._lp_username if lp.is_valid_lp_username(lpuser): - if not usr.update_git_ubuntu_lpuser(GIT_UBUNTU_SYSTEM_USER_USERNAME, lpuser): + if not usr.update_git_ubuntu_lpuser( + GIT_UBUNTU_SYSTEM_USER_USERNAME, lpuser, GIT_UBUNTU_USER_HOME_DIR + ): self.unit.status = ops.BlockedStatus("Failed to update lpuser config.") return False else: diff --git a/src/user_management.py b/src/user_management.py index d4948e2..dbe03dd 100644 --- a/src/user_management.py +++ b/src/user_management.py @@ -13,12 +13,13 @@ logger = logging.getLogger(__name__) -def _run_command_as_user(user: str, command: str) -> bool: +def _run_command_as_user(user: str, command: str, env: dict[str, str] | None = None) -> bool: """Run a command as a user. Args: user: The user to run the command as. command: The command to run. + env: Dictionary of environment variables to set for the command. Returns: True if the command was run successfully, False otherwise. @@ -27,6 +28,7 @@ def _run_command_as_user(user: str, command: str) -> bool: result = subprocess.run( command, user=user, + env=env, capture_output=True, text=True, check=False, @@ -138,29 +140,31 @@ def refresh_git_ubuntu_source( # Update origin to the current source url if not _run_command_as_user( - user, f"git -C {clone_dir.as_posix()} remote set-url origin {source_url}" + user, + f"git -C {clone_dir.as_posix()} remote set-url origin {source_url}", + {"HOME": home_dir}, ): logger.error("Failed to update git-ubuntu source origin.") return False # Run git pull to get up to date - pull_command = f"git -C {clone_dir.as_posix()} pull" + env = {"HOME": home_dir} if https_proxy != "": - pull_command = f"https_proxy={https_proxy} {pull_command}" + env["HTTPS_PROXY"] = https_proxy - if not _run_command_as_user(user, pull_command): + if not _run_command_as_user(user, f"git -C {clone_dir.as_posix()} pull", env): logger.error("Failed to update existing git-ubuntu source.") return False return True # Clone the repository - clone_command = f"git clone {source_url} {clone_dir.as_posix()}" + env = {"HOME": home_dir} if https_proxy != "": - clone_command = f"https_proxy={https_proxy} {clone_command}" + env["HTTPS_PROXY"] = https_proxy logger.info("Cloning git-ubuntu source to %s", clone_dir.as_posix()) - if not _run_command_as_user(user, clone_command): + if not _run_command_as_user(user, f"git clone {source_url} {clone_dir.as_posix()}", env): logger.error("Failed to clone git-ubuntu source.") return False @@ -325,43 +329,52 @@ def set_snap_homedirs(home_dir: str) -> bool: return True -def update_git_user_name(user: str, name: str) -> bool: +def update_git_user_name(user: str, name: str, home_dir: str) -> bool: """Update the git user full name entry. Args: user: The system user to update the config for. name: The full name for the git user. + home_dir: The home directory for the user. Returns: True if config update succeeded, False otherwise. """ logger.info("Setting git user.name to %s for user %s.", name, user) - return _run_command_as_user(user, f"git config --global user.name '{name}'") + return _run_command_as_user( + user, f"git config --global user.name '{name}'", {"HOME": home_dir} + ) -def update_git_email(user: str, email: str) -> bool: +def update_git_email(user: str, email: str, home_dir: str) -> bool: """Update the git user email address entry. Args: user: The system user to update the config for. email: The email address for the git user. + home_dir: The home directory for the user. Returns: True if config update succeeded, False otherwise. """ logger.info("Setting git user.email to %s for user %s.", email, user) - return _run_command_as_user(user, f"git config --global user.email {email}") + return _run_command_as_user( + user, f"git config --global user.email {email}", {"HOME": home_dir} + ) -def update_git_ubuntu_lpuser(user: str, lp_username: str) -> bool: +def update_git_ubuntu_lpuser(user: str, lp_username: str, home_dir: str) -> bool: """Update the launchpad user setting for git. Args: user: The system user to update the config for. lp_username: The launchpad username set in git. + home_dir: The home directory for the user. Returns: True if config update succeeded, False otherwise. """ logger.info("Setting git gitubuntu.lpuser to %s for user %s.", lp_username, user) - return _run_command_as_user(user, f"git config --global gitubuntu.lpuser {lp_username}") + return _run_command_as_user( + user, f"git config --global gitubuntu.lpuser {lp_username}", {"HOME": home_dir} + ) diff --git a/tests/unit/test_user_management.py b/tests/unit/test_user_management.py index ed1a51d..dd243ec 100644 --- a/tests/unit/test_user_management.py +++ b/tests/unit/test_user_management.py @@ -15,9 +15,9 @@ def test_git_update_user_name_config_success(mock_run_command_as_user): """Test successful git user name config update.""" mock_run_command_as_user.return_value = True - assert user_management.update_git_user_name("ubuntu", "Test User") + assert user_management.update_git_user_name("ubuntu", "Test User", "/home/ubuntu") mock_run_command_as_user.assert_called_once_with( - "ubuntu", "git config --global user.name 'Test User'" + "ubuntu", "git config --global user.name 'Test User'", {"HOME": "/home/ubuntu"} ) @@ -26,9 +26,9 @@ def test_git_update_user_name_config_fail(mock_run_command_as_user): """Test failed git user name config update.""" mock_run_command_as_user.return_value = False - assert not user_management.update_git_user_name("ubuntu", "Test User") + assert not user_management.update_git_user_name("ubuntu", "Test User", "/home/ubuntu") mock_run_command_as_user.assert_called_once_with( - "ubuntu", "git config --global user.name 'Test User'" + "ubuntu", "git config --global user.name 'Test User'", {"HOME": "/home/ubuntu"} ) @@ -37,9 +37,9 @@ def test_git_update_user_email_config_success(mock_run_command_as_user): """Test successful git user email config update.""" mock_run_command_as_user.return_value = True - assert user_management.update_git_email("ubuntu", "test@example.com") + assert user_management.update_git_email("ubuntu", "test@example.com", "/home/ubuntu") mock_run_command_as_user.assert_called_once_with( - "ubuntu", "git config --global user.email test@example.com" + "ubuntu", "git config --global user.email test@example.com", {"HOME": "/home/ubuntu"} ) @@ -48,9 +48,9 @@ def test_git_update_user_email_config_fail(mock_run_command_as_user): """Test failed git user email config update.""" mock_run_command_as_user.return_value = False - assert not user_management.update_git_email("ubuntu", "test@example.com") + assert not user_management.update_git_email("ubuntu", "test@example.com", "/home/ubuntu") mock_run_command_as_user.assert_called_once_with( - "ubuntu", "git config --global user.email test@example.com" + "ubuntu", "git config --global user.email test@example.com", {"HOME": "/home/ubuntu"} ) @@ -59,9 +59,9 @@ def test_lp_user_config_success(mock_run_command_as_user): """Test successful launchpad user config update.""" mock_run_command_as_user.return_value = True - assert user_management.update_git_ubuntu_lpuser("ubuntu", "test-lp-user") + assert user_management.update_git_ubuntu_lpuser("ubuntu", "test-lp-user", "/home/ubuntu") mock_run_command_as_user.assert_called_once_with( - "ubuntu", "git config --global gitubuntu.lpuser test-lp-user" + "ubuntu", "git config --global gitubuntu.lpuser test-lp-user", {"HOME": "/home/ubuntu"} ) @@ -70,7 +70,7 @@ def test_lp_user_config_fail(mock_run_command_as_user): """Test failed launchpad user config update.""" mock_run_command_as_user.return_value = False - assert not user_management.update_git_ubuntu_lpuser("ubuntu", "test-lp-user") + assert not user_management.update_git_ubuntu_lpuser("ubuntu", "test-lp-user", "/home/ubuntu") mock_run_command_as_user.assert_called_once_with( - "ubuntu", "git config --global gitubuntu.lpuser test-lp-user" + "ubuntu", "git config --global gitubuntu.lpuser test-lp-user", {"HOME": "/home/ubuntu"} ) From e7df78268b3908a9124ed717f1ad98f06f00561c Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Mon, 15 Dec 2025 17:18:07 -0500 Subject: [PATCH 19/19] Use workload_status --- tests/integration/helpers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index 15bb2e6..c76b988 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -82,7 +82,7 @@ def wait_for_all_units_running(app: str, juju: jubilant.Juju) -> None: juju.wait( lambda status: all( unit_status.is_active - and "Running git-ubuntu importer" in unit_status.juju_status.message + and "Running git-ubuntu importer" in unit_status.workload_status.message for unit_status in status.apps[app].units.values() ) )