nagios-charmers team mailing list archive

[Chris Sanders <chris.sanders@xxxxxxxxxxxxx>]

Review: Needs Fixing

A few comments inline, and while I hate to do this I think the charmhelpers and this change need to be split. While reviewing it I'm not convinced that this merge isn't confusing local vs charmhelpers functions. For example 'ingress_address' is defined in this change and I *believe* is only actually used from charmhelpers.

If I'm just having difficulty understanding and you *do* think the change is dependent on charmhelpers you can make this MR dependent on the charmhelpers MR so the changes specific to this bug can be reviewed seperately.

Diff comments:

> 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']

I'm struggling to see this function used anywhere. Why is this being added?

> +
> +
>  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/install b/hooks/install
> index f002e46..a8900a3 100755
> --- a/hooks/install
> +++ b/hooks/install
> @@ -29,7 +29,7 @@ echo nagios3-cgi nagios3/adminpassword password $PASSWORD | debconf-set-selectio
>  echo nagios3-cgi nagios3/adminpassword-repeat password $PASSWORD | debconf-set-selections
>  
>  DEBIAN_FRONTEND=noninteractive apt-get -qy \
> -  install nagios3 nagios-plugins python-cheetah python-jinja2 dnsutils debconf-utils nagios-nrpe-plugin pynag python-apt python-yaml
> +  install nagios3 nagios-plugins python-cheetah python-jinja2 dnsutils debconf-utils nagios-nrpe-plugin pynag python-apt python-yaml python-enum34

Are we installing python-enum34 to support charmhelpers? I don't see this used in this diff other than charmhelpers.

>  
>  scripts/postfix_loopback_only.sh
>  


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