nagios-charmers team mailing list archive

[Rodrigo Barbieri <rodrigo.barbieri@xxxxxxxxxxxxx>]

Review: Needs Fixing

Thanks for your efforts in this fix Giulio! I added comments to the review as per our previous conversation about the improvements and validation that we've agreed to be addressed.

Diff comments:

> diff --git a/hooks/upgrade-charm b/hooks/upgrade-charm
> index 1016391..c1b7841 100755
> --- a/hooks/upgrade-charm
> +++ b/hooks/upgrade-charm
> @@ -38,6 +38,7 @@ pagerduty_cfg = "/etc/nagios3/conf.d/pagerduty_nagios.cfg"
>  pagerduty_cron = "/etc/cron.d/nagios-pagerduty-flush"
>  password = hookenv.config('password')
>  ro_password = hookenv.config('ro-password')
> +broker_module = hookenv.config('broker_module')

See comment below. You can either remove this line or use in line 280

>  
>  
>  # Checks the charm relations for legacy relations
> @@ -276,6 +277,7 @@ def update_config():
>                         'is_container': host.is_container(),
>                         'service_check_timeout': hookenv.config('service_check_timeout'),
>                         'service_check_timeout_state': hookenv.config('service_check_timeout_state'),
> +                       'broker_module': hookenv.config('broker_module'),

you can either use the broker_module variable you introduced earlier in line 41, or remove the broker_module variable declaration (line 41), as it is not currently being used.

>                         }

I see that we are passing whatever is set in the config directly to the config file template. We could validate here if the module file exists and raise an exception if it doesn't, so there is an immediate feedback from juju as soon as it tries to apply the config if the path to the file is incorrect. This saves time trying to debug why a module failed to load in Nagios in this situation.

>  
>      with open('hooks/templates/nagios-cfg.tmpl', 'r') as f:


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