nagios-charmers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Review: Approve

This is fine. Some code can be simplified before landing by using config.changed('foo') instead of manually comparing config.get('foo', None) with config.previous('foo'). But I'm not going to block on this since it is matching the existing code.

Diff comments:

> diff --git a/hooks/actions.py b/hooks/actions.py
> index 1593b51..0323ae3 100644
> --- a/hooks/actions.py
> +++ b/hooks/actions.py
> @@ -91,15 +94,22 @@ def update_ppa(service_name):
>      config = hookenv.config()
>      new_source = config.get('source')
>      prev_source = config.previous('source')
> +
> +    new_key = config.get('key')
> +    prev_key = config.previous('key')
> +
>      if prev_source is not None and prev_source != new_source:
>          subprocess.check_call(['add-apt-repository',
>                                 '--yes', '--remove', prev_source],
>                                 env=hookenv.env_proxy_settings(['https']))
> -    add_source(config.get('source'), config.get('key', None))
> -    apt_update(fatal=True)
> -    package_list = ["thruk", "pwgen", "apache2-utils"]
> -    apt_install(packages=package_list, fatal=True)
> +    if prev_source != new_source or prev_key != new_key:

This code would be simpler if you used """ config.changed('source') or config.changed('key') """

> +        add_source(config.get('source'), config.get('key', None))
> +        apt_update(fatal=True)
> +
> +    pkgs_to_install = filter_installed_packages(PACKAGE_LIST)
> +    if pkgs_to_install:
> +        apt_install(packages=pkgs_to_install, fatal=True)
>  
>  
>  def update_status(service_name):
> -   hookenv.status_set('active', 'ready')
> +    hookenv.status_set('active', 'ready')


-- 
https://code.launchpad.net/~freyes/thruk-agent-charm/+git/thruk-agent-charm/+merge/376486
Your team Nagios Charm developers is requested to review the proposed merge of ~freyes/thruk-agent-charm:lp1829681 into thruk-agent-charm:master.