From 0b6dc021b7ae54688fca70168c70744ad752834a Mon Sep 17 00:00:00 2001 From: Jake Hunsaker Date: Wed, 11 Aug 2021 17:11:17 -0400 Subject: [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 --- sos/report/__init__.py | 61 ++++++++------ sos/report/plugins/__init__.py | 157 +++++++++++++++++++++++------------ tests/unittests/conformance_tests.py | 5 +- tests/unittests/option_tests.py | 20 +++-- tests/unittests/plugin_tests.py | 37 ++------- 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( -- cgit