From 48918c0d4a27332a7feff468a4b589a65750dd25 Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Wed, 12 Nov 2025 13:43:53 -0500 Subject: [PATCH 1/9] Add some additional links to charmcraft yaml --- charmcraft.yaml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/charmcraft.yaml b/charmcraft.yaml index 856ccf9..c47fadf 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -48,7 +48,14 @@ peers: interface: git_ubuntu_primary_info links: - documentation: https://discourse.charmhub.io/t/git-ubuntu-operator-documentation/19098 + website: + - https://charmhub.io/git-ubuntu + issues: + - https://github.com/canonical/git-ubuntu-operator/issues + source: + - https://github.com/canonical/git-ubuntu-operator + documentation: + - https://discourse.charmhub.io/t/git-ubuntu-operator-documentation/19098 config: options: From 2308267f827b81e6cd1a36fd3c36679fc02a60ba Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Thu, 13 Nov 2025 08:31:42 -0500 Subject: [PATCH 2/9] Refactor folder creation to reduce duplicate code --- src/user_management.py | 107 +++++++++++++++-------------------------- 1 file changed, 39 insertions(+), 68 deletions(-) diff --git a/src/user_management.py b/src/user_management.py index 22ba352..5bd5698 100644 --- a/src/user_management.py +++ b/src/user_management.py @@ -30,6 +30,42 @@ def _run_command_as_user(user: str, command: str) -> bool: return True +def _mkdir_for_user_with_error_checking( + directory: pathops.LocalPath, user: str, mode: int = 0o755 +) -> bool: + """Create a directory and handle possible mkdir errors. + + Args: + directory: The directory to create, skipping if it exists. + user: The user who should own this directory. + mode: The permissions mode for the folder, defaults to standard rwxr-xr-x. + + Returns: + True if the folder was created, False otherwise. + """ + try: + directory.mkdir(parents=True, user=user, group=user, mode=mode) + return True + except FileExistsError: + logger.info("Directory %s already exists.", directory.as_posix()) + return True + except NotADirectoryError: + logger.error("Directory location %s already exists as a file.", directory.as_posix()) + except PermissionError: + logger.error( + "Unable to create new directory %s: permission denied.", + directory.as_posix(), + ) + except LookupError: + logger.error( + "Unable to create directory %s: unknown user/group %s", + directory.as_posix(), + user, + ) + + return False + + def _clone_git_ubuntu_source(cloning_user: str, parent_directory: str, source_url: str) -> bool: """Clone the git-ubuntu git repo to a given directory. @@ -71,31 +107,8 @@ def _write_python_keyring_config_file(user: str, home_dir: str) -> bool: python_keyring_config = pathops.LocalPath(home_dir, ".config/python_keyring/keyringrc.cfg") parent_dir = python_keyring_config.parent - config_dir_success = False - try: - parent_dir.mkdir(parents=True, user=user, group=user) - config_dir_success = True - except FileExistsError: - logger.info("User config directory %s already exists.", parent_dir.as_posix()) - config_dir_success = True - except NotADirectoryError: - logger.error( - "User config directory location %s already exists as a file.", parent_dir.as_posix() - ) - except PermissionError: - logger.error( - "Unable to create new user config directory %s: permission denied.", - parent_dir.as_posix(), - ) - except LookupError: - logger.error( - "Unable to create config directory %s: unknown user/group %s", - parent_dir.as_posix(), - user, - ) - - if not config_dir_success: + if not _mkdir_for_user_with_error_checking(parent_dir, user): return False keyring_config_success = False @@ -151,27 +164,8 @@ def setup_git_ubuntu_user_files(user: str, home_dir: str, git_ubuntu_source_url: # Create the services folder if it does not yet exist services_dir = pathops.LocalPath(home_dir, "services") - services_dir_success = False - - try: - services_dir.mkdir(parents=True, user=user, group=user) - logger.info("Created services directory %s.", services_dir) - services_dir_success = True - except FileExistsError: - logger.info("Services directory %s already exists.", services_dir) - services_dir_success = True - except NotADirectoryError: - logger.error("Service directory location %s already exists as a file.", services_dir) - except PermissionError: - logger.error("Unable to create new service directory %s: permission denied.", services_dir) - except LookupError: - logger.error( - "Unable to create service directory %s: unknown user/group %s", - services_dir, - user, - ) - if not services_dir_success: + if not _mkdir_for_user_with_error_checking(services_dir, user): return False return _write_python_keyring_config_file(user, home_dir) @@ -191,31 +185,8 @@ def update_ssh_private_key(user: str, home_dir: str, ssh_key_data: str) -> bool: ssh_key_file = pathops.LocalPath(home_dir, ".ssh/id") parent_dir = ssh_key_file.parent - ssh_dir_success = False - - try: - parent_dir.mkdir(mode=0o700, parents=True, user=user, group=user) - ssh_dir_success = True - except FileExistsError: - logger.info("ssh directory %s already exists.", parent_dir.as_posix()) - ssh_dir_success = True - except NotADirectoryError: - logger.error( - "User ssh directory location %s already exists as a file.", parent_dir.as_posix() - ) - except PermissionError: - logger.error( - "Unable to create user ssh directory %s: permission denied.", - parent_dir.as_posix(), - ) - except LookupError: - logger.error( - "Unable to create user ssh directory %s: unknown user/group %s", - parent_dir.as_posix(), - user, - ) - if not ssh_dir_success: + if not _mkdir_for_user_with_error_checking(parent_dir, user, 0o700): return False key_success = False From e3f4384af3120f872d58c5fe70693abe90816b0a Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Thu, 13 Nov 2025 09:07:16 -0500 Subject: [PATCH 3/9] Fix link entry format --- charmcraft.yaml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/charmcraft.yaml b/charmcraft.yaml index c47fadf..adaea6c 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -48,14 +48,14 @@ peers: interface: git_ubuntu_primary_info links: - website: - - https://charmhub.io/git-ubuntu - issues: - - https://github.com/canonical/git-ubuntu-operator/issues - source: - - https://github.com/canonical/git-ubuntu-operator - documentation: - - https://discourse.charmhub.io/t/git-ubuntu-operator-documentation/19098 + website: | + https://charmhub.io/git-ubuntu + issues: | + https://github.com/canonical/git-ubuntu-operator/issues + source: | + https://github.com/canonical/git-ubuntu-operator + documentation: | + https://discourse.charmhub.io/t/git-ubuntu-operator-documentation/19098 config: options: From ebdb09bd8641cd1f594a7e01277e789581969a28 Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Thu, 13 Nov 2025 09:07:42 -0500 Subject: [PATCH 4/9] Add lpuser_lp_key config option --- charmcraft.yaml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/charmcraft.yaml b/charmcraft.yaml index adaea6c..0124ac4 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -81,6 +81,11 @@ config: An ssh private key that matches with a public key associated with the lpuser account on Launchpad. type: secret + lpuser_lp_key: + description: | + A Launchpad keyring entry that allows launchpadlib access, associated + with the lpuser account on Launchpad. + type: secret publish: description: | If updates should be pushed to Launchpad. Set to False for local From a25138e2fefb57beb16998ce42ae6b556a5d8ec6 Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Thu, 13 Nov 2025 09:07:00 -0500 Subject: [PATCH 5/9] Add lp key property --- src/charm.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/charm.py b/src/charm.py index e7c2ebc..d2a2b84 100755 --- a/src/charm.py +++ b/src/charm.py @@ -112,6 +112,21 @@ def _lpuser_ssh_key(self) -> str | None: return None + @property + def _lpuser_lp_key(self) -> str | None: + try: + secret_id = str(self.config["lpuser_lp_key"]) + lp_key_secret = self.model.get_secret(id=secret_id) + lp_key_data = lp_key_secret.get_content().get("lpkey") + + if lp_key_data is not None: + return str(lp_key_data) + + except (KeyError, ops.SecretNotFoundError, ops.model.ModelError): + pass + + return None + @property def _git_ubuntu_primary_relation(self) -> ops.Relation | None: """Get the peer relation that contains the primary node IP. From 362f4676aa8c8f86f84b8b413d9248442906a596 Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Thu, 13 Nov 2025 09:24:14 -0500 Subject: [PATCH 6/9] Add function to update launchpad keyring secret --- src/user_management.py | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/user_management.py b/src/user_management.py index 5bd5698..685b9ca 100644 --- a/src/user_management.py +++ b/src/user_management.py @@ -209,6 +209,44 @@ def update_ssh_private_key(user: str, home_dir: str, ssh_key_data: str) -> bool: return key_success +def update_launchpad_keyring_secret(user: str, home_dir: str, lp_key_data: str) -> bool: + """Create or refresh the python keyring file for launchpad access. + + Args: + user: The git-ubuntu user. + home_dir: The home directory for the user. + lp_key_data: The private keyring data. + + Returns: + True if directory and file creation succeeded, False otherwise. + """ + lp_key_file = pathops.LocalPath(home_dir, ".local/share/python_keyring/keyring_pass.cfg") + + parent_dir = lp_key_file.parent + + if not _mkdir_for_user_with_error_checking(parent_dir, user): + return False + + key_success = False + + try: + lp_key_file.write_text( + lp_key_data, + mode=0o600, + user=user, + group=user, + ) + key_success = True + except (FileNotFoundError, NotADirectoryError) as e: + logger.error("Failed to create lp key entry due to directory issues: %s", str(e)) + except LookupError as e: + logger.error("Failed to create lp key entry due to issues with root user: %s", str(e)) + except PermissionError as e: + logger.error("Failed to create lp key entry due to permission issues: %s", str(e)) + + return key_success + + def set_snap_homedirs(home_dir: str) -> bool: """Allow snaps to run for a user with a given home directory. From 161b5294637a7d39d0da3f865c77f52c451704ba Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Thu, 13 Nov 2025 09:58:08 -0500 Subject: [PATCH 7/9] Add launchpad key entry if possible --- src/charm.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/charm.py b/src/charm.py index d2a2b84..e8ba368 100755 --- a/src/charm.py +++ b/src/charm.py @@ -208,6 +208,7 @@ def _refresh_importer_node(self) -> None: will_publish = self._is_publishing_active ssh_key_data = self._lpuser_ssh_key + lp_key_data = self._lpuser_lp_key if will_publish: if ssh_key_data is None: @@ -220,6 +221,15 @@ def _refresh_importer_node(self) -> None: GIT_UBUNTU_SYSTEM_USER_USERNAME, GIT_UBUNTU_USER_HOME_DIR, ssh_key_data ) + if lp_key_data is None: + logger.warning( + "Launchpad keyring entry unavailable, unable to gather package updates." + ) + else: + usr.update_launchpad_keyring_secret( + GIT_UBUNTU_SYSTEM_USER_USERNAME, GIT_UBUNTU_USER_HOME_DIR, lp_key_data + ) + if self._is_primary: if not node.setup_primary_node( GIT_UBUNTU_USER_HOME_DIR, From cb9965f8027c146219e1ddfbf680805d83fe0210 Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Thu, 13 Nov 2025 10:52:05 -0500 Subject: [PATCH 8/9] move docs to top of links --- charmcraft.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/charmcraft.yaml b/charmcraft.yaml index 0124ac4..8bdf334 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -48,14 +48,14 @@ peers: interface: git_ubuntu_primary_info links: + documentation: | + https://discourse.charmhub.io/t/git-ubuntu-operator-documentation/19098 website: | https://charmhub.io/git-ubuntu issues: | https://github.com/canonical/git-ubuntu-operator/issues source: | https://github.com/canonical/git-ubuntu-operator - documentation: | - https://discourse.charmhub.io/t/git-ubuntu-operator-documentation/19098 config: options: From 11e98cdc0f521a432783db182b702a4c9a4ed3a9 Mon Sep 17 00:00:00 2001 From: Lena Voytek Date: Thu, 13 Nov 2025 17:48:56 -0500 Subject: [PATCH 9/9] Remove workers from primary node to split responsibilities --- charmcraft.yaml | 2 +- src/charm.py | 3 --- src/importer_node.py | 19 +------------------ tests/integration/test_charm.py | 23 ++++++++++++----------- tests/unit/test_importer_node.py | 21 +++++---------------- 5 files changed, 19 insertions(+), 49 deletions(-) diff --git a/charmcraft.yaml b/charmcraft.yaml index 8bdf334..d291bf3 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -94,6 +94,6 @@ config: type: boolean workers: description: | - The number of git-ubuntu worker processes to maintain. + The number of git-ubuntu worker processes to maintain per secondary node. default: 2 type: int diff --git a/src/charm.py b/src/charm.py index e8ba368..2cfd94b 100755 --- a/src/charm.py +++ b/src/charm.py @@ -233,10 +233,7 @@ def _refresh_importer_node(self) -> None: if self._is_primary: if not node.setup_primary_node( GIT_UBUNTU_USER_HOME_DIR, - self._node_id, - self._num_workers, GIT_UBUNTU_SYSTEM_USER_USERNAME, - will_publish, self._controller_port, ): self.unit.status = ops.BlockedStatus("Failed to install git-ubuntu services.") diff --git a/src/importer_node.py b/src/importer_node.py index 6a21981..7b18060 100644 --- a/src/importer_node.py +++ b/src/importer_node.py @@ -57,20 +57,14 @@ def setup_secondary_node( def setup_primary_node( git_ubuntu_user_home: str, - node_id: int, - num_workers: int, system_user: str, - push_to_lp: bool, primary_port: int, ) -> bool: - """Set up necessary services for a primary git-ubuntu importer node. + """Set up poller and broker services to create a primary git-ubuntu importer 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. Returns: @@ -78,17 +72,6 @@ def setup_primary_node( """ services_folder = pathops.LocalPath(git_ubuntu_user_home, "services") - if not setup_secondary_node( - git_ubuntu_user_home, - node_id, - num_workers, - system_user, - push_to_lp, - primary_port, - "127.0.0.1", - ): - return False - # Setup broker service. if not git_ubuntu.setup_broker_service( services_folder.as_posix(), diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index b8cc4bc..52cfe4c 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -67,18 +67,19 @@ def get_services_dict(unit_name: str, juju: jubilant.Juju) -> dict[str, dict[str == "git-ubuntu importer service poller" ) - node_id = int(unit_name.split("/")[-1]) + 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" - ) + 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): diff --git a/tests/unit/test_importer_node.py b/tests/unit/test_importer_node.py index d5717a8..ee39618 100644 --- a/tests/unit/test_importer_node.py +++ b/tests/unit/test_importer_node.py @@ -37,19 +37,14 @@ def test_setup_secondary_node_failure(mock_setup_worker): @patch("importer_node.git_ubuntu.setup_poller_service") @patch("importer_node.git_ubuntu.setup_broker_service") -@patch("importer_node.setup_secondary_node") -def test_setup_primary_node_success(mock_secondary, mock_broker, mock_poller): +def test_setup_primary_node_success(mock_broker, mock_poller): """Test successful primary node setup.""" - mock_secondary.return_value = True mock_broker.return_value = True mock_poller.return_value = True - result = importer_node.setup_primary_node( - "/var/local/git-ubuntu", 1, 2, "git-ubuntu", True, 1692 - ) + result = importer_node.setup_primary_node("/var/local/git-ubuntu", "git-ubuntu", 1692) assert result is True - mock_secondary.assert_called_once() mock_broker.assert_called_once() mock_poller.assert_called_once() @@ -59,9 +54,7 @@ def test_setup_primary_node_secondary_failure(mock_secondary): """Test primary node setup with secondary failure.""" mock_secondary.return_value = False - result = importer_node.setup_primary_node( - "/var/local/git-ubuntu", 1, 2, "git-ubuntu", True, 1692 - ) + result = importer_node.setup_primary_node("/var/local/git-ubuntu", "git-ubuntu", 1692) assert result is False @@ -73,9 +66,7 @@ def test_setup_primary_node_broker_failure(mock_secondary, mock_broker): mock_secondary.return_value = True mock_broker.return_value = False - result = importer_node.setup_primary_node( - "/var/local/git-ubuntu", 1, 2, "git-ubuntu", True, 1692 - ) + result = importer_node.setup_primary_node("/var/local/git-ubuntu", "git-ubuntu", 1692) assert result is False @@ -89,9 +80,7 @@ def test_setup_primary_node_poller_failure(mock_secondary, mock_broker, mock_pol mock_broker.return_value = True mock_poller.return_value = False - result = importer_node.setup_primary_node( - "/var/local/git-ubuntu", 1, 2, "git-ubuntu", True, 1692 - ) + result = importer_node.setup_primary_node("/var/local/git-ubuntu", "git-ubuntu", 1692) assert result is False