Review: Approve Heya, thanks for the update. There's some minor things inline -- I'm ok with raising bugs too to limit scope though.
Diff comments: > diff --git a/src/actions/actions.py b/src/actions/actions.py > new file mode 100755 > index 0000000..29d12cc > --- /dev/null > +++ b/src/actions/actions.py > @@ -0,0 +1,57 @@ > +#!/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: it's recommended multiline docstrings have a summary line, blank line, then details (cf. pep-0257 which pep-0008 references). E.g.: """Action to clear the IPMI system event log Prints the log before clearing it. """ Similarly with other docstrings below. > + Action to clear the IPMI system event log, prints the content > + of the log before clearing it. > + """ > + 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: > + 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..47f9d0e 100644 > --- a/src/lib/utils/tooling.py > +++ b/src/lib/utils/tooling.py > @@ -29,26 +29,42 @@ 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 > + :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)) > + return False > + if len(tools) == 0: Maybe lose the superfluous len() as the empty set should eval to False? Similarly below > 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 len(tools) == 0: > 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: > diff --git a/src/tox.ini b/src/tox.ini > index ddf16bf..491fbe6 100644 > --- a/src/tox.ini > +++ b/src/tox.ini > @@ -8,6 +8,14 @@ envdir={toxworkdir}/py3 > deps= > charms.reactive > nose > + mock > commands = > {toxworkdir}/../tests/download_nagios_plugin3.py > nosetests tests/unit > + > +[testenv:pep8] > +basepython = python3 > +deps = > + flake8 > +commands = flake8 {posargs} --max-complexity=20 --max-line-length=120 > reactive files unit_tests The IS dev process document requires max-complexity=10. Now, with max-complexity=10 flake8 complains about files/{cron_mdadm.py,megaraid/check_megacli.py} which are not part of this review. Maybe just set max-complexity=10 for visibility and have a sep. commit to simplify those functions > + -- https://code.launchpad.net/~xavpaice/hw-health-charm/+git/hw-health-charm/+merge/363638 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
