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.
--
Mailing list: https://launchpad.net/~nagios-charmers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~nagios-charmers
More help : https://help.launchpad.net/ListHelp
