From bc89c9bafa3175ba0a349032131793c596ed106b Mon Sep 17 00:00:00 2001 From: Sjoerd Langkemper Date: Sun, 21 Jan 2024 17:28:16 +0100 Subject: [PATCH 1/3] fix(cli): allow exclusive arguments as optional The CLI takes its arguments from the RequiredOptional, which has three fields: required, optional, and exclusive. In practice, the exclusive options are not defined as either required or optional, and would not be allowed in the CLI. This changes that, so that exclusive options are also added to the argument parser. Closes #2769 --- gitlab/v4/cli.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gitlab/v4/cli.py b/gitlab/v4/cli.py index 4bfa5ea93..bde3c0fc8 100644 --- a/gitlab/v4/cli.py +++ b/gitlab/v4/cli.py @@ -258,7 +258,7 @@ def _populate_sub_parser_by_class( sub_parser_action.add_argument( f"--{x.replace('_', '-')}", required=True ) - for x in mgr_cls._create_attrs.optional: + for x in mgr_cls._create_attrs.optional + mgr_cls._create_attrs.exclusive: sub_parser_action.add_argument( f"--{x.replace('_', '-')}", required=False ) @@ -274,7 +274,7 @@ def _populate_sub_parser_by_class( f"--{x.replace('_', '-')}", required=True ) - for x in mgr_cls._update_attrs.optional: + for x in mgr_cls._update_attrs.optional + mgr_cls._update_attrs.exclusive: if x != cls._id_attr: sub_parser_action.add_argument( f"--{x.replace('_', '-')}", required=False From 251ac1a0b6c018296f099783698ae0c3b58374c8 Mon Sep 17 00:00:00 2001 From: Sjoerd Langkemper Date: Sun, 28 Jan 2024 21:31:03 +0100 Subject: [PATCH 2/3] fix(cli): inform argument parser that options are mutually exclusive --- gitlab/v4/cli.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/gitlab/v4/cli.py b/gitlab/v4/cli.py index bde3c0fc8..da8d659da 100644 --- a/gitlab/v4/cli.py +++ b/gitlab/v4/cli.py @@ -258,10 +258,14 @@ def _populate_sub_parser_by_class( sub_parser_action.add_argument( f"--{x.replace('_', '-')}", required=True ) - for x in mgr_cls._create_attrs.optional + mgr_cls._create_attrs.exclusive: + for x in mgr_cls._create_attrs.optional: sub_parser_action.add_argument( f"--{x.replace('_', '-')}", required=False ) + if mgr_cls._create_attrs.exclusive: + group = sub_parser_action.add_mutually_exclusive_group() + for x in mgr_cls._create_attrs.exclusive: + group.add_argument(f"--{x.replace('_', '-')}") if action_name == "update": if cls._id_attr is not None: @@ -274,12 +278,17 @@ def _populate_sub_parser_by_class( f"--{x.replace('_', '-')}", required=True ) - for x in mgr_cls._update_attrs.optional + mgr_cls._update_attrs.exclusive: + for x in mgr_cls._update_attrs.optional: if x != cls._id_attr: sub_parser_action.add_argument( f"--{x.replace('_', '-')}", required=False ) + if mgr_cls._create_attrs.exclusive: + group = sub_parser_action.add_mutually_exclusive_group() + for x in mgr_cls._create_attrs.exclusive: + group.add_argument(f"--{x.replace('_', '-')}") + if cls.__name__ in cli.custom_actions: name = cls.__name__ for action_name in cli.custom_actions[name]: From 04171d06d8edb113d104bb1e503623f44730e424 Mon Sep 17 00:00:00 2001 From: Sjoerd Langkemper Date: Mon, 29 Jan 2024 14:27:26 +0100 Subject: [PATCH 3/3] fix(cli): use correct exclusive options, add unit test --- gitlab/v4/cli.py | 4 +-- tests/unit/test_cli.py | 61 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/gitlab/v4/cli.py b/gitlab/v4/cli.py index da8d659da..c6159bc26 100644 --- a/gitlab/v4/cli.py +++ b/gitlab/v4/cli.py @@ -284,9 +284,9 @@ def _populate_sub_parser_by_class( f"--{x.replace('_', '-')}", required=False ) - if mgr_cls._create_attrs.exclusive: + if mgr_cls._update_attrs.exclusive: group = sub_parser_action.add_mutually_exclusive_group() - for x in mgr_cls._create_attrs.exclusive: + for x in mgr_cls._update_attrs.exclusive: group.add_argument(f"--{x.replace('_', '-')}") if cls.__name__ in cli.custom_actions: diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index e40440ed6..eaa3908b5 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -11,6 +11,8 @@ import gitlab.base from gitlab import cli from gitlab.exceptions import GitlabError +from gitlab.mixins import CreateMixin, UpdateMixin +from gitlab.types import RequiredOptional from gitlab.v4 import cli as v4_cli @@ -157,6 +159,65 @@ def test_v4_parser(): assert actions["--name"].required +def test_extend_parser(): + class ExceptionArgParser(argparse.ArgumentParser): + def error(self, message): + "Raise error instead of exiting on invalid arguments, to make testing easier" + raise ValueError(message) + + class Fake: + _id_attr = None + + class FakeManager(gitlab.base.RESTManager, CreateMixin, UpdateMixin): + _obj_cls = Fake + _create_attrs = RequiredOptional( + required=("create",), + optional=("opt_create",), + exclusive=("create_a", "create_b"), + ) + _update_attrs = RequiredOptional( + required=("update",), + optional=("opt_update",), + exclusive=("update_a", "update_b"), + ) + + parser = ExceptionArgParser() + with mock.patch.dict( + "gitlab.v4.objects.__dict__", {"FakeManager": FakeManager}, clear=True + ): + v4_cli.extend_parser(parser) + + assert parser.parse_args(["fake", "create", "--create", "1"]) + assert parser.parse_args(["fake", "create", "--create", "1", "--opt-create", "1"]) + assert parser.parse_args(["fake", "create", "--create", "1", "--create-a", "1"]) + assert parser.parse_args(["fake", "create", "--create", "1", "--create-b", "1"]) + + with pytest.raises(ValueError): + # missing required "create" + parser.parse_args(["fake", "create", "--opt_create", "1"]) + + with pytest.raises(ValueError): + # both exclusive options + parser.parse_args( + ["fake", "create", "--create", "1", "--create-a", "1", "--create-b", "1"] + ) + + assert parser.parse_args(["fake", "update", "--update", "1"]) + assert parser.parse_args(["fake", "update", "--update", "1", "--opt-update", "1"]) + assert parser.parse_args(["fake", "update", "--update", "1", "--update-a", "1"]) + assert parser.parse_args(["fake", "update", "--update", "1", "--update-b", "1"]) + + with pytest.raises(ValueError): + # missing required "update" + parser.parse_args(["fake", "update", "--opt_update", "1"]) + + with pytest.raises(ValueError): + # both exclusive options + parser.parse_args( + ["fake", "update", "--update", "1", "--update-a", "1", "--update-b", "1"] + ) + + @pytest.mark.skipif(sys.version_info < (3, 8), reason="added in 3.8") def test_legacy_display_without_fields_warns(fake_object_no_id): printer = v4_cli.LegacyPrinter()