Fix pkg_mgr_name fact finding for Fedora #40922

maxamillion · 2018-05-30T21:16:03Z

Signed-off-by: Adam Miller admiller@redhat.com

SUMMARY

Raises priority of dnf over apt because it's possible to install apt on Fedora from the official distro repositories but not the other way around.

Add integration tests to ensure this doesn't break in the future.

Fixes #34014
Fixes #27300

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

lib/ansible/module_utils/facts/system/pkg_mgr.py

ANSIBLE VERSION

ansible 2.7.0dev0 (facts/system/pkg_mgr 14a2dc89ca) last updated 2018/05/30 16:13:01 (GMT -500)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/admiller/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/admiller/src/dev/ansible/lib/ansible
  executable location = /home/admiller/src/dev/ansible/bin/ansible
  python version = 2.7.15 (default, May 15 2018, 15:37:31) [GCC 7.3.1 20180303 (Red Hat 7.3.1-5)]

ansibot · 2018-05-30T22:21:59Z

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/module_utils/facts/system/pkg_mgr.py:29:13: E126 continuation line over-indented for hanging indent
lib/ansible/module_utils/facts/system/pkg_mgr.py:52:13: E123 closing bracket does not match indentation of opening bracket's line

click here for bot help

bcoca · 2018-05-31T16:21:24Z

incorrect assumption as i can apt-get install yum on debian machines.

bcoca · 2018-05-31T21:48:55Z

lib/ansible/module_utils/facts/system/pkg_mgr.py

-            pkg_mgr_name = 'apt_rpm'
+        if pkg_mgr_name == 'apt':
+            if os.path.exists('/etc/fedora-release'):
+                pkg_mgr_name = 'dnf'


won't this break 'old' fedora that are 'pre dnf'?

@bcoca to the best of my knowledge, Fedora that's pre-dnf don't have a version of python new enough to fall within the support matrix of current Ansible versions. Fedora has been dnf since Fedora 15 which was released 2011-05-24 .... so, yes? technically, but outside the scope of practical support (IMO anyway).

ok, just wanted to make sure someone had looked it up

@bcoca I'm incorrect, I just checked. Fedora 14 was the first release to ship with python 2.7. So my new question is: How far back in the history of EOL'd distros do we want to maintain compat for?

I would use the word 'reasonable' but too many people make it unreasonable ....

fine with the change, the problem hits ancient version and this is mainly a desktop distro or 'cutting edge' so i expect almost no one will be hit by this.

/me now waits for thousands of users complaining

@bcoca actually, I'll go ahead and add a check for version range that way we don't accidentally break anyone who happens to still have an old Fedora machine in their environment. (I'm sure that number is non-zero)

it might be non-zero, but i suspect it is infinitely close to 0

alikins

error handling for a string 'major version' rest should be ok

alikins · 2018-06-20T20:53:15Z

lib/ansible/module_utils/facts/system/pkg_mgr.py

+        # are reported/requested
+        if pkg_mgr_name == 'apt':
+            if collected_facts["ansible_distribution"] == "Fedora":
+                if int(collected_facts["ansible_distribution_major_version"]) < 15:


In theory, ansible_distribution_major_version can be a string[1] that wont cast to int, so may need a try/except here.

1: For ex, if all else fails, it can end up as the string 'NA'

alikins · 2018-06-20T20:53:56Z

lib/ansible/module_utils/facts/system/pkg_mgr.py

+                    pkg_mgr_name = 'yum'
+                else:
+                    pkg_mgr_name = 'dnf'
+            elif collected_facts["ansible_distribution"] == 'ALT Linux':


minor nitpick... mixed use of single and double quote for keys.

Akasurde · 2018-06-21T02:30:24Z

@maxamillion You may want to tackle this as well in this PR - #27300

ansibot · 2018-06-21T18:11:41Z

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/module_utils/facts/system/pkg_mgr.py:75:19: undefined-variable Undefined variable 'collected_facts'
lib/ansible/module_utils/facts/system/pkg_mgr.py:113:0: trailing-newlines Trailing newlines

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/module_utils/facts/system/pkg_mgr.py:113:1: W391 blank line at end of file

click here for bot help

mattclay · 2018-06-27T23:04:42Z

CI failure in unit tests: https://app.shippable.com/github/ansible/ansible/runs/71044/3/tests
CI failure in integration tests: https://app.shippable.com/github/ansible/ansible/runs/71044/31/tests

bcoca · 2018-07-12T14:24:29Z

lib/ansible/module_utils/facts/system/pkg_mgr.py

+        # that are debian based, this handles some of those scenarios as they
+        # are reported/requested
+        if pkg_mgr_name == 'apt':
+            import q; q(collected_facts['ansible_distribution'])


shouldn't we look at 'os_family' and any RH based and assume yum/dnf instead?

"ansible_os_family": "RedHat" .. though we might want to include Suse and other rpm based that would appear as 'family' value

I was going off of apt purely because of the precedence model, but I could pivot and focus on distro families.

ansibot · 2018-07-12T15:21:16Z

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/module_utils/facts/system/pkg_mgr.py:76:18: multiple-statements More than one statement on a single line
lib/ansible/module_utils/facts/system/pkg_mgr.py:101:22: multiple-statements More than one statement on a single line

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with the error:

Command "ansible-doc -t strategy debug free linear" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: No module named q

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with the error:

Command "ansible-doc -t strategy debug free linear" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: No module named q

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with the error:

Command "ansible-doc -t strategy debug free linear" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: No module named 'q'

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with the error:

Command "ansible-doc -t strategy debug free linear" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: No module named 'q'

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with the error:

Command "ansible-doc -t strategy debug free linear" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: No module named 'q'

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/module_utils/facts/system/pkg_mgr.py:73:0: ImportError: No module named q

The test ansible-test sanity --test import --python 2.7 [explain] failed with 6 errors:

lib/ansible/module_utils/facts/system/pkg_mgr.py:73:0: ImportError: No module named q
lib/ansible/modules/cloud/azure/_azure.py:169:0: ImportError: No module named q
lib/ansible/modules/cloud/cloudstack/cs_facts.py:118:0: ImportError: No module named q
lib/ansible/modules/system/hostname.py:50:0: ImportError: No module named q
lib/ansible/modules/system/selinux.py:100:0: ImportError: No module named q
lib/ansible/modules/system/setup.py:134:0: ImportError: No module named q

The test ansible-test sanity --test import --python 3.5 [explain] failed with 3 errors:

lib/ansible/module_utils/facts/system/pkg_mgr.py:73:0: ImportError: No module named 'q'
lib/ansible/modules/cloud/cloudstack/cs_facts.py:118:0: ImportError: No module named 'q'
lib/ansible/modules/system/setup.py:135:0: ImportError: No module named 'q'

The test ansible-test sanity --test import --python 3.6 [explain] failed with 3 errors:

lib/ansible/module_utils/facts/system/pkg_mgr.py:73:0: ModuleNotFoundError: No module named 'q'
lib/ansible/modules/cloud/cloudstack/cs_facts.py:118:0: ModuleNotFoundError: No module named 'q'
lib/ansible/modules/system/setup.py:135:0: ModuleNotFoundError: No module named 'q'

The test ansible-test sanity --test import --python 3.7 [explain] failed with 3 errors:

lib/ansible/module_utils/facts/system/pkg_mgr.py:73:0: ModuleNotFoundError: No module named 'q'
lib/ansible/modules/cloud/cloudstack/cs_facts.py:118:0: ModuleNotFoundError: No module named 'q'
lib/ansible/modules/system/setup.py:135:0: ModuleNotFoundError: No module named 'q'

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

lib/ansible/module_utils/facts/system/pkg_mgr.py:74:5: E301 expected 1 blank line, found 0
lib/ansible/module_utils/facts/system/pkg_mgr.py:76:17: E702 multiple statements on one line (semicolon)
lib/ansible/module_utils/facts/system/pkg_mgr.py:101:21: E702 multiple statements on one line (semicolon)

click here for bot help

Signed-off-by: Adam Miller <admiller@redhat.com>

… when needed Signed-off-by: Adam Miller <admiller@redhat.com>

Signed-off-by: Adam Miller <admiller@redhat.com>

alikins · 2018-07-12T18:47:30Z

test/units/module_utils/facts/test_collectors.py

+    }
+
+    import ansible.module_utils.facts.system.pkg_mgr
+    ansible.module_utils.facts.system.pkg_mgr.os = Mock()


I think this should be using mock.patch?

iirc, this can leave ansible.module_utils.facts.system.pkg_mgr monkeypatched to be a mock for the rest of the tests.

Something like:

--- test/units/module_utils/facts/test_collectors.py +++ test/units/module_utils/facts/test_collectors.py @@ -258,12 +258,8 @@ class TestPkgMgrFactsAptFedora(BaseFactsTest): "ansible_pkg_mgr": "apt" } - import ansible.module_utils.facts.system.pkg_mgr - ansible.module_utils.facts.system.pkg_mgr.os = Mock() - ansible.module_utils.facts.system.pkg_mgr.os.path = Mock() - ansible.module_utils.facts.system.pkg_mgr.os.path.exists = Mock(side_effect=_sanitize_os_path_apt_get) - - def test_collect(self): + @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)

Signed-off-by: Adam Miller <admiller@redhat.com>

maxamillion · 2018-07-19T22:08:18Z

rebuild_merge

maxamillion · 2018-07-20T14:10:24Z

Marking this here for posterity, @alikins +1'd in slack.

@alikins

@alikins +1'd in slack

maxamillion · 2018-07-20T14:10:55Z

rebuild_merge

maxamillion · 2018-07-20T16:21:27Z

rebuild_merge

maxamillion · 2018-07-21T05:21:20Z

rebuild_merge

maxamillion · 2018-07-23T13:25:24Z

rebuild_merge

maxamillion · 2018-07-23T13:49:03Z

rebuild_merge

sourcejedi · 2018-07-23T15:22:24Z

Thank you! I believe this will fix my bug with mkosi.

Independent from that (at least for now), I notice Fedora has zypper as a package you can install . I think for much the same reason, looking at man systemd-nspawn. I think that's it for current Fedora, looking at dnf search "package manager".

abadger · 2018-07-24T15:30:31Z

lib/ansible/module_utils/facts/system/pkg_mgr.py

+    required_facts = set(['distribution'])
+
+    def _check_rh_versions(self, collected_facts):
+        if collected_facts['ansible_distribution'] == 'Fedora':


Maybe we should also check whether yum or dnf exists in the PATH before setting it in this function?

abadger · 2018-07-24T15:34:35Z

lib/ansible/module_utils/facts/system/pkg_mgr.py

+        # 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)


Maybe organizing this with one conditional per ansible_os_family will be better for the future (and allow handling a distro being able to install more package managers in the future)

if collected_facts['ansible_os_family'] == "RedHat": if pkg_mgr_name not in ('yum', 'dnf'): pkg_mgr_name = self._check_rh_versions(collected_facts) elif collected_facts['ansible_os_family'] == 'Altlinux': if pkg_mgr_name == 'apt': pkg_mgr_name = 'apt_rpm'

* 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 ansible#34014 Signed-off-by: Adam Miller <admiller@redhat.com> * remove q debugging output I accidentally left behind Signed-off-by: Adam Miller <admiller@redhat.com> * add os_family to the conditional so we're only hitting that code path when needed Signed-off-by: Adam Miller <admiller@redhat.com> * setup for a _check* pattern for general os_family group pkg_mgr checking Signed-off-by: Adam Miller <admiller@redhat.com> * use Mock.patch decorator for os.path.exists in TestPkgMgrFactsAptFedora Signed-off-by: Adam Miller <admiller@redhat.com>

* 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 <admiller@redhat.com> * remove q debugging output I accidentally left behind Signed-off-by: Adam Miller <admiller@redhat.com> * add os_family to the conditional so we're only hitting that code path when needed Signed-off-by: Adam Miller <admiller@redhat.com> * setup for a _check* pattern for general os_family group pkg_mgr checking Signed-off-by: Adam Miller <admiller@redhat.com> * use Mock.patch decorator for os.path.exists in TestPkgMgrFactsAptFedora Signed-off-by: Adam Miller <admiller@redhat.com> * fix fedora version dnf fact, default pkg_mgr detection per distro family (#43261) * fix fedora version dnf fact, default pkg_mgr detection per distro family * loop over possible dnf/yum paths in case there are multiple canonical sources later in life Signed-off-by: Adam Miller <admiller@redhat.com> * pkg_mgr: fixed apt_rpm detection (#43769) Instead of checking the distribution name (which apparently is tricky to find out) check if /usr/bin/apt-get is managed by RPM. Fixes #43539 * Ensure that apt is always chosen on debian/ubuntu One can install alternate packages managers on debuntu machines. However, doing so doesn't mean you want to suddenly start using them. Add in a check similar to the fedora yum/dnf check that sets apt as the pkg_mgr if the ansible_os_family is Debian.

maxamillion force-pushed the facts/system/pkg_mgr branch from b961850 to 0699fc9 Compare May 30, 2018 21:17

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels May 30, 2018

maxamillion changed the title ~~give dnf preference over apt when fact finding pkg_mgr~~ Fix pkg_mgr_name fact finding for Fedora May 31, 2018

bcoca removed the needs_triage Needs a first human triage before being processed. label May 31, 2018

bcoca reviewed May 31, 2018

View reviewed changes

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label May 31, 2018

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jun 10, 2018

maxamillion force-pushed the facts/system/pkg_mgr branch from 4d06427 to 103f107 Compare June 20, 2018 17:03

ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jun 20, 2018

maxamillion force-pushed the facts/system/pkg_mgr branch 4 times, most recently from 3098978 to 2c50add Compare June 20, 2018 20:31

alikins previously requested changes Jun 20, 2018

View reviewed changes

mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Jun 27, 2018

bcoca reviewed Jul 12, 2018

View reviewed changes

maxamillion added 3 commits July 12, 2018 10:33

remove q debugging output I accidentally left behind

9c27494

Signed-off-by: Adam Miller <admiller@redhat.com>

add os_family to the conditional so we're only hitting that code path…

ac79eeb

… when needed Signed-off-by: Adam Miller <admiller@redhat.com>

setup for a _check* pattern for general os_family group pkg_mgr checking

6dcaa50

Signed-off-by: Adam Miller <admiller@redhat.com>

maxamillion force-pushed the facts/system/pkg_mgr branch from 133673a to 6dcaa50 Compare July 12, 2018 16:19

bcoca approved these changes Jul 12, 2018

View reviewed changes

alikins reviewed Jul 12, 2018

View reviewed changes

use Mock.patch decorator for os.path.exists in TestPkgMgrFactsAptFedora

891dff3

Signed-off-by: Adam Miller <admiller@redhat.com>

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 20, 2018

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jul 21, 2018

ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jul 23, 2018

ansibot merged commit 562ff66 into ansible:devel Jul 23, 2018

abadger reviewed Jul 24, 2018

View reviewed changes

maxamillion mentioned this pull request Jul 25, 2018

fix fedora version dnf fact, default pkg_mgr detection per distro family #43261

Merged

dagwieers added the packaging Packaging category label Mar 3, 2019

ansible locked and limited conversation to collaborators Jul 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pkg_mgr_name fact finding for Fedora #40922

Fix pkg_mgr_name fact finding for Fedora #40922

maxamillion commented May 30, 2018 •

edited

ansibot commented May 30, 2018

bcoca commented May 31, 2018

bcoca May 31, 2018

maxamillion May 31, 2018

bcoca May 31, 2018

maxamillion May 31, 2018

bcoca May 31, 2018

maxamillion May 31, 2018

bcoca May 31, 2018

alikins left a comment

alikins Jun 20, 2018

alikins Jun 20, 2018

Akasurde commented Jun 21, 2018

ansibot commented Jun 21, 2018

mattclay commented Jun 27, 2018

bcoca Jul 12, 2018

bcoca Jul 12, 2018

maxamillion Jul 12, 2018

ansibot commented Jul 12, 2018

alikins Jul 12, 2018

alikins Jul 12, 2018

maxamillion commented Jul 19, 2018

maxamillion commented Jul 20, 2018

maxamillion commented Jul 20, 2018

maxamillion commented Jul 20, 2018

maxamillion commented Jul 21, 2018

maxamillion commented Jul 23, 2018

maxamillion commented Jul 23, 2018

sourcejedi commented Jul 23, 2018

abadger Jul 24, 2018

abadger Jul 24, 2018 •

edited

Fix pkg_mgr_name fact finding for Fedora #40922

Fix pkg_mgr_name fact finding for Fedora #40922

Conversation

maxamillion commented May 30, 2018 • edited

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ansibot commented May 30, 2018

bcoca commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alikins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akasurde commented Jun 21, 2018

ansibot commented Jun 21, 2018

mattclay commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansibot commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxamillion commented Jul 19, 2018

maxamillion commented Jul 20, 2018

maxamillion commented Jul 20, 2018

maxamillion commented Jul 20, 2018

maxamillion commented Jul 21, 2018

maxamillion commented Jul 23, 2018

maxamillion commented Jul 23, 2018

sourcejedi commented Jul 23, 2018

Choose a reason for hiding this comment

abadger Jul 24, 2018 • edited

Choose a reason for hiding this comment

maxamillion commented May 30, 2018 •

edited

abadger Jul 24, 2018 •

edited