[OE-core] [PATCH 1/3 v2] scripts/oe-git-archive: fix non-existent key referencing error

[Richard Purdie]

On Thu, 2019-01-03 at 15:23 +0800, Yeoh Ee Peng wrote:
> Without installing gitpython package, oe-git-archive will face error
> below, where it was referencing key that was non-existent inside
> metadata object.
> 
> Traceback (most recent call last):
>   File "<poky_dir>/scripts/oe-git-archive", line 271, in <module>
>     sys.exit(main())
>   File "<poky_dir>/scripts/oe-git-archive", line 229, in main
>     'commit_count': metadata['layers']['meta']['commit_count'],
> KeyError: 'commit_count'
> 
> Fix this error by checking the key existent from metadata before
> referencing it.
> 
> [YOCTO# 13082]
> 
> Signed-off-by: Yeoh Ee Peng <ee.peng.yeoh at intel.com>
> ---
>  scripts/oe-git-archive | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/scripts/oe-git-archive b/scripts/oe-git-archive
> index ab19cb9..80c2379 100755
> --- a/scripts/oe-git-archive
> +++ b/scripts/oe-git-archive
> @@ -1,4 +1,4 @@
> -#!/usr/bin/python3
> +#!/usr/bin/env python3
>  #
>  # Helper script for committing data to git and pushing upstream
>  #
> @@ -208,6 +208,18 @@ def parse_args(argv):
>                          help="Data to commit")
>      return parser.parse_args(argv)
>  
> +def _get_metadata_value(metadata, keys):
> +    if len(keys) > 0:
> +        key = keys.pop(0)
> +        if key in metadata:
> +            if len(keys) == 0:
> +                return metadata[key]
> +            else:
> +                return _get_metadata_value(metadata[key], keys)
> +        else:
> +            return ""
> +    else:
> +        return ""

This isn't very pythonic unfortunately. You can do "if X:" instead of
"if len(X)" or "if len(X) > 0:". You can also remove the else: return
"" and just put return "" at the end of the function.

I also think the recursion here is hard to understand and python has a
better way of handling this, python tends to work on the "forgiveness"
rather than "permission" approach to code. By that I mean:

try:
    return metadata[key]
except KeyError:
    return ""

is more "pythonic" than:

if key in metadata:
    return metadata[key]
else:
    return ""

(for dicts you can also do metadata.get(key, "") )

>  def main(argv=None):
>      """Script entry point"""
> @@ -223,11 +235,15 @@ def main(argv=None):
>  
>          # Get keywords to be used in tag and branch names and
> messages
>          metadata = metadata_from_bb()
> -        keywords = {'hostname': metadata['hostname'],
> -                    'branch': metadata['layers']['meta']['branch'],
> -                    'commit': metadata['layers']['meta']['commit'],
> -                    'commit_count':
> metadata['layers']['meta']['commit_count'],
> -                    'machine': metadata['config']['MACHINE']}
> +        keywords_map = {'hostname': ['hostname'],
> +                        'branch': ['layers', 'meta', 'branch'],
> +                        'commit': ['layers', 'meta', 'commit'],
> +                        'commit_count': ['layers', 'meta',
> 'commit_count'],
> +                        'machine': ['config', 'MACHINE']}
> +        keywords = {}
> +        for map_key in keywords_map.keys():
> +            keywords_value = _get_metadata_value(metadata,
> keywords_map[map_key])
> +            keywords[map_key] = keywords_value
>  
>          # Expand strings early in order to avoid getting into
> inconsistent
>          # state (e.g. no tag even if data was committed)


How about something like:

def get_nested(d, list_of_keys):
    try:
        for k in list_of_keys:
            d = d[k]
        return d
    except KeyError:
        return ""

keywords = {'hostname': get_nested(metadata, ['hostname']),
            'branch': get_nested(metadata, ['layers', 'meta', 'branch']),
            'commit': get_nested(metadata, ['layers', 'meta', 'commit']),
            'commit_count': get_nested(metadata, ['layers', 'meta', 'commit_count']),
            'machine': get_nested(metadata, ['config', 'MACHINE'])}

Cheers,

Richard