nagios-charmers team mailing list archive

[Peter Sabaini <peter.sabaini@xxxxxxxxxxxxx>]

Review: Approve

LGTM, some nits but nothing blocking imo

Diff comments:

> diff --git a/bin/charm_helpers_sync.py b/bin/charm_helpers_sync.py
> index bd79460..7c0c194 100644
> --- a/bin/charm_helpers_sync.py
> +++ b/bin/charm_helpers_sync.py
> @@ -39,10 +39,16 @@ def parse_config(conf_file):
>      return yaml.load(open(conf_file).read())
>  
>  
> -def clone_helpers(work_dir, branch):
> +def clone_helpers(work_dir, repo):
>      dest = os.path.join(work_dir, 'charm-helpers')
> -    logging.info('Checking out %s to %s.' % (branch, dest))
> -    cmd = ['bzr', 'checkout', '--lightweight', branch, dest]
> +    logging.info('Cloning out %s to %s.' % (repo, dest))

Nit: could use interpolation from the logging module
logging.info('Cloning out %s to %s.', repo, dest)

> +    branch = None
> +    if '@' in repo:
> +        repo, branch = repo.split('@', 1)
> +    cmd = ['git', 'clone', '--depth=1']
> +    if branch is not None:
> +        cmd += ['--branch', branch]
> +    cmd += [repo, dest]
>      subprocess.check_call(cmd)
>      return dest
>  
> @@ -174,6 +180,9 @@ def extract_options(inc, global_options=None):
>  
>  
>  def sync_helpers(include, src, dest, options=None):
> +    if os.path.exists(dest):
> +        logging.debug('Removing existing directory: %s' % dest)

Nit as above: could use interpolation from the logging module

> +        shutil.rmtree(dest)
>      if not os.path.isdir(dest):
>          os.makedirs(dest)
>  
> diff --git a/hooks/common.py b/hooks/common.py
> index 66d41ec..c2280a3 100644
> --- a/hooks/common.py
> +++ b/hooks/common.py
> @@ -43,6 +43,12 @@ def check_ip(n):
>              return False
>  
>  
> +def ingress_address(relation_data):
> +    if 'ingress-address' in relation_data:
> +        return relation_data['ingress-address']
> +    return relation_data['private-address']

Nit: somewhat shorter:
return relation_data.get('ingress-address', relation_data['private-address'])

> +
> +
>  def get_local_ingress_address(binding='website'):
>      # using network-get to retrieve the address details if available.
>      log('Getting hostname for binding %s' % binding)
> diff --git a/hooks/monitors-relation-changed b/hooks/monitors-relation-changed
> index 13cb96c..e16589d 100755
> --- a/hooks/monitors-relation-changed
> +++ b/hooks/monitors-relation-changed
> @@ -18,17 +18,77 @@
>  
>  import sys
>  import os
> -import subprocess
>  import yaml
> -import json
>  import re
> -
> -
> -from common import (customize_service, get_pynag_host,
> -        get_pynag_service, refresh_hostgroups,
> -        get_valid_relations, get_valid_units,
> -        initialize_inprogress_config, flush_inprogress_config,
> -        get_local_ingress_address)
> +from collections import defaultdict
> +
> +from charmhelpers.core.hookenv import (
> +    relation_get,
> +    ingress_address,
> +    related_units,
> +    relation_ids,
> +    log,
> +    DEBUG
> +)
> +
> +from common import (
> +    customize_service,
> +    get_pynag_host,
> +    get_pynag_service,
> +    refresh_hostgroups,
> +    initialize_inprogress_config,
> +    flush_inprogress_config
> +)
> +
> +
> +REQUIRED_REL_DATA_KEYS = [
> +    'target-address',
> +    'monitors',
> +    'target-id',
> +]
> +
> +
> +def _prepare_relation_data(unit, rid):
> +    relation_data = relation_get(unit=unit, rid=rid)
> +
> +    if not relation_data:
> +        msg = (
> +            'no relation data found for unit {} in relation {} - '
> +            'skipping'.format(unit, rid)
> +        )
> +        log(msg, level=DEBUG)
> +        return {}
> +
> +    if rid.split(':')[0] == 'nagios':
> +        # Fake it for the more generic 'nagios' relation'

OCD: unbalanced single quote

> +        relation_data['target-id'] = unit.replace('/', '-')
> +        relation_data['monitors'] = {'monitors': {'remote': {}}}
> +
> +    if not relation_data.get('target-address'):
> +        relation_data['target-address'] = ingress_address(unit=unit, rid=rid)
> +
> +    for key in REQUIRED_REL_DATA_KEYS:
> +        if not relation_data.get(key):
> +            msg = (
> +                '{} not found for unit {} in relation {} - '
> +                'skipping'.format(key, unit, rid)
> +            )
> +            log(msg, level=DEBUG)

So if any of target-id, monitors is missing we skip the relation... Might warrant higher level, maybe log(level=WARNING)?

> +            return {}
> +
> +    return relation_data
> +
> +
> +def _collect_relation_data():
> +    all_relations = defaultdict(dict)
> +    for relname in ['nagios', 'monitors']:
> +        for relid in relation_ids(relname):
> +            for unit in related_units(relid):
> +                relation_data = _prepare_relation_data(unit=unit, rid=relid)
> +                if relation_data:
> +                    all_relations[relid][unit] = relation_data
> +
> +    return all_relations
>  
>  
>  def main(argv):


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