[Plugin] Make plugin options their own class

Up until now plugin options were defined via tuples, with positional
significance of the tuple elements. This made plugin option creation
fairly easy, but option handling could easily become confusing.

Instead, create a new `PluginOpt` class that plugin options from here on
out will need to build from in order to function. These will still be
applied to plugins by inserting them into the `option_list` class attr
in order to retain an easy way to expand plugin options for authors.

Internally, options are now assigned to a dict which is then directly
accessed for plugin option manipulations. PluginOpt default values are
retained separate from their current value, and elements are assigned
directly to meaningful identifiers within the class. This should
alleviate some of the overhead when handling plugin options within sos.

Not all current tuple elements have been carried over into the new
`PluginOpt` class - for example, the 'speed' attribute has been dropped
as it does not have a current function. In the planned `sos info`
component, the time effects of a plugin option should be documented in
the `long_desc` attribute instead.

Additionally, the `Plugin.get_option_as_list()` method has been removed
as it was not being used anywhere.

Note that this particular commit only introduces the new class, and the
loading options used by `SoSReport()`. As such, plugins using options
currently will report errors during test runs. A commit following this
one will shuffle existing plugin options into the new class structure
and allow the plugins to execute normally.

Resolves: #274
Resolves: #452
Resolves: #1597

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

5 files changed, 163 insertions, 117 deletions

diff --git a/sos/report/__init__.py b/sos/report/__init__.py
index 16213c0a..b0159e5b 100644
--- a/sos/report/__init__.py
+++ b/sos/report/__init__.py
@@ -619,21 +619,24 @@ class SoSReport(SoSComponent):
     def _set_all_options(self):
         if self.opts.alloptions:
             for plugname, plug in self.loaded_plugins:
-                for name, parms in zip(plug.opt_names, plug.opt_parms):
-                    if type(parms["enabled"]) == bool:
-                        parms["enabled"] = True
+                for opt in plug.options:
+                    if bool in opt.val_type:
+                        opt.value = True
 
     def _set_tunables(self):
         if self.opts.plugopts:
             opts = {}
             for opt in self.opts.plugopts:
-                # split up "general.syslogsize=5"
                 try:
                     opt, val = opt.split("=")
                 except ValueError:
                     val = True
-                else:
-                    if val.lower() in ["off", "disable", "disabled", "false"]:
+
+                if isinstance(val, str):
+                    val = val.lower()
+                    if val in ["on", "enable", "enabled", "true", "yes"]:
+                        val = True
+                    elif val in ["off", "disable", "disabled", "false", "no"]:
                         val = False
                     else:
                         # try to convert string "val" to int()
@@ -642,7 +645,6 @@ class SoSReport(SoSComponent):
                         except ValueError:
                             pass
 
-                # split up "general.syslogsize"
                 try:
                     plug, opt = opt.split(".")
                 except ValueError:
@@ -652,16 +654,25 @@ class SoSReport(SoSComponent):
                 try:
                     opts[plug]
                 except KeyError:
-                    opts[plug] = []
-                opts[plug].append((opt, val))
+                    opts[plug] = {}
+                opts[plug][opt] = val
 
             for plugname, plug in self.loaded_plugins:
                 if plugname in opts:
-                    for opt, val in opts[plugname]:
-                        if not plug.set_option(opt, val):
+                    for opt in opts[plugname]:
+                        if opt not in plug.options:
                             self.soslog.error('no such option "%s" for plugin '
                                               '(%s)' % (opt, plugname))
                             self._exit(1)
+                        try:
+                            plug.options[opt].set_value(opts[plugname][opt])
+                            self.soslog.debug(
+                                "Set %s plugin option to %s"
+                                % (plugname, plug.options[opt])
+                            )
+                        except Exception as err:
+                            self.soslog.error(err)
+                            self._exit(1)
                     del opts[plugname]
             for plugname in opts.keys():
                 self.soslog.error('WARNING: unable to set option for disabled '
@@ -688,10 +699,8 @@ class SoSReport(SoSComponent):
 
     def _set_plugin_options(self):
         for plugin_name, plugin in self.loaded_plugins:
-            names, parms = plugin.get_all_options()
-            for optname, optparm in zip(names, parms):
-                self.all_options.append((plugin, plugin_name, optname,
-                                         optparm))
+            for opt in plugin.options:
+                self.all_options.append(plugin.options[opt])
 
     def _report_profiles_and_plugins(self):
         self.ui_log.info("")
@@ -732,31 +741,33 @@ class SoSReport(SoSComponent):
         if self.all_options:
             self.ui_log.info(_("The following options are available for ALL "
                                "plugins:"))
-            for opt in self.all_options[0][0]._default_plug_opts:
-                val = opt[3]
-                if val == -1:
+            _defaults = self.loaded_plugins[0][1]._default_plug_opts
+            for _opt in _defaults:
+                opt = _defaults[_opt]
+                val = opt.default
+                if opt.default == -1:
                     val = TIMEOUT_DEFAULT
-                self.ui_log.info(" %-25s %-15s %s" % (opt[0], val, opt[1]))
+                self.ui_log.info(" %-25s %-15s %s" % (opt.name, val, opt.desc))
             self.ui_log.info("")
 
             self.ui_log.info(_("The following plugin options are available:"))
-            for (plug, plugname, optname, optparm) in self.all_options:
-                if optname in ('timeout', 'postproc', 'cmd-timeout'):
+            for opt in self.all_options:
+                if opt.name in ('timeout', 'postproc', 'cmd-timeout'):
                     continue
                 # format option value based on its type (int or bool)
-                if type(optparm["enabled"]) == bool:
-                    if optparm["enabled"] is True:
+                if isinstance(opt.default, bool):
+                    if opt.default is True:
                         tmpopt = "on"
                     else:
                         tmpopt = "off"
                 else:
-                    tmpopt = optparm["enabled"]
+                    tmpopt = opt.default
 
                 if tmpopt is None:
                     tmpopt = 0
 
                 self.ui_log.info(" %-25s %-15s %s" % (
-                    plugname + "." + optname, tmpopt, optparm["desc"]))
+                    opt.plugin + "." + opt.name, tmpopt, opt.desc))
         else:
             self.ui_log.info(_("No plugin options available."))
 
diff --git a/sos/report/plugins/__init__.py b/sos/report/plugins/__init__.py
index 549a70ef..6c32c665 100644
--- a/sos/report/plugins/__init__.py
+++ b/sos/report/plugins/__init__.py
@@ -406,7 +406,86 @@ class SoSCommand(object):
                          sorted(self.__dict__.items()))
 
 
-class Plugin(object):
+class PluginOpt():
+    """This is used to define options available to plugins. Plugins will need
+    to define options alongside their distro-specific classes in order to add
+    support for user-controlled changes in Plugin behavior.
+
+    :param name:        The name of the plugin option
+    :type name:         ``str``
+
+    :param default:     The default value of the option
+    :type default:      Any
+
+    :param desc:        A short description of the effect of the option
+    :type desc:         ``str``
+
+    :param long_desc:   A detailed description of the option. Will be used by
+                        `sos info`
+    :type long_desc:    ``str``
+
+    :param val_type:    The type of object the option accepts for values. If
+                        not provided, auto-detect from the type of ``default``
+    :type val_type:     A single type or a ``list`` of types
+    """
+
+    name = ''
+    default = None
+    enabled = False
+    desc = ''
+    long_desc = ''
+    value = None
+    val_type = [None]
+    plugin = ''
+
+    def __init__(self, name='undefined', default=None, desc='', long_desc='',
+                 val_type=None):
+        self.name = name
+        self.default = default
+        self.desc = desc
+        self.long_desc = long_desc
+        self.value = self.default
+        if val_type is not None:
+            if not isinstance(val_type, list):
+                val_type = [val_type]
+        else:
+            val_type = [default.__class__]
+        self.val_type = val_type
+
+    def __str__(self):
+        items = [
+            'name=%s' % self.name,
+            'desc=\'%s\'' % self.desc,
+            'value=%s' % self.value,
+            'default=%s' % self.default
+        ]
+        return '(' + ', '.join(items) + ')'
+
+    def __repr__(self):
+        return self.__str__()
+
+    def set_value(self, val):
+        if not any([type(val) == _t for _t in self.val_type]):
+            valid = []
+            for t in self.val_type:
+                if t is None:
+                    continue
+                if t.__name__ == 'bool':
+                    valid.append("boolean true/false (on/off, etc)")
+                elif t.__name__ == 'str':
+                    valid.append("string (no spaces)")
+                elif t.__name__ == 'int':
+                    valid.append("integer values")
+            raise Exception(
+                "Plugin option '%s.%s' takes %s, not %s" % (
+                    self.plugin, self.name, ', '.join(valid),
+                    type(val).__name__
+                )
+            )
+        self.value = val
+
+
+class Plugin():
     """This is the base class for sosreport plugins. Plugins should subclass
     this and set the class variables where applicable.
 
@@ -474,12 +553,14 @@ class Plugin(object):
     # Default predicates
     predicate = None
     cmd_predicate = None
-    _default_plug_opts = [
-        ('timeout', 'Timeout in seconds for plugin to finish', 'fast', -1),
-        ('cmd-timeout', 'Timeout in seconds for a command', 'fast', -1),
-        ('postproc', 'Enable post-processing collected plugin data', 'fast',
-         True)
-    ]
+    _default_plug_opts = {
+        'timeout': PluginOpt('timeout', default=-1, val_type=int,
+                             desc='Timeout in seconds for plugin to finish'),
+        'cmd-timeout': PluginOpt('cmd-timeout', default=-1, val_type=int,
+                                 desc='Timeout in seconds for cmds to finish'),
+        'postproc': PluginOpt('postproc', default=True, val_type=bool,
+                              desc='Enable post-processing of collected data')
+    }
 
     def __init__(self, commons):
 
@@ -488,13 +569,12 @@ class Plugin(object):
         self._env_vars = set()
         self.alerts = []
         self.custom_text = ""
-        self.opt_names = []
-        self.opt_parms = []
         self.commons = commons
         self.forbidden_paths = []
         self.copy_paths = set()
         self.copy_strings = []
         self.collect_cmds = []
+        self.options = {}
         self.sysroot = commons['sysroot']
         self.policy = commons['policy']
         self.devices = commons['devices']
@@ -506,13 +586,12 @@ class Plugin(object):
             else logging.getLogger('sos')
 
         # add the default plugin opts
-        self.option_list.extend(self._default_plug_opts)
-
-        # get the option list into a dictionary
+        self.options.update(self._default_plug_opts)
+        for popt in self.options:
+            self.options[popt].plugin = self.name()
         for opt in self.option_list:
-            self.opt_names.append(opt[0])
-            self.opt_parms.append({'desc': opt[1], 'speed': opt[2],
-                                   'enabled': opt[3]})
+            opt.plugin = self.name()
+            self.options[opt.name] = opt
 
         # Initialise the default --dry-run predicate
         self.set_predicate(SoSPredicate(self))
@@ -1211,10 +1290,6 @@ class Plugin(object):
             for path in glob.glob(forbid, recursive=recursive):
                 self.forbidden_paths.append(path)
 
-    def get_all_options(self):
-        """return a list of all options selected"""
-        return (self.opt_names, self.opt_parms)
-
     def set_option(self, optionname, value):
         """Set the named option to value. Ensure the original type of the
         option value is preserved
@@ -1227,18 +1302,13 @@ class Plugin(object):
         :returns: ``True`` if the option is successfully set, else ``False``
         :rtype: ``bool``
         """
-        for name, parms in zip(self.opt_names, self.opt_parms):
-            if name == optionname:
-                # FIXME: ensure that the resulting type of the set option
-                # matches that of the default value. This prevents a string
-                # option from being coerced to int simply because it holds
-                # a numeric value (e.g. a password).
-                # See PR #1526 and Issue #1597
-                defaulttype = type(parms['enabled'])
-                if defaulttype != type(value) and defaulttype != type(None):
-                    value = (defaulttype)(value)
-                parms['enabled'] = value
+        if optionname in self.options:
+            try:
+                self.options[optionname].set_value(value)
                 return True
+            except Exception as err:
+                self._log_error(err)
+                raise
         return False
 
     def get_option(self, optionname, default=0):
@@ -1266,29 +1336,12 @@ class Plugin(object):
         if optionname in global_options:
             return getattr(self.commons['cmdlineopts'], optionname)
 
-        for name, parms in zip(self.opt_names, self.opt_parms):
-            if name == optionname:
-                val = parms['enabled']
-                if val is not None:
-                    return val
-                else:
-                    # if the value is `None`, use any non-zero default here,
-                    # but still return `None` if no default is given since
-                    # optionname did exist and had a `None` value
-                    return default or val
-
-        return default
-
-    def get_option_as_list(self, optionname, delimiter=",", default=None):
-        """Will try to return the option as a list separated by the
-        delimiter.
-        """
-        option = self.get_option(optionname)
-        try:
-            opt_list = [opt.strip() for opt in option.split(delimiter)]
-            return list(filter(None, opt_list))
-        except Exception:
+        if optionname in self.options:
+            opt = self.options[optionname]
+            if not default or opt.value is not None:
+                return opt.value
             return default
+        return default
 
     def _add_copy_paths(self, copy_paths):
         self.copy_paths.update(copy_paths)
diff --git a/tests/unittests/conformance_tests.py b/tests/unittests/conformance_tests.py
index 3a3f550f..5419f9d3 100644
--- a/tests/unittests/conformance_tests.py
+++ b/tests/unittests/conformance_tests.py
@@ -8,7 +8,7 @@
 import unittest
 
 
-from sos.report.plugins import import_plugin
+from sos.report.plugins import import_plugin, PluginOpt
 from sos.utilities import ImporterHelper
 
 
@@ -50,7 +50,8 @@ class PluginConformance(unittest.TestCase):
         for plug in self.plug_classes:
             self.assertIsInstance(plug.option_list, list)
             for opt in plug.option_list:
-                self.assertIsInstance(opt, tuple)
+                self.assertIsInstance(opt, PluginOpt)
+                self.assertFalse(opt.name == 'undefined')
 
     def test_plugin_architectures_set_correctly(self):
         for plug in self.plug_classes:
diff --git a/tests/unittests/option_tests.py b/tests/unittests/option_tests.py
index 58f54e94..4a9271ad 100644
--- a/tests/unittests/option_tests.py
+++ b/tests/unittests/option_tests.py
@@ -7,7 +7,7 @@
 # See the LICENSE file in the source distribution for further information.
 import unittest
 
-from sos.report.plugins import Plugin
+from sos.report.plugins import Plugin, PluginOpt
 from sos.policies.distros import LinuxPolicy
 from sos.policies.init_systems import InitSystem
 
@@ -21,6 +21,18 @@ class MockOptions(object):
     skip_files = []
 
 
+class MockPlugin(Plugin):
+
+    option_list = [
+        PluginOpt('baz', default=False),
+        PluginOpt('empty', default=None),
+        PluginOpt('test_option', default='foobar')
+    ]
+
+    def __init__(self, commons):
+        super(MockPlugin, self).__init__(commons=commons)
+
+
 class GlobalOptionTest(unittest.TestCase):
 
     def setUp(self):
@@ -30,11 +42,7 @@ class GlobalOptionTest(unittest.TestCase):
             'cmdlineopts': MockOptions(),
             'devices': {}
         }
-        self.plugin = Plugin(self.commons)
-        self.plugin.opt_names = ['baz', 'empty', 'test_option']
-        self.plugin.opt_parms = [
-            {'enabled': False}, {'enabled': None}, {'enabled': 'foobar'}
-        ]
+        self.plugin = MockPlugin(self.commons)
 
     def test_simple_lookup(self):
         self.assertEquals(self.plugin.get_option('test_option'), 'foobar')
diff --git a/tests/unittests/plugin_tests.py b/tests/unittests/plugin_tests.py
index 8dc038cb..fcd24143 100644
--- a/tests/unittests/plugin_tests.py
+++ b/tests/unittests/plugin_tests.py
@@ -12,7 +12,7 @@ import shutil
 
 from io import StringIO
 
-from sos.report.plugins import Plugin, regex_findall, _mangle_command
+from sos.report.plugins import Plugin, regex_findall, _mangle_command, PluginOpt
 from sos.archive import TarFileArchive
 from sos.policies.distros import LinuxPolicy
 from sos.policies.init_systems import InitSystem
@@ -64,8 +64,10 @@ class MockArchive(TarFileArchive):
 
 class MockPlugin(Plugin):
 
-    option_list = [("opt", 'an option', 'fast', None),
-                   ("opt2", 'another option', 'fast', False)]
+    option_list = [
+        PluginOpt("opt", default=None, desc='an option', val_type=str),
+        PluginOpt("opt2", default=False, desc='another option')
+    ]
 
     def setup(self):
         pass
@@ -271,35 +273,6 @@ class PluginTests(unittest.TestCase):
         })
         self.assertEquals(p.get_option("opt2", True), False)
 
-    def test_get_option_as_list_plugin_option(self):
-        p = MockPlugin({
-            'sysroot': self.sysroot,
-            'policy': LinuxPolicy(init=InitSystem(), probe_runtime=False),
-            'cmdlineopts': MockOptions(),
-            'devices': {}
-        })
-        p.set_option("opt", "one,two,three")
-        self.assertEquals(p.get_option_as_list("opt"), ['one', 'two', 'three'])
-
-    def test_get_option_as_list_plugin_option_default(self):
-        p = MockPlugin({
-            'sysroot': self.sysroot,
-            'policy': LinuxPolicy(init=InitSystem(), probe_runtime=False),
-            'cmdlineopts': MockOptions(),
-            'devices': {}
-        })
-        self.assertEquals(p.get_option_as_list("opt", default=[]), [])
-
-    def test_get_option_as_list_plugin_option_not_list(self):
-        p = MockPlugin({
-            'sysroot': self.sysroot,
-            'policy': LinuxPolicy(init=InitSystem(), probe_runtime=False),
-            'cmdlineopts': MockOptions(),
-            'devices': {}
-        })
-        p.set_option("opt", "testing")
-        self.assertEquals(p.get_option_as_list("opt"), ['testing'])
-
     def test_copy_dir(self):
         self.mp._do_copy_path("tests")
         self.assertEquals(