Review: Approve +1 LGTM for standards/readability - please review and address comments inline before merging.
Also, please avoid resubmitting merge proposals unless absolutely necessary (it makes review more difficult). Diff comments: > diff --git a/src/actions/actions.py b/src/actions/actions.py > new file mode 100755 > index 0000000..820f76d > --- /dev/null > +++ b/src/actions/actions.py > @@ -0,0 +1,59 @@ > +#!/usr/bin/env python3 > +# > +# Copyright 2016,2019 Canonical Ltd > +# > +# Licensed under the Apache License, Version 2.0 (the "License"); > +# you may not use this file except in compliance with the License. > +# You may obtain a copy of the License at > +# > +# http://www.apache.org/licenses/LICENSE-2.0 > +# > +# Unless required by applicable law or agreed to in writing, software > +# distributed under the License is distributed on an "AS IS" BASIS, > +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > +# See the License for the specific language governing permissions and > +# limitations under the License. > + > +import os > +import subprocess > +import sys > + > +from charmhelpers.core.hookenv import action_set, action_fail, log > + > + > +def clear_sel(): > + """ Nit: Summary should be on the same line as the """ - see https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings > + Action to clear the IPMI system event log. > + > + Uses ipmi-sel --post-clear, clears the SEL log and stores the cleared > entries > + in action output. > + """ > + command = ['/usr/sbin/ipmi-sel', '--post-clear'] > + try: > + output = subprocess.check_output(command) > + log("Action clear-sel completed, sel log cleared: {}".format(output)) > + action_set({"cleared_log": output}) > + except subprocess.CalledProcessError as e: > + action_fail("Action failed with {}".format(e)) > + > + > +# A dictionary of all the defined actions to callables (which take > +# parsed arguments). > +ACTIONS = {"clear-sel": clear_sel} > + > + > +def main(args): > + action_name = os.path.basename(args[0]) > + try: This seems an excessive use of a try block (and the else: further decreases readability) - I'd suggest just doing something like: if action_name not in ACTIONS: return "Action {} undefined".format(action_name) try: ACTIONS[action_name](args) except Exception as e: action_fail(str(e)) > + action = ACTIONS[action_name] > + except KeyError: > + return "Action {} undefined".format(action_name) > + else: > + try: > + action(args) > + except Exception as e: > + action_fail(str(e)) > + > + > +if __name__ == "__main__": > + sys.exit(main(sys.argv)) > diff --git a/src/lib/utils/tooling.py b/src/lib/utils/tooling.py > index 3b9b927..21794a5 100644 > --- a/src/lib/utils/tooling.py > +++ b/src/lib/utils/tooling.py > @@ -29,26 +29,43 @@ TOOLING = { > 'filename': 'check_mdadm.py', > 'cronjob': 'cron_mdadm.py' > }, > + 'ipmi': { > + 'filename': 'ipmi/check_ipmi_sensor', > + 'sudoers': 'ipmi/check_ipmi_sensor_sudoer' > + }, > } > > PLUGINS_DIR = '/usr/local/lib/nagios/plugins/' > TOOLS_DIR = '/usr/local/bin/' > CRONJOB_PATH = '/etc/cron.d/hw_health_{}' > +SUDOERS_DIR = '/etc/sudoers.d' > > > def install_resource(tools, resource): > - ntools = len(tools) > - if ntools == 0: > + """ > + Install hardware diagnostic tools from a charm resource Same re """ and summary. > + > + :param tools: set of tools to include > + :param resource: resource object > + :return: boolean, status of installation > + """ > + if not isinstance(tools, set): > + try: > + tools = set(tools) > + except TypeError as e: > + hookenv.log("exception found with install_resources, > {}".format(e)) "exception found" seems strange - is this really "invalid install_resources"? > + return False > + if not tools: > return False > - elif 'mdadm' in tools: > - tools = [tool for tool in tools if tool != 'mdadm'] > - ntools -= 1 > - if ntools == 0: > + elif tools & {'mdadm', 'ipmi'}: > + tools = tools - {'mdadm', 'ipmi'} > + if not tools: > return True > > if not isinstance(resource, str) \ > or not resource.endswith('.zip') \ > or not os.path.exists(resource): > + hookenv.log("The resource 'tools' does not end with .zip or does not > exist") > return False > > with tempfile.TemporaryDirectory() as tmpdir: > @@ -79,10 +96,14 @@ def install_resource(tools, resource): > return False > > > -def install_tools(tools, method=None): > - if not method: > - method = 'snap' > +def install_tools(tools, method='snap'): > + """ Ditto. > + Install hardware management tools from a snap > > + :param tools: list of tools to install > + :param method: default to snap if unspecified, other means have yet to > be added > + :return: > + """ > count = 0 > for tool in tools: > if tool in TOOLING and method in TOOLING[tool]: > diff --git a/src/tests/unit/test_cron_mdadm.py > b/src/tests/unit/test_cron_mdadm.py > index dffd65b..016745e 100644 > --- a/src/tests/unit/test_cron_mdadm.py > +++ b/src/tests/unit/test_cron_mdadm.py > @@ -29,10 +29,13 @@ class TestCronMdadm(unittest.TestCase): > > @mock.patch('os.path.exists') > @mock.patch('subprocess.Popen') > - def test_get_devices_mdadm_exception(self, mdadm_details, path_exists): > + @mock.patch('sys.exit') > + def test_get_devices_mdadm_exception(self, mock_exit, mdadm_details, This seems to be under 120 chars unwrapped. > + path_exists): > path_exists.return_value = True > mdadm_details.side_effect = subprocess.CalledProcessError( > 1, 'unittest') > + mock_exit.return_value = 0 > self.assertEqual(cron_mdadm.get_devices(), set()) > > @mock.patch('cron_mdadm.generate_output') -- https://code.launchpad.net/~xavpaice/hw-health-charm/+git/hw-health-charm/+merge/363682 Your team Nagios Charm developers is subscribed to branch hw-health-charm:master. -- Mailing list: https://launchpad.net/~nagios-charmers Post to : [email protected] Unsubscribe : https://launchpad.net/~nagios-charmers More help : https://help.launchpad.net/ListHelp
