From d1515daf737ceaca8e544b15bdcee5c10f67888e Mon Sep 17 00:00:00 2001 From: Trent Lloyd Date: Thu, 20 May 2021 11:47:43 +0800 Subject: [PATCH] filter_missing_packages expects a list, not string filter_missing_packages expects a list and not a single package name as a string. Pass the chrony package as a single item list to avoid it checking installation candiates for 'c', 'h', 'r', 'o', 'n' and 'y' This patch simplifies the function, which makes it a little easier to reason about. Also a test is added to stop regressions in the future. [1] Ie3c9c5899c1d46edd21c32868938d3290db321e7 [2] Ifef76eacd8bd837d2181ec75e406aa35f88b8b5b Closes-Bug: #1929054 Change-Id: I93f53274bc8c7e62ebb8cb7bf81ff816f4bfd98b --- charms_ceph/utils.py | 7 ++++--- unit_tests/test_utils.py | 42 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/charms_ceph/utils.py b/charms_ceph/utils.py index e5c3879..af61e45 100644 --- a/charms_ceph/utils.py +++ b/charms_ceph/utils.py @@ -3271,13 +3271,14 @@ def determine_packages(): def determine_packages_to_remove(): """Determines packages for removal + Note: if in a container, then the CHRONY_PACKAGE is removed. + :returns: list of packages to be removed + :rtype: List[str] """ rm_packages = REMOVE_PACKAGES.copy() if is_container(): - install_list = filter_missing_packages(CHRONY_PACKAGE) - if not install_list: - rm_packages.append(CHRONY_PACKAGE) + rm_packages.extend(filter_missing_packages([CHRONY_PACKAGE])) return rm_packages diff --git a/unit_tests/test_utils.py b/unit_tests/test_utils.py index 9fd6f0e..a73de9f 100644 --- a/unit_tests/test_utils.py +++ b/unit_tests/test_utils.py @@ -870,6 +870,48 @@ class CephTestCase(unittest.TestCase): def test_create_keyrings_nautilus(self): self._test_create_keyrings(ceph_version='14.0.0') + @patch.object(utils, 'filter_missing_packages') + @patch.object(utils, 'is_container') + def test_determine_packages_to_remove_chrony( + self, + mock_is_container, + mock_filter_missing_packages): + + packages_present = ["some", "random", "packages", utils.CHRONY_PACKAGE] + packages_missing = ["some", "random", "packages"] + chrony_is_present = True + + def _filter_missing_packages(query_installed_pkgs): + pkgs = packages_present if chrony_is_present else packages_missing + return [p for p in query_installed_pkgs if p in pkgs] + + mock_filter_missing_packages.side_effect = _filter_missing_packages + + # Scenarios to check: + # 1. Not in a container. + # 2. In a container and chrony is installed. + # 3. In a container and chrony is not installed + + # verify that an array is passed: bug: #1929054 + # 1. first not in a container + mock_is_container.return_value = False + packages = utils.determine_packages_to_remove() + self.assertNotIn(utils.CHRONY_PACKAGE, packages) + mock_is_container.assert_called_once() + mock_filter_missing_packages.assert_not_called() + + # 2. now in a container and chrony is installed + mock_is_container.return_value = True + packages = utils.determine_packages_to_remove() + self.assertIn(utils.CHRONY_PACKAGE, packages) + mock_filter_missing_packages.assert_called_once_with( + [utils.CHRONY_PACKAGE]) + + # 3. in a container and chrony is not installed + chrony_is_present = False + packages = utils.determine_packages_to_remove() + self.assertNotIn(utils.CHRONY_PACKAGE, packages) + @patch.object(utils, 'chownr') @patch.object(utils, 'cmp_pkgrevno') @patch.object(utils, 'ceph_user')