[PackageManager] Make pkg_by_name() more predictable

As highlighted in #1817, `pkg_by_name()` could provide unpredictable
results, when using wildcards. As such, limited this method to only
returning package info for exact package name matches. In turn, change
`Plugin.is_installed()` to leverage `PackageManager.all_pkgs_by_name()`
which does explicitly support wildcards and returns information on _all_
matching packages, not just the last one found.

In so doing, clean up the `PackageManager` design to use a new
`packages` property for these lookups, and update the former usage of
`all_pkgs()` accordingly. Similarly, signal `get_pkg_list()` should be
private (in any sense that a python method can be) by renaming to
`_get_pkg_list()` and update the single Plugin (`etcd`) referencing this
method.

Closes: #1817

Signed-off-by: Jake Hunsaker <jhunsake@redhat.com>

7 files changed, 28 insertions, 37 deletions

diff --git a/sos/policies/distros/redhat.py b/sos/policies/distros/redhat.py
index a6ac5efc..b3cc675d 100644
--- a/sos/policies/distros/redhat.py
+++ b/sos/policies/distros/redhat.py
@@ -62,7 +62,7 @@ class RedHatPolicy(LinuxPolicy):
 
         self.valid_subclasses += [RedHatPlugin]
 
-        self.pkgs = self.package_manager.all_pkgs()
+        self.pkgs = self.package_manager.packages
 
         # If rpm query failed, exit
         if not self.pkgs:
diff --git a/sos/policies/distros/suse.py b/sos/policies/distros/suse.py
index d1b5ea77..7fa0b0c9 100644
--- a/sos/policies/distros/suse.py
+++ b/sos/policies/distros/suse.py
@@ -32,10 +32,8 @@ class SuSEPolicy(LinuxPolicy):
         self.usrmove = False
         self.package_manager = RpmPackageManager()
 
-        pkgs = self.package_manager.all_pkgs()
-
         # If rpm query timed out after timeout duration exit
-        if not pkgs:
+        if not self.package_manager.packages:
             print("Could not obtain installed package list", file=sys.stderr)
             sys.exit(1)
 
diff --git a/sos/policies/package_managers/__init__.py b/sos/policies/package_managers/__init__.py
index 42eb9447..8a5c4ee5 100644
--- a/sos/policies/package_managers/__init__.py
+++ b/sos/policies/package_managers/__init__.py
@@ -22,7 +22,7 @@ class PackageManager():
 
         package name|package.version
 
-    You may also subclass this class and provide a get_pkg_list method to
+    You may also subclass this class and provide a _generate_pkg_list method to
     build the list of packages and versions.
 
     :cvar query_command: The command to use for querying packages
@@ -56,7 +56,7 @@ class PackageManager():
     def __init__(self, chroot=None, query_command=None,
                  verify_command=None, verify_filter=None,
                  files_command=None, remote_exec=None):
-        self.packages = {}
+        self._packages = {}
         self.files = []
 
         self.query_command = query_command or self.query_command
@@ -75,6 +75,12 @@ class PackageManager():
         if chroot:
             self.chroot = chroot
 
+    @property
+    def packages(self):
+        if not self._packages:
+            self._generate_pkg_list()
+        return self._packages
+
     def all_pkgs_by_name(self, name):
         """
         Get a list of packages that match name.
@@ -85,7 +91,7 @@ class PackageManager():
         :returns: List of all packages matching `name`
         :rtype: ``list``
         """
-        return fnmatch.filter(self.all_pkgs().keys(), name)
+        return fnmatch.filter(self.packages.keys(), name)
 
     def all_pkgs_by_name_regex(self, regex_name, flags=0):
         """
@@ -100,7 +106,7 @@ class PackageManager():
         :rtype: ``list``
         """
         reg = re.compile(regex_name, flags)
-        return [pkg for pkg in self.all_pkgs().keys() if reg.match(pkg)]
+        return [pkg for pkg in self.packages.keys() if reg.match(pkg)]
 
     def pkg_by_name(self, name):
         """
@@ -112,15 +118,14 @@ class PackageManager():
         :returns: The first package that matches `name`
         :rtype: ``str``
         """
-        pkgmatches = self.all_pkgs_by_name(name)
-        if (len(pkgmatches) != 0):
-            return self.all_pkgs_by_name(name)[-1]
-        else:
+        try:
+            return self.packages[name]
+        except Exception:
             return None
 
-    def get_pkg_list(self):
-        """Returns a dictionary of packages in the following
-        format::
+    def _generate_pkg_list(self):
+        """Generates a dictionary of packages for internal use by the package
+        manager in the format::
 
             {'package_name': {'name': 'package_name',
                               'version': 'major.minor.version'}}
@@ -140,14 +145,12 @@ class PackageManager():
                     release = None
                 elif pkg.count("|") == 2:
                     name, version, release = pkg.split("|")
-                self.packages[name] = {
+                self._packages[name] = {
                     'name': name,
                     'version': version.split(".")
                 }
                 release = release if release else None
-                self.packages[name]['release'] = release
-
-        return self.packages
+                self._packages[name]['release'] = release
 
     def pkg_version(self, pkg):
         """Returns the entry in self.packages for pkg if it exists
@@ -158,22 +161,10 @@ class PackageManager():
         :returns: Package name and version, if package exists
         :rtype: ``dict`` if found, else ``None``
         """
-        pkgs = self.all_pkgs()
-        if pkg in pkgs:
-            return pkgs[pkg]
+        if pkg in self.packages:
+            return self.packages[pkg]
         return None
 
-    def all_pkgs(self):
-        """
-        Get a list of all packages.
-
-        :returns: All packages, with name and version, installed on the system
-        :rtype: ``dict``
-        """
-        if not self.packages:
-            self.packages = self.get_pkg_list()
-        return self.packages
-
     def pkg_nvra(self, pkg):
         """Get the name, version, release, and architecture for a package
 
diff --git a/sos/report/plugins/__init__.py b/sos/report/plugins/__init__.py
index ba1397a8..eed656ae 100644
--- a/sos/report/plugins/__init__.py
+++ b/sos/report/plugins/__init__.py
@@ -987,7 +987,9 @@ class Plugin():
         :returns: ``True`` id the package is installed, else ``False``
         :rtype: ``bool``
         """
-        return self.policy.pkg_by_name(package_name) is not None
+        return (
+            len(self.policy.package_manager.all_pkgs_by_name(package_name)) > 0
+        )
 
     def is_service(self, name):
         """Does the service $name exist on the system?
diff --git a/sos/report/plugins/etcd.py b/sos/report/plugins/etcd.py
index fe017e9f..d1b7ff90 100644
--- a/sos/report/plugins/etcd.py
+++ b/sos/report/plugins/etcd.py
@@ -71,7 +71,7 @@ class etcd(Plugin, RedHatPlugin):
             # assume v3 is the default
             url = 'http://localhost:2379'
             try:
-                ver = self.policy.package_manager.get_pkg_list()['etcd']
+                ver = self.policy.package_manager.packages['etcd']
                 ver = ver['version'][0]
                 if ver == '2':
                     url = 'http://localhost:4001'
diff --git a/sos/report/plugins/networking.py b/sos/report/plugins/networking.py
index 2ab5d840..cfe3d70f 100644
--- a/sos/report/plugins/networking.py
+++ b/sos/report/plugins/networking.py
@@ -261,7 +261,7 @@ class RedHatNetworking(Networking, RedHatPlugin):
     def setup(self):
         # Handle change from -T to -W in Red Hat netstat 2.0 and greater.
         try:
-            netstat_pkg = self.policy.package_manager.all_pkgs()['net-tools']
+            netstat_pkg = self.policy.package_manager.packages['net-tools']
             # major version
             if int(netstat_pkg['version'][0]) < 2:
                 self.ns_wide = "-T"
diff --git a/tests/unittests/policy_tests.py b/tests/unittests/policy_tests.py
index 6d0c42b9..81210815 100644
--- a/tests/unittests/policy_tests.py
+++ b/tests/unittests/policy_tests.py
@@ -84,7 +84,7 @@ class PackageManagerTests(unittest.TestCase):
         self.pm = PackageManager()
 
     def test_default_all_pkgs(self):
-        self.assertEquals(self.pm.all_pkgs(), {})
+        self.assertEquals(self.pm.packages, {})
 
     def test_default_all_pkgs_by_name(self):
         self.assertEquals(self.pm.all_pkgs_by_name('doesntmatter'), [])