From 562ff66a987cbf7db53608fdf3981c7bb7118c30 Mon Sep 17 00:00:00 2001 From: Adam Miller Date: Mon, 23 Jul 2018 09:56:09 -0500 Subject: [PATCH] Fix pkg_mgr_name fact finding for Fedora (#40922) * Properly handle default package manager vs apt For distros where apt might be installed but is not the default package manager for the distro, properly identify the default distro package manager during fact finding and re-use fact finding from DistributionFactCollector and instead of reimplementing small portions of it in PkgMgrFactCollector Add unit test to always check the apt + Fedora combination to test the new code. Fixes #34014 Signed-off-by: Adam Miller * remove q debugging output I accidentally left behind Signed-off-by: Adam Miller * add os_family to the conditional so we're only hitting that code path when needed Signed-off-by: Adam Miller * setup for a _check* pattern for general os_family group pkg_mgr checking Signed-off-by: Adam Miller * use Mock.patch decorator for os.path.exists in TestPkgMgrFactsAptFedora Signed-off-by: Adam Miller --- .../module_utils/facts/system/pkg_mgr.py | 33 +++++++++++++++--- .../targets/package/tasks/main.yml | 24 ++++++++++++- .../facts/test_ansible_collector.py | 12 +++++-- .../module_utils/facts/test_collectors.py | 34 +++++++++++++++++++ 4 files changed, 96 insertions(+), 7 deletions(-) diff --git a/lib/ansible/module_utils/facts/system/pkg_mgr.py b/lib/ansible/module_utils/facts/system/pkg_mgr.py index a8fcf0929d..dd277cf893 100644 --- a/lib/ansible/module_utils/facts/system/pkg_mgr.py +++ b/lib/ansible/module_utils/facts/system/pkg_mgr.py @@ -68,6 +68,20 @@ class PkgMgrFactCollector(BaseFactCollector): name = 'pkg_mgr' _fact_ids = set() _platform = 'Generic' + required_facts = set(['distribution']) + + def _check_rh_versions(self, collected_facts): + if collected_facts['ansible_distribution'] == 'Fedora': + try: + if int(collected_facts['ansible_distribution_major_version']) < 15: + pkg_mgr_name = 'yum' + else: + pkg_mgr_name = 'dnf' + except ValueError: + # If there's some new magical Fedora version in the future, + # just default to dnf + pkg_mgr_name = 'dnf' + return pkg_mgr_name def collect(self, module=None, collected_facts=None): facts_dict = {} @@ -78,10 +92,21 @@ class PkgMgrFactCollector(BaseFactCollector): if os.path.exists(pkg['path']): pkg_mgr_name = pkg['name'] - if pkg_mgr_name == 'apt' and \ - os.path.exists('/usr/bin/rpm') and \ - not os.path.exists('/usr/bin/dpkg'): - pkg_mgr_name = 'apt_rpm' + # apt is easily installable and supported by distros other than those + # that are debian based, this handles some of those scenarios as they + # are reported/requested + if pkg_mgr_name == 'apt' and collected_facts['ansible_os_family'] in ["RedHat", "Altlinux"]: + if collected_facts['ansible_os_family'] == 'RedHat': + pkg_mgr_name = self._check_rh_versions(collected_facts) + + elif collected_facts['ansible_os_family'] == 'Altlinux': + pkg_mgr_name = 'apt_rpm' + + # pacman has become available by distros other than those that are Arch + # based by virtue of a dependency to the systemd mkosi project, this + # handles some of those scenarios as they are reported/requested + if pkg_mgr_name == 'pacman' and collected_facts['ansible_os_family'] in ["RedHat"]: + pkg_mgr_name = self._check_rh_versions(collected_facts) facts_dict['pkg_mgr'] = pkg_mgr_name return facts_dict diff --git a/test/integration/targets/package/tasks/main.yml b/test/integration/targets/package/tasks/main.yml index 5fc7a5a01d..0ce58739ef 100644 --- a/test/integration/targets/package/tasks/main.yml +++ b/test/integration/targets/package/tasks/main.yml @@ -24,11 +24,33 @@ - name: create our testing sub-directory file: path="{{ output_dir_test }}" state=directory +# Verify correct default package manager for Fedora +# Validates: https://github.com/ansible/ansible/issues/34014 +- block: + - name: install apt + dnf: + name: apt + state: present + - name: gather facts again + setup: + - name: validate output + assert: + that: + - 'ansible_pkg_mgr == "dnf"' + always: + - name: remove apt + dnf: + name: apt + state: absent + - name: gather facts again + setup: + when: ansible_distribution == "Fedora" + ## ## package ## -- name: define distros to attempt installing at on +- name: define distros to attempt installing at on set_fact: package_distros: - RedHat diff --git a/test/units/module_utils/facts/test_ansible_collector.py b/test/units/module_utils/facts/test_ansible_collector.py index 3d61dce4bc..5117209095 100644 --- a/test/units/module_utils/facts/test_ansible_collector.py +++ b/test/units/module_utils/facts/test_ansible_collector.py @@ -202,6 +202,8 @@ class TestCollectedFacts(unittest.TestCase): 'env'] not_expected_facts = ['facter', 'ohai'] + collected_facts = {} + def _mock_module(self, gather_subset=None): return mock_module(gather_subset=self.gather_subset) @@ -212,7 +214,8 @@ class TestCollectedFacts(unittest.TestCase): fact_collector = \ ansible_collector.AnsibleFactCollector(collectors=collectors, namespace=ns) - self.facts = fact_collector.collect(module=mock_module) + self.facts = fact_collector.collect(module=mock_module, + collected_facts=self.collected_facts) def _collectors(self, module, all_collector_classes=None, @@ -466,10 +469,15 @@ class TestOhaiCollectedFacts(TestCollectedFacts): class TestPkgMgrFacts(TestCollectedFacts): gather_subset = ['pkg_mgr'] min_fact_count = 1 - max_fact_count = 10 + max_fact_count = 20 expected_facts = ['gather_subset', 'module_setup', 'pkg_mgr'] + collected_facts = { + "ansible_distribution": "Fedora", + "ansible_distribution_major_version": "28", + "ansible_os_family": "RedHat" + } class TestOpenBSDPkgMgrFacts(TestPkgMgrFacts): diff --git a/test/units/module_utils/facts/test_collectors.py b/test/units/module_utils/facts/test_collectors.py index 6311934c3b..668344d368 100644 --- a/test/units/module_utils/facts/test_collectors.py +++ b/test/units/module_utils/facts/test_collectors.py @@ -224,6 +224,11 @@ class TestPkgMgrFacts(BaseFactsTest): valid_subsets = ['pkg_mgr'] fact_namespace = 'ansible_pkgmgr' collector_class = PkgMgrFactCollector + collected_facts = { + "ansible_distribution": "Fedora", + "ansible_distribution_major_version": "28", + "ansible_os_family": "RedHat" + } def test_collect(self): module = self._mock_module() @@ -233,6 +238,35 @@ class TestPkgMgrFacts(BaseFactsTest): self.assertIn('pkg_mgr', facts_dict) +def _sanitize_os_path_apt_get(path): + if path == '/usr/bin/apt-get': + return True + else: + return False + + +class TestPkgMgrFactsAptFedora(BaseFactsTest): + __test__ = True + gather_subset = ['!all', 'pkg_mgr'] + valid_subsets = ['pkg_mgr'] + fact_namespace = 'ansible_pkgmgr' + collector_class = PkgMgrFactCollector + collected_facts = { + "ansible_distribution": "Fedora", + "ansible_distribution_major_version": "28", + "ansible_os_family": "RedHat", + "ansible_pkg_mgr": "apt" + } + + @patch('ansible.module_utils.facts.system.pkg_mgr.os.path.exists', side_effect=_sanitize_os_path_apt_get) + def test_collect(self, mock_os_path_exists): + module = self._mock_module() + fact_collector = self.collector_class() + facts_dict = fact_collector.collect(module=module, collected_facts=self.collected_facts) + self.assertIsInstance(facts_dict, dict) + self.assertIn('pkg_mgr', facts_dict) + + class TestOpenBSDPkgMgrFacts(BaseFactsTest): __test__ = True gather_subset = ['!all', 'pkg_mgr']