nagios-charmers team mailing list archive

[Peter Sabaini <peter.sabaini@xxxxxxxxxxxxx>]

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.