livepatch-charmers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Review: Approve

Yup. Comments inline, but its a matter of taste.

Diff comments:

> diff --git a/reactive/canonical_livepatch.py b/reactive/canonical_livepatch.py
> index 03c1903..4a542da 100644
> --- a/reactive/canonical_livepatch.py
> +++ b/reactive/canonical_livepatch.py
> @@ -53,19 +53,23 @@ def unit_update(status=None, message=None):
>          hookenv.status_set('active', 'Running kernel {}, patchState: {}'.format(*patch_details))
>  
>  
> -def get_patch_details():
> -    kernel = patch_state = 'unknown'
> -
> +def get_livepatch_status():
>      cmd = ['/snap/bin/canonical-livepatch', 'status']
>      try:
> -        livepatch_state = check_output(cmd, universal_newlines=True)
> +        livepatch_status = check_output(cmd, universal_newlines=True)
>      except CalledProcessError as e:
>          hookenv.log('Unable to get status: {}'.format(str(e)), hookenv.ERROR)
> -        return kernel, patch_state
> +        return None
> +    return livepatch_status
> +
> +
> +def get_patch_details():
> +    kernel = patch_state = 'unknown'
> +    patch_state = get_livepatch_status()

its status everywhere else, no patch_status is better than patch_state here.

>  
>      # status will usually pass YAML (but not always!)
>      try:
> -        status_yaml = safe_load(livepatch_state)
> +        status_yaml = safe_load(patch_state)

_yaml makes me think it contains a yaml encoded string. I personally would do 'raw_status = get_livepatch_status()' and 'status = safe_load(raw_status)'.

>      except Exception:
>          hookenv.log('Unable to parse status yaml', hookenv.ERROR)
>          return kernel, patch_state


-- 
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.