livepatch-charmers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Review: Approve

Yup. Comments inline. 

Diff comments:

> diff --git a/reactive/canonical_livepatch.py b/reactive/canonical_livepatch.py
> index 03c1903..be6e7bf 100644
> --- a/reactive/canonical_livepatch.py
> +++ b/reactive/canonical_livepatch.py
> @@ -216,6 +237,7 @@ def init_key():
>  def update_key():
>      # handle regular config-changed hooks for the livepatch key
>      activate_livepatch()
> +    write_status_to_disk()

Kind of annoying that this is a side effect of a handler named update_key, rather than as an expected step of the handler. But I can't think of a better handler name to convey that. And breaking this single line into a separate handler would be annoying.

>  
>  
>  @when('snap.installed.canonical-livepatch', 'config.changed.snap_channel')
> @@ -238,6 +260,10 @@ def configure_nagios(nagios):
>      hostname = nrpe.get_nagios_hostname()
>      nrpe_setup = nrpe.NRPE(hostname=hostname, primary=False)
>  
> +    # In a race with nrpe, create /var/lib/nagios if nrpe didn't do it yet
> +    if not path.exists('/var/lib/nagios'):
> +        mkdir('/var/lib/nagios')

This is safe? Other charms return early if the directory doesn't exist, trusting that it will exist in a future hook. You would need a trigger though, because the config.changed.snap_channel flag is ephemeral and won't be set in the future hook.

If just creating the directory as root works fine when the race is one, and the nagios package gets installed as expected and ownership and permissions corrected, what you have is probably better since it is a less confusing approach.

> +
>      # install nagios support files
>      file_to_units(
>          'files/check_canonical-livepatch.cron',


-- 
https://code.launchpad.net/~barryprice/canonical-livepatch-charm/+git/canonical-livepatch-charm/+merge/355912
Your team Livepatch charm developers is subscribed to branch canonical-livepatch-charm:master.