made full pylint checks the default, expanded pylint coverage to lots of tools

14 files changed, 170 insertions, 152 deletions

diff --git a/src/lib/Bcfg2/Client/Tools/APK.py b/src/lib/Bcfg2/Client/Tools/APK.py
index 9c7692dbb..539a0fddb 100644
--- a/src/lib/Bcfg2/Client/Tools/APK.py
+++ b/src/lib/Bcfg2/Client/Tools/APK.py
@@ -28,7 +28,7 @@ class APK(Bcfg2.Client.Tools.PkgTool):
             self.logger.debug(" version: %s" % version)
             self.installed[pkgname] = version
 
-    def VerifyPackage(self, entry, modlist):
+    def VerifyPackage(self, entry, _):
         """Verify Package status for entry."""
         if not 'version' in entry.attrib:
             self.logger.info("Cannot verify unversioned package %s" %
@@ -36,7 +36,8 @@ class APK(Bcfg2.Client.Tools.PkgTool):
             return False
 
         if entry.attrib['name'] in self.installed:
-            if entry.attrib['version'] in ['auto', self.installed[entry.attrib['name']]]:
+            if entry.attrib['version'] in \
+                    ['auto', self.installed[entry.attrib['name']]]:
                 #if not self.setup['quick'] and \
                 #                entry.get('verify', 'true') == 'true':
                 #FIXME: Does APK have any sort of verification mechanism?
diff --git a/src/lib/Bcfg2/Client/Tools/APT.py b/src/lib/Bcfg2/Client/Tools/APT.py
index ce7e9701f..879d2720a 100644
--- a/src/lib/Bcfg2/Client/Tools/APT.py
+++ b/src/lib/Bcfg2/Client/Tools/APT.py
@@ -66,7 +66,7 @@ class APT(Bcfg2.Client.Tools.Tool):
             except SystemError:
                 e = sys.exc_info()[1]
                 self.logger.info("Failed to initialize APT cache: %s" % e)
-                raise Bcfg2.Client.Tools.toolInstantiationError
+                raise Bcfg2.Client.Tools.ToolInstantiationError
             self.pkg_cache.update()
         self.pkg_cache = apt.cache.Cache()
         if 'req_reinstall_pkgs' in dir(self.pkg_cache):
diff --git a/src/lib/Bcfg2/Client/Tools/Action.py b/src/lib/Bcfg2/Client/Tools/Action.py
index 4f4fdc3b2..7726da94c 100644
--- a/src/lib/Bcfg2/Client/Tools/Action.py
+++ b/src/lib/Bcfg2/Client/Tools/Action.py
@@ -2,21 +2,7 @@
 
 import Bcfg2.Client.Tools
 from Bcfg2.Client.Frame import matches_white_list, passes_black_list
-from Bcfg2.Compat import input
-
-"""
-<Action timing='pre|post|both'
-        name='name'
-        command='cmd text'
-        when='always|modified'
-        status='ignore|check'/>
-<PostInstall name='foo'/>
-  => <Action timing='post'
-             when='modified'
-             name='n'
-             command='foo'
-             status='ignore'/>
-"""
+from Bcfg2.Compat import input  # pylint: disable=W0622
 
 
 class Action(Bcfg2.Client.Tools.Tool):
@@ -27,6 +13,8 @@ class Action(Bcfg2.Client.Tools.Tool):
                'Action': ['name', 'timing', 'when', 'command', 'status']}
 
     def _action_allowed(self, action):
+        """ Return true if the given action is allowed to be run by
+        the whitelist or blacklist """
         if self.setup['decision'] == 'whitelist' and \
            not matches_white_list(action, self.setup['decision_list']):
             self.logger.info("In whitelist mode: suppressing Action:" + \
@@ -50,16 +38,18 @@ class Action(Bcfg2.Client.Tools.Tool):
                     return False
             if self.setup['servicemode'] == 'build':
                 if entry.get('build', 'true') == 'false':
-                    self.logger.debug("Action: Deferring execution of %s due to build mode" % (entry.get('command')))
+                    self.logger.debug("Action: Deferring execution of %s due "
+                                      "to build mode" % entry.get('command'))
                     return False
             self.logger.debug("Running Action %s" % (entry.get('name')))
-            rc = self.cmd.run(entry.get('command'))[0]
-            self.logger.debug("Action: %s got rc %s" % (entry.get('command'), rc))
-            entry.set('rc', str(rc))
+            rv = self.cmd.run(entry.get('command'))[0]
+            self.logger.debug("Action: %s got return code %s" %
+                              (entry.get('command'), rv))
+            entry.set('rc', str(rv))
             if entry.get('status', 'check') == 'ignore':
                 return True
             else:
-                return rc == 0
+                return rv == 0
         else:
             self.logger.debug("In dryrun mode: not running action: %s" %
                               (entry.get('name')))
@@ -80,6 +70,9 @@ class Action(Bcfg2.Client.Tools.Tool):
         return True
 
     def InstallPostInstall(self, entry):
+        """ Install a deprecated PostInstall entry """
+        self.logger.warning("Installing deprecated PostInstall entry %s" %
+                            entry.get("name"))
         return self.InstallAction(entry)
 
     def BundleUpdated(self, bundle, states):
diff --git a/src/lib/Bcfg2/Client/Tools/Chkconfig.py b/src/lib/Bcfg2/Client/Tools/Chkconfig.py
index b62fb9ef8..ef697091c 100644
--- a/src/lib/Bcfg2/Client/Tools/Chkconfig.py
+++ b/src/lib/Bcfg2/Client/Tools/Chkconfig.py
@@ -51,10 +51,8 @@ class Chkconfig(Bcfg2.Client.Tools.SvcTool):
         pstatus = self.check_service(entry)
         if entry.get('status') == 'on':
             status = (len(onlevels) > 0 and pstatus)
-            command = 'start'
         else:
             status = (len(onlevels) == 0 and not pstatus)
-            command = 'stop'
 
         if not status:
             if entry.get('status') == 'on':
@@ -84,8 +82,9 @@ class Chkconfig(Bcfg2.Client.Tools.SvcTool):
 
     def FindExtra(self):
         """Locate extra chkconfig Services."""
-        allsrv = [line.split()[0] for line in \
-                  self.cmd.run("/sbin/chkconfig --list 2>/dev/null|grep :on")[1]]
+        allsrv = [line.split()[0]
+                  for line in self.cmd.run("/sbin/chkconfig "
+                                           "--list 2>/dev/null|grep :on")[1]]
         self.logger.debug('Found active services:')
         self.logger.debug(allsrv)
         specified = [srv.get('name') for srv in self.getSupportedEntries()]
diff --git a/src/lib/Bcfg2/Client/Tools/Encap.py b/src/lib/Bcfg2/Client/Tools/Encap.py
index fa09c3ec7..b5057786f 100644
--- a/src/lib/Bcfg2/Client/Tools/Encap.py
+++ b/src/lib/Bcfg2/Client/Tools/Encap.py
@@ -4,6 +4,7 @@ import glob
 import re
 import Bcfg2.Client.Tools
 
+
 class Encap(Bcfg2.Client.Tools.PkgTool):
     """Support for Encap packages."""
     name = 'Encap'
@@ -14,9 +15,6 @@ class Encap(Bcfg2.Client.Tools.PkgTool):
     pkgtool = ("/usr/local/bin/epkg -l -f -q %s", ("%s", ["url"]))
     splitter = re.compile('.*/(?P<name>[\w-]+)\-(?P<version>[\w\.+-]+)')
 
-# If you define self.pkgtool and self.pkgname it will [use] the Pkgtool.Install
-# method will do the installation stuff for you
-
     def RefreshPackages(self):
         """Try to find encap packages."""
         self.installed = {}
@@ -26,24 +24,24 @@ class Encap(Bcfg2.Client.Tools.PkgTool):
                 self.installed[match.group('name')] = match.group('version')
             else:
                 print("Failed to split name %s" % pkg)
-        self.logger.debug("Encap.py: RefreshPackages: self.installed.keys() are:")
+        self.logger.debug("Encap: RefreshPackages: self.installed.keys() are:")
         self.logger.debug("%s" % list(self.installed.keys()))
 
     def VerifyPackage(self, entry, _):
         """Verify Package status for entry."""
         if not entry.get('version'):
-            self.logger.info("Insufficient information of Package %s; cannot Verify" % entry.get('name'))
+            self.logger.info("Insufficient information of Package %s; "
+                             "cannot Verify" % entry.get('name'))
             return False
         cmdrc = self.cmd.run("/usr/local/bin/epkg -q -S -k %s-%s >/dev/null" %
                             (entry.get('name'), entry.get('version')))[0]
         if cmdrc != 0:
-            self.logger.debug("Package %s version incorrect" % entry.get('name'))
+            self.logger.debug("Package %s version incorrect" %
+                              entry.get('name'))
         else:
             return True
         return False
 
-    # Can use the FindExtraPackages method from Bcfg2.Client.Tools.PkgTool
-
     def RemovePackages(self, packages):
         """Deal with extra configuration detected."""
         names = " ".join([pkg.get('name') for pkg in packages])
diff --git a/src/lib/Bcfg2/Client/Tools/FreeBSDInit.py b/src/lib/Bcfg2/Client/Tools/FreeBSDInit.py
index 10f0f2e93..8ff26d8f3 100644
--- a/src/lib/Bcfg2/Client/Tools/FreeBSDInit.py
+++ b/src/lib/Bcfg2/Client/Tools/FreeBSDInit.py
@@ -18,7 +18,7 @@ class FreeBSDInit(Bcfg2.Client.Tools.SvcTool):
     def __init__(self, logger, cfg, setup):
         Bcfg2.Client.Tools.Tool.__init__(self, logger, cfg, setup)
         if os.uname()[0] != 'FreeBSD':
-            raise Bcfg2.Client.Tools.toolInstantiationError
+            raise Bcfg2.Client.Tools.ToolInstantiationError
 
     def VerifyService(self, entry, _):
         return True
diff --git a/src/lib/Bcfg2/Client/Tools/FreeBSDPackage.py b/src/lib/Bcfg2/Client/Tools/FreeBSDPackage.py
index 3e6f2b6bb..ded84bef4 100644
--- a/src/lib/Bcfg2/Client/Tools/FreeBSDPackage.py
+++ b/src/lib/Bcfg2/Client/Tools/FreeBSDPackage.py
@@ -28,7 +28,7 @@ class FreeBSDPackage(Bcfg2.Client.Tools.PkgTool):
                 version = pattern.match(pkg).group(2)
                 self.installed[name] = version
 
-    def VerifyPackage(self, entry, modlist):
+    def VerifyPackage(self, entry, _):
         if not 'version' in entry.attrib:
             self.logger.info("Cannot verify unversioned package %s" %
                (entry.attrib['name']))
diff --git a/src/lib/Bcfg2/Client/Tools/IPS.py b/src/lib/Bcfg2/Client/Tools/IPS.py
index e30bbd2a4..dc4d48235 100644
--- a/src/lib/Bcfg2/Client/Tools/IPS.py
+++ b/src/lib/Bcfg2/Client/Tools/IPS.py
@@ -35,7 +35,7 @@ class IPS(Bcfg2.Client.Tools.PkgTool):
             if pinfo['upgradable']:
                 self.pending_upgrades.add(pname)
 
-    def VerifyPackage(self, entry, modlist):
+    def VerifyPackage(self, entry, _):
         """Verify package for entry."""
         pname = entry.get('name')
         if not 'version' in entry.attrib:
diff --git a/src/lib/Bcfg2/Client/Tools/MacPorts.py b/src/lib/Bcfg2/Client/Tools/MacPorts.py
index 9724fab57..d768019a9 100644
--- a/src/lib/Bcfg2/Client/Tools/MacPorts.py
+++ b/src/lib/Bcfg2/Client/Tools/MacPorts.py
@@ -29,7 +29,7 @@ class MacPorts(Bcfg2.Client.Tools.PkgTool):
             self.logger.info(" pkgname: %s version: %s" % (pkgname, version))
             self.installed[pkgname] = version
 
-    def VerifyPackage(self, entry, modlist):
+    def VerifyPackage(self, entry, _):
         """Verify Package status for entry."""
         if not 'version' in entry.attrib:
             self.logger.info("Cannot verify unversioned package %s" %
@@ -37,8 +37,8 @@ class MacPorts(Bcfg2.Client.Tools.PkgTool):
             return False
 
         if entry.attrib['name'] in self.installed:
-            if (self.installed[entry.attrib['name']] == entry.attrib['version'] or
-                entry.attrib['version'] == 'any'):
+            if (self.installed[entry.attrib['name']] == entry.attrib['version']
+                or entry.attrib['version'] == 'any'):
                 #if not self.setup['quick'] and \
                 #                entry.get('verify', 'true') == 'true':
                 #FIXME: We should be able to check this once
@@ -46,10 +46,11 @@ class MacPorts(Bcfg2.Client.Tools.PkgTool):
                 return True
             else:
                 self.logger.info("  %s: Wrong version installed.  "
-                                 "Want %s, but have %s" % (entry.get("name"),
-                                                           entry.get("version"),
-                                                           self.installed[entry.get("name")],
-                                                           ))
+                                 "Want %s, but have %s" %
+                                 (entry.get("name"),
+                                  entry.get("version"),
+                                  self.installed[entry.get("name")],
+                                  ))
 
                 entry.set('current_version', self.installed[entry.get('name')])
                 return False
diff --git a/src/lib/Bcfg2/Client/Tools/Pacman.py b/src/lib/Bcfg2/Client/Tools/Pacman.py
index 02889a2e7..75dd62ede 100644
--- a/src/lib/Bcfg2/Client/Tools/Pacman.py
+++ b/src/lib/Bcfg2/Client/Tools/Pacman.py
@@ -28,7 +28,7 @@ class Pacman(Bcfg2.Client.Tools.PkgTool):
             #self.logger.info(" pkgname: %s, version: %s" % (pkgname, version))
             self.installed[pkgname] = version
 
-    def VerifyPackage(self, entry, modlist):
+    def VerifyPackage(self, entry, _):
         '''Verify Package status for entry'''
 
         self.logger.info("VerifyPackage : %s : %s" % entry.get('name'),
@@ -42,7 +42,8 @@ class Pacman(Bcfg2.Client.Tools.PkgTool):
         if entry.attrib['name'] in self.installed:
             if entry.attrib['version'] == 'auto':
                 return True
-            elif self.installed[entry.attrib['name']] == entry.attrib['version']:
+            elif self.installed[entry.attrib['name']] == \
+                    entry.attrib['version']:
                 #if not self.setup['quick'] and \
                 #                entry.get('verify', 'true') == 'true':
                 #FIXME: need to figure out if pacman
@@ -79,6 +80,6 @@ class Pacman(Bcfg2.Client.Tools.PkgTool):
         try:
             self.logger.debug("Running : %s -S %s" % (self.pkgtool, pkgline))
             self.cmd.run("%s -S %s" % (self.pkgtool, pkgline))
-        except Exception:
-            e = sys.exc_info()[1]
-            self.logger.error("Error occurred during installation: %s" % e)
+        except:  # pylint: disable=W0702
+            err = sys.exc_info()[1]
+            self.logger.error("Error occurred during installation: %s" % err)
diff --git a/src/lib/Bcfg2/Client/Tools/RcUpdate.py b/src/lib/Bcfg2/Client/Tools/RcUpdate.py
index a53a45251..d5cef6e34 100644
--- a/src/lib/Bcfg2/Client/Tools/RcUpdate.py
+++ b/src/lib/Bcfg2/Client/Tools/RcUpdate.py
@@ -23,8 +23,8 @@ class RcUpdate(Bcfg2.Client.Tools.SvcTool):
 
         # check if service is enabled
         cmd = '/sbin/rc-update show default | grep %s'
-        rc = self.cmd.run(cmd % entry.get('name'))[0]
-        is_enabled = (rc == 0)
+        rv = self.cmd.run(cmd % entry.get('name'))[0]
+        is_enabled = (rv == 0)
 
         # check if init script exists
         try:
@@ -36,8 +36,8 @@ class RcUpdate(Bcfg2.Client.Tools.SvcTool):
 
         # check if service is enabled
         cmd = '/etc/init.d/%s status | grep started'
-        rc = self.cmd.run(cmd % entry.attrib['name'])[0]
-        is_running = (rc == 0)
+        rv = self.cmd.run(cmd % entry.attrib['name'])[0]
+        is_running = (rv == 0)
 
         if entry.get('status') == 'on' and not (is_enabled and is_running):
             entry.set('current_status', 'off')
@@ -60,16 +60,16 @@ class RcUpdate(Bcfg2.Client.Tools.SvcTool):
                 self.start_service(entry)
             # make sure it's enabled
             cmd = '/sbin/rc-update add %s default'
-            rc = self.cmd.run(cmd % entry.get('name'))[0]
-            return (rc == 0)
+            rv = self.cmd.run(cmd % entry.get('name'))[0]
+            return (rv == 0)
 
         elif entry.get('status') == 'off':
             if entry.get('current_status') == 'on':
                 self.stop_service(entry)
             # make sure it's disabled
             cmd = '/sbin/rc-update del %s default'
-            rc = self.cmd.run(cmd % entry.get('name'))[0]
-            return (rc == 0)
+            rv = self.cmd.run(cmd % entry.get('name'))[0]
+            return (rv == 0)
 
         return False
 
diff --git a/src/lib/Bcfg2/Client/Tools/SMF.py b/src/lib/Bcfg2/Client/Tools/SMF.py
index 3e0a9da13..43e4b3bf5 100644
--- a/src/lib/Bcfg2/Client/Tools/SMF.py
+++ b/src/lib/Bcfg2/Client/Tools/SMF.py
@@ -104,7 +104,7 @@ class SMF(Bcfg2.Client.Tools.SvcTool):
                     cmdrc = 1
             else:
                 srvdata = self.cmd.run("/usr/bin/svcs -H -o STA %s" %
-                                       entry.get('FMRI'))[1] [0].split()
+                                       entry.get('FMRI'))[1][0].split()
                 if srvdata[0] == 'MNT':
                     cmdarg = 'clear'
                 else:
@@ -126,7 +126,8 @@ class SMF(Bcfg2.Client.Tools.SvcTool):
                    self.cmd.run("/usr/bin/svcs -a -H -o FMRI,STATE")[1]]
                   if version != 'disabled']
 
-        [allsrv.remove(svc.get('FMRI')) for svc in self.getSupportedEntries() \
-         if svc.get("FMRI") in allsrv]
+        for svc in self.getSupportedEntries():
+            if svc.get("FMRI") in allsrv:
+                allsrv.remove(svc.get('FMRI')) 
         return [Bcfg2.Client.XML.Element("Service", type='smf', name=name) \
                 for name in allsrv]
diff --git a/src/lib/Bcfg2/Client/Tools/Upstart.py b/src/lib/Bcfg2/Client/Tools/Upstart.py
index aa5a921a6..02ed52544 100644
--- a/src/lib/Bcfg2/Client/Tools/Upstart.py
+++ b/src/lib/Bcfg2/Client/Tools/Upstart.py
@@ -38,14 +38,15 @@ class Upstart(Bcfg2.Client.Tools.SvcTool):
             params = ''
 
         try:
-            output = self.cmd.run('/usr/sbin/service %s status %s' % \
-                            ( entry.get('name'), params ))[1][0]
+            output = self.cmd.run('/usr/sbin/service %s status %s' %
+                                  (entry.get('name'), params))[1][0]
         except IndexError:
-            self.logger.error("Service %s not an Upstart service" % \
+            self.logger.error("Service %s not an Upstart service" %
                               entry.get('name'))
             return False
 
-        match = re.compile("%s( \(.*\))? (start|stop)/(running|waiting)" %entry.get('name') ).match( output )
+        match = re.compile("%s( \(.*\))? (start|stop)/(running|waiting)" %
+                           entry.get('name')).match(output)
         if match == None:
             # service does not exist
             entry.set('current_status', 'off')
@@ -80,9 +81,8 @@ class Upstart(Bcfg2.Client.Tools.SvcTool):
         """Locate extra Upstart services."""
         specified = [entry.get('name') for entry in self.getSupportedEntries()]
         extra = []
-        for name in [self.svcre.match(fname).group('name') for fname in
-                     glob.glob("/etc/init/*.conf") \
-                     if self.svcre.match(fname).group('name') not in specified]:
-            extra.append(name)
-        return [Bcfg2.Client.XML.Element('Service', type='upstart', name=name) \
+        for fname in glob.glob("/etc/init/*.conf"):
+            if self.svcre.match(fname).group('name') not in specified:
+                extra.append(self.svcre.match(fname).group('name'))
+        return [Bcfg2.Client.XML.Element('Service', type='upstart', name=name)
                 for name in extra]
diff --git a/src/lib/Bcfg2/Client/Tools/__init__.py b/src/lib/Bcfg2/Client/Tools/__init__.py
index c11b96ef7..8f1d277d8 100644
--- a/src/lib/Bcfg2/Client/Tools/__init__.py
+++ b/src/lib/Bcfg2/Client/Tools/__init__.py
@@ -1,23 +1,25 @@
 """This contains all Bcfg2 Tool modules"""
+
 import os
-import sys
 import stat
-import time
 from subprocess import Popen, PIPE
 import Bcfg2.Client.XML
-from Bcfg2.Compat import input, walk_packages
+from Bcfg2.Compat import input, walk_packages  # pylint: disable=W0622
 
 __all__ = [m[1] for m in walk_packages(path=__path__)]
+
+# pylint: disable=C0103
 drivers = [item for item in __all__ if item not in ['rpmtools']]
-default = [item for item in drivers if item not in ['RPM', 'Yum']]
+default = drivers[:]
+# pylint: enable=C0103
 
 
-class toolInstantiationError(Exception):
+class ToolInstantiationError(Exception):
     """This error is called if the toolset cannot be instantiated."""
     pass
 
 
-class executor:
+class Executor:
     """This class runs stuff for us"""
 
     def __init__(self, logger):
@@ -25,19 +27,33 @@ class executor:
 
     def run(self, command):
         """Run a command in a pipe dealing with stdout buffer overloads."""
-        p = Popen(command, shell=True, bufsize=16384,
-                  stdin=PIPE, stdout=PIPE, close_fds=True)
-        output = p.communicate()[0]
+        proc = Popen(command, shell=True, bufsize=16384,
+                     stdin=PIPE, stdout=PIPE, close_fds=True)
+        output = proc.communicate()[0]
         for line in output.splitlines():
             self.logger.debug('< %s' % line)
-        return (p.returncode, output.splitlines())
+        return (proc.returncode, output.splitlines())
+
 
+class ClassName(object):
+    """ This very simple descriptor class exists only to get the name
+    of the owner class.  This is used because, for historical reasons,
+    we expect every tool to have a ``name`` attribute that is in
+    almost all cases the same as the ``__class__.__name__`` attribute
+    of the plugin object.  This makes that more dynamic so that each
+    plugin isn't repeating its own name."""
 
+    def __get__(self, inst, owner):
+        return owner.__name__
+
+
+# pylint: disable=W0702
+# in the base tool class we frequently want to catch all exceptions,
+# regardless of type, so disable the pylint rule that catches that.
 class Tool(object):
-    """
-    All tools subclass this. It defines all interfaces that need to be defined.
-    """
-    name = 'Tool'
+    """ All tools subclass this. It defines all interfaces that need
+    to be defined. """
+    name = ClassName()
     __execs__ = []
     __handles__ = []
     __req__ = {}
@@ -50,7 +66,7 @@ class Tool(object):
         if not hasattr(self, '__ireq__'):
             self.__ireq__ = self.__req__
         self.config = config
-        self.cmd = executor(logger)
+        self.cmd = Executor(logger)
         self.modified = []
         self.extra = []
         self.__important__ = []
@@ -66,38 +82,38 @@ class Tool(object):
             try:
                 mode = stat.S_IMODE(os.stat(filename)[stat.ST_MODE])
                 if mode & stat.S_IEXEC != stat.S_IEXEC:
-                    self.logger.debug("%s: %s not executable" % \
+                    self.logger.debug("%s: %s not executable" %
                                       (self.name, filename))
-                    raise toolInstantiationError
+                    raise ToolInstantiationError
             except OSError:
-                raise toolInstantiationError
+                raise ToolInstantiationError
             except:
                 self.logger.debug("%s failed" % filename, exc_info=1)
-                raise toolInstantiationError
+                raise ToolInstantiationError
 
-    def BundleUpdated(self, _, states):
+    def BundleUpdated(self, bundle, states):  # pylint: disable=W0613
         """This callback is used when bundle updates occur."""
         return
 
-    def BundleNotUpdated(self, _, states):
+    def BundleNotUpdated(self, bundle, states):  # pylint: disable=W0613
         """This callback is used when a bundle is not updated."""
         return
 
-    def Inventory(self, states, structures=[]):
+    def Inventory(self, states, structures=None):
         """Dispatch verify calls to underlying methods."""
         if not structures:
             structures = self.config.getchildren()
         mods = self.buildModlist()
-        for (struct, entry) in [(struct, entry) for struct in structures \
-                                for entry in struct.getchildren() \
-                                if self.canVerify(entry)]:
-            try:
-                func = getattr(self, "Verify%s" % (entry.tag))
-                states[entry] = func(entry, mods)
-            except:
-                self.logger.error(
-                    "Unexpected failure of verification method for entry type %s" \
-                    % (entry.tag), exc_info=1)
+        for struct in structures:
+            for entry in struct.getchildren():
+                if self.canVerify(entry):
+                    try:
+                        func = getattr(self, "Verify%s" % entry.tag)
+                        states[entry] = func(entry, mods)
+                    except:
+                        self.logger.error("Unexpected failure of verification "
+                                          "method for entry type %s" %
+                                          entry.tag, exc_info=1)
         self.extra = self.FindExtra()
 
     def Install(self, entries, states):
@@ -109,8 +125,9 @@ class Tool(object):
                 if states[entry]:
                     self.modified.append(entry)
             except:
-                self.logger.error("Unexpected failure of install method for entry type %s" \
-                                  % (entry.tag), exc_info=1)
+                self.logger.error("Unexpected failure of install method for "
+                                  "entry type %s" % entry.tag,
+                                  exc_info=1)
 
     def Remove(self, entries):
         """Remove specified extra entries"""
@@ -118,26 +135,32 @@ class Tool(object):
 
     def getSupportedEntries(self):
         """Return a list of supported entries."""
-        return [entry for struct in \
-                self.config.getchildren() for entry in \
-                struct.getchildren() \
-                if self.handlesEntry(entry)]
+        rv = []
+        for struct in self.config.getchildren():
+            rv.extend([entry for entry in struct.getchildren()
+                       if self.handlesEntry(entry)])
+        return rv
 
     def handlesEntry(self, entry):
         """Return if entry is handled by this tool."""
         return (entry.tag, entry.get('type')) in self.__handles__
 
     def buildModlist(self):
-        '''Build a list of potentially modified POSIX paths for this entry'''
-        return [entry.get('name') for struct in self.config.getchildren() \
-                for entry in struct.getchildren() \
-                if entry.tag == 'Path']
+        """ Build a list of potentially modified POSIX paths for this
+        entry """
+        rv = []
+        for struct in self.config.getchildren():
+            rv.extend([entry.get('name') for entry in struct.getchildren()
+                       if entry.tag == 'Path'])
+        return rv
 
     def gatherCurrentData(self, entry):
         """Default implementation of the information gathering routines."""
         pass
 
     def missing_attrs(self, entry):
+        """ Return a list of attributes that were expected on entry
+        but not found """
         required = self.__req__[entry.tag]
         if isinstance(required, dict):
             required = ["type"]
@@ -155,9 +178,8 @@ class Tool(object):
             return False
 
         if 'failure' in entry.attrib:
-            self.logger.error("Entry %s:%s reports bind failure: %s" % \
-                              (entry.tag,
-                               entry.get('name'),
+            self.logger.error("Entry %s:%s reports bind failure: %s" %
+                              (entry.tag, entry.get('name'),
                                entry.get('failure')))
             return False
 
@@ -190,7 +212,7 @@ class Tool(object):
             return False
 
         if 'failure' in entry.attrib:
-            self.logger.error("Cannot install entry %s:%s with bind failure" % \
+            self.logger.error("Cannot install entry %s:%s with bind failure" %
                               (entry.tag, entry.get('name')))
             return False
 
@@ -202,6 +224,7 @@ class Tool(object):
                                ", ".join(missing)))
             return False
         return True
+# pylint: enable=W0702
 
 
 class PkgTool(Tool):
@@ -216,8 +239,6 @@ class PkgTool(Tool):
     def __init__(self, logger, setup, config):
         Tool.__init__(self, logger, setup, config)
         self.installed = {}
-        self.Remove = self.RemovePackages
-        self.FindExtra = self.FindExtraPackages
         self.RefreshPackages()
 
     def VerifyPackage(self, dummy, _):
@@ -225,32 +246,30 @@ class PkgTool(Tool):
         return False
 
     def Install(self, packages, states):
-        """
-           Run a one-pass install, followed by
-           single pkg installs in case of failure.
-        """
-        self.logger.info("Trying single pass package install for pkgtype %s" % \
+        """ Run a one-pass install, followed by single pkg installs in
+        case of failure. """
+        self.logger.info("Trying single pass package install for pkgtype %s" %
                          self.pkgtype)
 
         data = [tuple([pkg.get(field) for field in self.pkgtool[1][1]])
                 for pkg in packages]
         pkgargs = " ".join([self.pkgtool[1][0] % datum for datum in data])
 
-        self.logger.debug("Installing packages: :%s:" % pkgargs)
-        self.logger.debug("Running command ::%s::" % (self.pkgtool[0] % pkgargs))
+        self.logger.debug("Installing packages: %s" % pkgargs)
+        self.logger.debug("Running command: %s" % (self.pkgtool[0] % pkgargs))
 
         cmdrc = self.cmd.run(self.pkgtool[0] % pkgargs)[0]
         if cmdrc == 0:
             self.logger.info("Single Pass Succeded")
             # set all package states to true and flush workqueues
             pkgnames = [pkg.get('name') for pkg in packages]
-            for entry in [entry for entry in list(states.keys())
-                          if entry.tag == 'Package'
-                          and entry.get('type') == self.pkgtype
-                          and entry.get('name') in pkgnames]:
-                self.logger.debug('Setting state to true for pkg %s' % \
-                                  (entry.get('name')))
-                states[entry] = True
+            for entry in list(states.keys()):
+                if (entry.tag == 'Package'
+                    and entry.get('type') == self.pkgtype
+                    and entry.get('name') in pkgnames):
+                    self.logger.debug('Setting state to true for pkg %s' %
+                                      (entry.get('name')))
+                    states[entry] = True
             self.RefreshPackages()
         else:
             self.logger.error("Single Pass Failed")
@@ -259,19 +278,21 @@ class PkgTool(Tool):
             for pkg in packages:
                 # handle state tracking updates
                 if self.VerifyPackage(pkg, []):
-                    self.logger.info("Forcing state to true for pkg %s" % \
+                    self.logger.info("Forcing state to true for pkg %s" %
                                      (pkg.get('name')))
                     states[pkg] = True
                 else:
                     self.logger.info("Installing pkg %s version %s" %
                                      (pkg.get('name'), pkg.get('version')))
-                    cmdrc = self.cmd.run(self.pkgtool[0] %
-                                         (self.pkgtool[1][0] %
-                                          tuple([pkg.get(field) for field in self.pkgtool[1][1]])))
+                    cmdrc = self.cmd.run(
+                        self.pkgtool[0] %
+                        (self.pkgtool[1][0] %
+                         tuple([pkg.get(field)
+                                for field in self.pkgtool[1][1]])))
                     if cmdrc[0] == 0:
                         states[pkg] = True
                     else:
-                        self.logger.error("Failed to install package %s" % \
+                        self.logger.error("Failed to install package %s" %
                                           (pkg.get('name')))
             self.RefreshPackages()
         for entry in [ent for ent in packages if states[ent]]:
@@ -294,6 +315,9 @@ class PkgTool(Tool):
                                          type=self.pkgtype, version=version) \
                                          for (name, version) in extras]
 
+    Remove = RemovePackages
+    FindExtra = FindExtraPackages
+
 
 class SvcTool(Tool):
     """This class defines basic Service behavior"""
@@ -308,19 +332,23 @@ class SvcTool(Tool):
         return '/etc/init.d/%s %s' % (service.get('name'), action)
 
     def start_service(self, service):
+        """ Start a service """
         self.logger.debug('Starting service %s' % service.get('name'))
         return self.cmd.run(self.get_svc_command(service, 'start'))[0]
 
     def stop_service(self, service):
+        """ Stop a service """
         self.logger.debug('Stopping service %s' % service.get('name'))
         return self.cmd.run(self.get_svc_command(service, 'stop'))[0]
 
     def restart_service(self, service):
+        """ Restart a service """
         self.logger.debug('Restarting service %s' % service.get('name'))
         restart_target = service.get('target', 'restart')
         return self.cmd.run(self.get_svc_command(service, restart_target))[0]
 
     def check_service(self, service):
+        """ Get the status of a service """
         return self.cmd.run(self.get_svc_command(service, 'status'))[0] == 0
 
     def Remove(self, services):
@@ -342,10 +370,10 @@ class SvcTool(Tool):
                  not self.setup['interactive'])):
                 continue
 
-            rc = None
+            rv = None
             if entry.get('status') == 'on':
                 if self.setup['servicemode'] == 'build':
-                    rc = self.stop_service(entry)
+                    rv = self.stop_service(entry)
                 elif entry.get('name') not in self.restarted:
                     if self.setup['interactive']:
                         prompt = ('Restart service %s?: (y/N): ' %
@@ -353,30 +381,26 @@ class SvcTool(Tool):
                         ans = input(prompt)
                         if ans not in ['y', 'Y']:
                             continue
-                    rc = self.restart_service(entry)
-                    if not rc:
+                    rv = self.restart_service(entry)
+                    if not rv:
                         self.restarted.append(entry.get('name'))
             else:
-                rc = self.stop_service(entry)
-            if rc:
+                rv = self.stop_service(entry)
+            if rv:
                 self.logger.error("Failed to manipulate service %s" %
                                   (entry.get('name')))
 
     def Install(self, entries, states):
         """Install all entries in sublist."""
+        install_entries = []
         for entry in entries:
             if entry.get('install', 'true').lower() == 'false':
                 self.logger.info("Service %s installation is false. Skipping "
                                  "installation." % (entry.get('name')))
-                continue
-            try:
-                func = getattr(self, "Install%s" % (entry.tag))
-                states[entry] = func(entry)
-                if states[entry]:
-                    self.modified.append(entry)
-            except:
-                self.logger.error("Unexpected failure of install method for entry type %s"
-                                  % (entry.tag), exc_info=1)
+            else:
+                install_entries.append(entry)
+        return Tool.Install(self, install_entries, states)
 
     def InstallService(self, entry):
+        """ Install a single service entry """
         raise NotImplementedError