livepatch-charmers team mailing list archive

[Paul Gear <paul.gear@xxxxxxxxxxxxx>]

Review: Approve

Some stylistic comments inline, but LGTM.

Diff comments:

> diff --git a/reactive/canonical_livepatch.py b/reactive/canonical_livepatch.py
> index bb9d547..63aef4d 100644
> --- a/reactive/canonical_livepatch.py
> +++ b/reactive/canonical_livepatch.py
> @@ -42,7 +41,10 @@ def wait_for_path(file_path, timeout=30):
>          sleep(interval)
>          seconds_waited += interval
>          if seconds_waited >= timeout:
> -            hookenv.log('Giving up waiting for {}'.format(file_path))
> +            hookenv.log(
> +                'Giving up waiting for {}'.format(file_path),
> +                hookenv.ERROR
> +            )

These are ugly, and I think it's fine that we ignore the line length limit to keep a log message to 1 line; ditto for various other examples throughout.

>              return
>      return
>  
> @@ -57,37 +59,44 @@ def unit_update(status=None, message=None):
>      if status and message:
>          hookenv.status_set(status, message)
>      else:
> -        hookenv.status_set('active', 'Effective kernel {}'.format(
> -            get_equiv_kernel_version())
> +        patch_details = get_patch_details()
> +        hookenv.status_set(
> +                'active',
> +                'Running kernel {}, patchState: {}'.format(*patch_details)
>          )
>  
>  
> -def get_equiv_kernel_version():
> -    # default to actual running kernel
> -    version_string = release()
> -    livepatch_status = ''
> +def get_patch_details():
> +    kernel = patch_state = 'unknown'
>  
>      cmd = ['/snap/bin/canonical-livepatch', 'status']
>      try:
> -        livepatch_status = check_output(cmd, universal_newlines=True)
> +        livepatch_state = check_output(cmd, universal_newlines=True)
>      except CalledProcessError as e:
> -        hookenv.log('Unable to get status: {}'.format(str(e)))
> -        return version_string
> +        hookenv.log('Unable to get status: {}'.format(str(e)), hookenv.ERROR)
> +        return(kernel, patch_state)

Needs space after return; ditto for other returns below.

>  
>      # status will usually pass YAML (but not always!)
>      try:
> -        status_yaml = load(livepatch_status)
> +        status_yaml = load(livepatch_state)
>      except Exception:
>          hookenv.log('Unable to parse status yaml')
> -        return version_string
> +        return(kernel, patch_state)
>  
>      # even if we got YAML, be paranoid
>      try:
> -        version_string = status_yaml['kernel']
> +        for k in status_yaml['status']:
> +            if k['running'] is True:
> +                kernel = k['kernel']
> +                patch_state = k['livepatch']['patchState']
>      except Exception:
> -        hookenv.log('Unable to find kernel line in status yaml')
> +        hookenv.log(
> +            'Unable to find patching details in status yaml',
> +            hookenv.ERROR
> +        )
> +        return(kernel, patch_state)
>  
> -    return version_string
> +    return(kernel, patch_state)
>  
>  
>  def get_yaml_if_exists(path_to_yaml):


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