diff --git a/changelogs/fragments/zypper-on-ubuntu.yaml b/changelogs/fragments/zypper-on-ubuntu.yaml new file mode 100644 index 00000000000..6afe40fc3b1 --- /dev/null +++ b/changelogs/fragments/zypper-on-ubuntu.yaml @@ -0,0 +1,4 @@ +bugfixes: + - Fixed an issue where ``ansible_facts.pkg_mgr`` would incorrectly set + to ``zypper`` on Debian/Ubuntu systems that happened to have the + command installed. diff --git a/lib/ansible/module_utils/facts/system/pkg_mgr.py b/lib/ansible/module_utils/facts/system/pkg_mgr.py index a8fcf0929dc..3bb18343c8f 100644 --- a/lib/ansible/module_utils/facts/system/pkg_mgr.py +++ b/lib/ansible/module_utils/facts/system/pkg_mgr.py @@ -19,6 +19,7 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type import os +import subprocess from ansible.module_utils.facts.collector import BaseFactCollector @@ -68,6 +69,43 @@ class PkgMgrFactCollector(BaseFactCollector): name = 'pkg_mgr' _fact_ids = set() _platform = 'Generic' + required_facts = set(['distribution']) + + def _check_rh_versions(self, pkg_mgr_name, collected_facts): + if collected_facts['ansible_distribution'] == 'Fedora': + try: + if int(collected_facts['ansible_distribution_major_version']) < 23: + for yum in [pkg_mgr for pkg_mgr in PKG_MGRS if pkg_mgr['name'] == 'yum']: + if os.path.exists(yum['path']): + pkg_mgr_name = 'yum' + break + else: + for dnf in [pkg_mgr for pkg_mgr in PKG_MGRS if pkg_mgr['name'] == 'dnf']: + if os.path.exists(dnf['path']): + pkg_mgr_name = 'dnf' + break + 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 _check_apt_flavor(self, pkg_mgr_name): + # Check if '/usr/bin/apt' is APT-RPM or an ordinary (dpkg-based) APT. + # There's rpm package on Debian, so checking if /usr/bin/rpm exists + # is not enough. Instead ask RPM if /usr/bin/apt-get belongs to some + # RPM package. + rpm_query = '/usr/bin/rpm -q --whatprovides /usr/bin/apt-get'.split() + if os.path.exists('/usr/bin/rpm'): + with open(os.devnull, 'w') as null: + try: + subprocess.check_call(rpm_query, stdout=null, stderr=null) + pkg_mgr_name = 'apt_rpm' + except subprocess.CalledProcessError: + # No apt-get in RPM database. Looks like Debian/Ubuntu + # with rpm package installed + pkg_mgr_name = 'apt' + return pkg_mgr_name def collect(self, module=None, collected_facts=None): facts_dict = {} @@ -78,10 +116,29 @@ 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' + # Handle distro family defaults when more than one package manager is + # installed, the ansible_fact entry should be the default package + # manager provided by the distro. + if collected_facts['ansible_os_family'] == "RedHat": + if pkg_mgr_name not in ('yum', 'dnf'): + pkg_mgr_name = self._check_rh_versions(pkg_mgr_name, collected_facts) + elif collected_facts['ansible_os_family'] == 'Debian' and pkg_mgr_name != 'apt': + # It's possible to install yum, dnf, zypper, rpm, etc inside of + # Debian. Doing so does not mean the system wants to use them. + pkg_mgr_name = 'apt' + elif collected_facts['ansible_os_family'] == 'Altlinux': + if pkg_mgr_name == 'apt': + pkg_mgr_name = 'apt_rpm' + + # Check if /usr/bin/apt-get is ordinary (dpkg-based) APT or APT-RPM + if pkg_mgr_name == 'apt': + pkg_mgr_name = self._check_apt_flavor(pkg_mgr_name) + + # 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 5fc7a5a01d4..4fc3a8a6796 100644 --- a/test/integration/targets/package/tasks/main.yml +++ b/test/integration/targets/package/tasks/main.yml @@ -24,11 +24,57 @@ - 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" + +# Verify correct default package manager for Debian/Ubuntu when Zypper installed +- block: + # Just make an executable file called "zypper" - installing zypper itself + # consistently is hard - and we're not going to use it + - name: install fake zypper + file: + state: touch + mode: 0755 + path: /usr/bin/zypper + - name: gather facts again + setup: + - name: validate output + assert: + that: + - 'ansible_pkg_mgr == "apt"' + always: + - name: remove fake zypper + file: + path: /usr/bin/zypper + state: absent + - name: gather facts again + setup: + when: ansible_os_family == "Debian" + ## ## 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 3d61dce4bcb..51172090956 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 6311934c3b3..668344d3684 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']