nagios-charmers team mailing list archive

[Joel Sing <joel.sing@xxxxxxxxxxxxx>]

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.