Re: [edk2-staging/EdkRepo] [PATCH] EdkRepo: Add the manifest-repos command

["Ashley E Desimone" <ashley.e.desimone@intel.com>]

Hi Nate,

A V2 patchset has been sent please see an additional comment below regarding your feedback.

Thanks,
Ashley

-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> 
Sent: Sunday, May 10, 2020 11:45 PM
To: Desimone, Ashley E <ashley.e.desimone@intel.com>; devel@edk2.groups.io
Cc: Pandya, Puja <puja.pandya@intel.com>; Bjorge, Erik C <erik.c.bjorge@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Agyeman, Prince <prince.agyeman@intel.com>
Subject: RE: [edk2-staging/EdkRepo] [PATCH] EdkRepo: Add the manifest-repos command

Hi Ashley,

Please see comments inline below.

Thanks,
Nate

> -----Original Message-----
> From: Desimone, Ashley E <ashley.e.desimone@intel.com>
> Sent: Sunday, May 10, 2020 9:38 PM
> To: devel@edk2.groups.io
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Pandya, 
> Puja <puja.pandya@intel.com>; Bjorge, Erik C 
> <erik.c.bjorge@intel.com>; Bret Barkelew 
> <Bret.Barkelew@microsoft.com>; Agyeman, Prince 
> <prince.agyeman@intel.com>
> Subject: [edk2-staging/EdkRepo] [PATCH] EdkRepo: Add the 
> manifest-repos command
> 
> Add the manifest_repos_command to list, add, or remove manifest 
> repositories in the ekdrepo_user.cfg
> 
> Signed-off-by: Ashley E Desimone <ashley.e.desimone@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Puja Pandya <puja.pandya@intel.com>
> Cc: Erik Bjorge <erik.c.bjorge@intel.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Cc: Prince Agyeman <prince.agyeman@intel.com>
> ---
>  edkrepo/commands/arguments/manifest_repo_args.py |  21 +++++ 
> edkrepo/commands/humble/manifest_repos_humble.py |  21 +++++
>  edkrepo/commands/manifest_repos_command.py       | 106
> +++++++++++++++++++++++
>  3 files changed, 148 insertions(+)
>  create mode 100644
> edkrepo/commands/arguments/manifest_repo_args.py
>  create mode 100644
> edkrepo/commands/humble/manifest_repos_humble.py
>  create mode 100644 edkrepo/commands/manifest_repos_command.py
> 
> diff --git a/edkrepo/commands/arguments/manifest_repo_args.py
> b/edkrepo/commands/arguments/manifest_repo_args.py
> new file mode 100644
> index 0000000..b99d9d6
> --- /dev/null
> +++ b/edkrepo/commands/arguments/manifest_repo_args.py
> @@ -0,0 +1,21 @@
> +#!/usr/bin/env python3
> +#
> +## @file
> +# manifest_repo_args.py
> +#
> +# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR> #
> +SPDX-License-Identifier: BSD-2-Clause-Patent #
> +
> +''' Contains the help and description strings for arguments in the 
> +manifest_repo command meta data.
> +'''
> +
> +COMMAND_DESCRIPTION = 'Lists, adds or removes a manifest repository.'
> +LIST_HELP = 'List all available manifest repositories.'
> +ADD_HELP = 'Add a manifest repository.'
> +REMOVE_HELP = 'Remove a manifest repository.'
> +NAME_HELP = 'The name of a manifest repository to add/remove.
> Required with Add or Remove flags.'
> +URL_HELP = 'The URL of a manifest repository to add. Required with 
> +Add
> flag.'
> +LOCAL_PATH_HELP = 'The local path that a manifest is stored at in the
> edkrepo global data directory. Required with Add flag.'
> +BRANCH_HELP = 'The branch of a manifest repository to use. Required 
> +with
> Add flag.'
> diff --git a/edkrepo/commands/humble/manifest_repos_humble.py
> b/edkrepo/commands/humble/manifest_repos_humble.py
> new file mode 100644
> index 0000000..bc00796
> --- /dev/null
> +++ b/edkrepo/commands/humble/manifest_repos_humble.py
> @@ -0,0 +1,21 @@
> +#!/usr/bin/env python3
> +#
> +## @file
> +# manifest_repos_humble.py
> +#
> +# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> #

Update copyright year

> +SPDX-License-Identifier: BSD-2-Clause-Patent #
> +
> +'''
> +Contains user visible strings printed by the manifest_repos command.
> +'''
> +
> +CFG_LIST_ENTRY = 'Config File: edkrepo.cfg Manifest Repository Name: {}'
> +USER_CFG_LIST_ENTRY = 'Config File: edkrepo_user.cfg Manifest
> Repository Name: {}'
> +NAME_REQUIRED = 'The "name" argument is required to add/remove a
> manifest repository'
> +ADD_REQUIRED = 'The "name", "url", "branch" and "local-path" 
> +arguments
> are required to add a manifest repository'
> +ADD_REMOVE = 'The "add" and "remove" flags cannot be used at the same
> time.'
> +CANNOT_REMOVE_CFG = 'Manifest repositories cannot be removed from
> the edkrepo.cfg file.'
> +REMOVE_NOT_EXIST = 'The selected manifest repository does note exist 
> +in
> the edkrepo_user.cfg file.'
> +ALREADY_EXISTS = 'A manifest repository already exists with name: {}'
> diff --git a/edkrepo/commands/manifest_repos_command.py
> b/edkrepo/commands/manifest_repos_command.py
> new file mode 100644
> index 0000000..c43e9e6
> --- /dev/null
> +++ b/edkrepo/commands/manifest_repos_command.py
> @@ -0,0 +1,106 @@
> +#!/usr/bin/env python3
> +#
> +## @file
> +# ,amofest+_repos_command.py

Misspelling, should be manifest_repos_command.py

> +#
> +# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR> #
> +SPDX-License-Identifier: BSD-2-Clause-Patent #
> +
> +import configparser
> +
> +from edkrepo.commands.edkrepo_command import EdkrepoCommand
> import
> +edkrepo.commands.arguments.manifest_repo_args as arguments import 
> +edkrepo.commands.humble.manifest_repos_humble as humble from 
> +edkrepo.common.edkrepo_exception import 
> +EdkrepoInvalidParametersException from 
> +edkrepo.common.workspace_maintenance.manifest_repos_maintenance
> import
> +list_available_manifest_repos
> +
> +
> +
> +
> +class ManifestRepos(EdkrepoCommand):
> +    def __init__(self):
> +        super().__init__()
> +
> +    def get_metadata(self):
> +        metadata = {}
> +        metadata['name'] = 'manifest-repos'
> +        metadata['help-text'] = arguments.COMMAND_DESCRIPTION
> +        args = []
> +        metadata['arguments'] = args
> +        args.append({'name': 'list',
> +                     'positional': False,
> +                     'required': False,
> +                     'help-text': arguments.LIST_HELP})
> +        args.append({'name': 'add',
> +                     'positional': False,
> +                     'required': False,
> +                     'help-text': arguments.ADD_HELP})
> +        args.append({'name': 'remove',
> +                     'positional': False,
> +                     'required': False,
> +                     'help-text': arguments.REMOVE_HELP})

There is a better way to do this, use the "choice" parameter type.

For example:
        args.append({choice: 'list',
                     'parent': 'action',
                     'help-text': arguments.LIST_HELP})
        args.append({choice: 'add',
                     'parent': 'action',
                     'help-text': arguments.ADD_HELP})
        args.append({choice: 'remove',
                     'parent': 'action',
                     'help-text': arguments.REMOVE_HELP})
        args.append({'name': 'action',
                     'positional': True,
                     'position': 0,
                     'required': True,
                     'choices': True,
                     'help-text': arguments.ACTION_HELP})


> +        args.append({'name': 'name',
> +                     'positional': True,
> +                     'required': False,
> +                     'position': 0,
> +                     'nargs' : 1,
> +                     'help-text': arguments.NAME_HELP})
> +        args.append({'name': 'branch',
> +                     'positional': True,
> +                     'required': False,
> +                     'position': 2,
> +                     'nargs' : 1,
> +                     'help-text': arguments.BRANCH_HELP})
> +        args.append({'name': 'url',
> +                     'positional': True,
> +                     'required': False,
> +                     'position': 1,
> +                     'nargs' : 1,
> +                     'help-text': arguments.URL_HELP})
> +        args.append({'name': 'path',
> +                     'positional': True,
> +                     'required': False,
> +                     'position': 3,
> +                     'nargs' : 1,
> +                     'help-text': arguments.LOCAL_PATH_HELP})
> +        return metadata
> +
> +    def run_command(self, args, config):
> +        cfg_repos, user_cfg_repos, conflicts = 
> + list_available_manifest_repos(config['cfg_file'],
> + config['user_cfg_file'])
> +
> +        if args.list:
> +            for repo in cfg_repos:
> +                print(humble.CFG_LIST_ENTRY.format(repo))
> +            for repo in user_cfg_repos:
> +                print(humble.USER_CFG_LIST_ENTRY.format(repo))
> +
> +        if args.add and args.remove:
> +            raise EdkrepoInvalidParametersException(humble.ADD_REMOVE)
> +        elif (args.add or args.remove) and not args.name:
> +            raise EdkrepoInvalidParametersException(humble.NAME_REQUIRED)
> +        elif args.add and not (args.branch or args.url or args.local_path):
> +            raise 
> + EdkrepoInvalidParametersException(humble.ADD_REQUIRED)

All of the mess above goes away if you use choice.
Ashley- Not entirely there is no good built in method in arg parse to handle conditional mutual exclusivity of arguments. Even with choices used for list, add and remove the checks for whether name/branch/url/path are included with remove or add will need to remain, updated to reflect the change to using choice. 

> +        elif args.remove and args.name and args.name in cfg_repos:
> +            raise
> EdkrepoInvalidParametersException(humble.CANNOT_REMOVE_CFG)
> +        elif args.remove and args.name not in
> config['user_cfg_file'].manifest_repo_list:
> +            raise
> EdkrepoInvalidParametersException(humble.REMOVE_NOT_EXIST)
> +        elif args.add and (args.name in cfg_repos or args.name in
> user_cfg_repos):
> +            raise
> +
> EdkrepoInvalidParametersException(humble.ALREADY_EXISTS.format(args.n
> a
> + me))
> +
> +        user_cfg_file_path = config['user_cfg_file'].cfg_filename
> +
> +        if args.add or args.remove:
> +            user_cfg_file = configparser.ConfigParser(allow_no_value=True)
> +            user_cfg_file.read(user_cfg_file_path)
> +            if args.add:
> +                user_cfg_file.set('manifest-repos', args.name, None)
> +                user_cfg_file.add_section(args.name)
> +                user_cfg_file.set(args.name, 'URL', args.url)
> +                user_cfg_file.set(args.name, 'Branch', args.branch)
> +                user_cfg_file.set(args.name, 'LocalPath', args.path)
> +            if args.remove:
> +                user_cfg_file.remove_option('manifest-repos', args.name)
> +                user_cfg_file.remove_section(args.name)
> +            with open(user_cfg_file_path, 'w') as cfg_stream:
> +                user_cfg_file.write(cfg_stream)
> --
> 2.16.2.windows.1