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