nagios-charmers team mailing list archive

[Paul Goins <paul.goins@xxxxxxxxxxxxx>]

Review: Approve

This looks okay, except that there's no tests.  I'd feel more comfortable if there were at least a functional test that verified that the file on disk was being created as expected when the extra_contacts config option was set.

There's not many functional tests right now in this charm, so I'm not ready to block the merge based upon that alone.  We can do a manual test to verify the functionality.

I left one comment regarding a part of code that had me scratching my head a tiny bit.  It made sense after re-reading, but could be made more readable, in my opinion.

Anyway, all that said: +1.

Diff comments:

> diff --git a/hooks/upgrade-charm b/hooks/upgrade-charm
> index 19e736c..86643e1 100755
> --- a/hooks/upgrade-charm
> +++ b/hooks/upgrade-charm
> @@ -52,6 +53,53 @@ def warn_legacy_relations():
>                  "WARNING")
>  
>  
> +# Parses a list of extra Nagios contacts from a YAML string
> +# Does basic sanitization only
> +def get_extra_contacts(yaml_string, log=False):
> +    # Final result
> +    extra_contacts = []
> +
> +    # Valid characters for contact names
> +    valid_name_chars = (
> +        'abcdefghijklmnopqrstuvwxyz'
> +        'ABCDEFGHIJKLMNOPQRSTUVWXYZ'
> +        '0123456789_-'
> +    )
> +
> +    try:
> +        extra_contacts_raw = yaml.load(yaml_string, Loader=yaml.SafeLoader) or []
> +        if not isinstance(extra_contacts_raw, list):
> +            raise ValueError('not a list')
> +
> +        required_fields = ['name', 'host', 'service']
> +        for contact in extra_contacts_raw:
> +            if {'name', 'host', 'service'} > set(contact.keys()):
> +                if log:
> +                    hookenv.log(
> +                        'Contact {} is missing fields.'.format(contact))
> +                continue
> +
> +            if not (set(contact['name']) < set(valid_name_chars)):

To avoid the "not" in the if check, this could be rewritten as:

    if (set(contact['name']) > set(valid_name_chars)):

I think that would be slightly more readable.

> +                if log:
> +                    hookenv.log(
> +                        'Contact name {} is illegal'.format(contact['name']))
> +                continue
> +
> +            if '\n' in (contact['host'] + contact['service']):
> +                if log:
> +                    hookenv.log('Line breaks not allowed in commands')
> +                continue
> +
> +            extra_contacts.append(contact)
> +
> +    except (ValueError, yaml.error.YAMLError), e:
> +        if log:
> +            hookenv.log(
> +                'Invalid "extra_contacts" configuration: {}'.format(e))
> +
> +    return extra_contacts
> +
> +
>  # If the charm has extra configuration provided, write that to the
>  # proper nagios3 configuration file, otherwise remove the config
>  def write_extra_config():


-- 
https://code.launchpad.net/~aggkolaitis/charm-nagios/+git/nagios-charm/+merge/374074
Your team Nagios Charm developers is subscribed to branch charm-nagios:master.