From 54035bcd4fdadd0c48e4fcf65be1dd2d26404512 Mon Sep 17 00:00:00 2001 From: Chi Wai Chan Date: Thu, 29 Aug 2024 12:59:56 +0800 Subject: [PATCH 1/2] Fix --purge / --purge-before-date dependency issue. Currently, the dependency check only works if `--purge` is specified first. If the `--purge` is after `--purge-before-date`, the dependency check action still report missing require args `--purge`. For example, `cou plan --purge-before-date 2000-1-1 --purge` will still failed. This PR fixed that issue. --- cou/commands.py | 32 ++++++++++++-------------------- tests/unit/test_commands.py | 26 ++------------------------ 2 files changed, 14 insertions(+), 44 deletions(-) diff --git a/cou/commands.py b/cou/commands.py index 70cd28ca..e021ec1b 100644 --- a/cou/commands.py +++ b/cou/commands.py @@ -128,23 +128,6 @@ def purge_before_arg(value: str) -> str: raise argparse.ArgumentTypeError("purge before format must be YYYY-MM-DD[HH:mm][:ss]") -class PurgeBeforeArgumentAction(argparse.Action): - """Custom action to make sure the arguments dependency.""" - - required_arg = "purge" - - def __call__( - self, - parser: argparse.ArgumentParser, - namespace: argparse.Namespace, - values: Any, - option_string: Optional[str] = None, - ) -> None: - if getattr(namespace, self.required_arg) is False: - parser.error(f"--{self.dest} requires --{self.required_arg}") - setattr(namespace, self.dest, values) - - def get_subcommand_common_opts_parser() -> argparse.ArgumentParser: """Create a shared parser for options specific to subcommands. @@ -185,8 +168,10 @@ def get_subcommand_common_opts_parser() -> argparse.ArgumentParser: ) subcommand_common_opts_parser.add_argument( "--archive-batch-size", - help="Batch size for nova old database data archiving.\n" - "Decrease the batch size if performance issues are detected.\n(default: 1000)", + help=( + "Batch size for nova old database data archiving." + "\nDecrease the batch size if performance issues are detected.\n(default: 1000)" + ), type=batch_size_arg, default=1000, ) @@ -199,12 +184,12 @@ def get_subcommand_common_opts_parser() -> argparse.ArgumentParser: subcommand_common_opts_parser.add_argument( "--purge-before-date", dest="purge_before", - action=PurgeBeforeArgumentAction, help=( "Providing this argument will delete data from all shadow tables" "\nthat is older than the date provided." "\nDate string format should be YYYY-MM-DD[HH:mm][:ss]." "\nWithout --purge-before-date the purge step will delete all the data." + "\nThis option requires --purge." ), type=purge_before_arg, required=False, @@ -555,6 +540,13 @@ def parse_args(args: Any) -> CLIargs: # pylint: disable=inconsistent-return-sta subparsers.choices["upgrade"].print_help() parser.exit() + # validate arguments + validation_errors = [] + if parsed_args.purge_before and not getattr(parsed_args, "purge", None): + validation_errors.append("--purge-before-date requires --purge") + if len(validation_errors) > 0: + parser.error("\n" + "\n".join(validation_errors)) + return parsed_args except argparse.ArgumentError as exc: parser.error( diff --git a/tests/unit/test_commands.py b/tests/unit/test_commands.py index ad558dac..e9b8605e 100644 --- a/tests/unit/test_commands.py +++ b/tests/unit/test_commands.py @@ -935,31 +935,9 @@ def test_purge_before_arg(val, raise_err): @patch("cou.commands.setattr") -def test_purge_before_argument_action(mock_setattr): - parser = ArgumentParser() - parser.add_argument("--purge", action="store_true", default=False) - parser.add_argument( - "--purge-before-date", - dest="purge_before", - type=str, - action=commands.PurgeBeforeArgumentAction, - ) - parser.parse_args("--purge --purge-before-date 2000-01-02".split()) - mock_setattr.assert_called() - - -@patch("cou.commands.setattr") -def test_purge_before_argument_action_failed(mock_setattr): - parser = ArgumentParser() - parser.add_argument("--purge", action="store_true", default=False) - parser.add_argument( - "--purge-before-date", - dest="purge_before", - type=str, - action=commands.PurgeBeforeArgumentAction, - ) +def test_purge_before_argument_missing_dependency(mock_setattr): with pytest.raises(SystemExit, match="2"): - parser.parse_args("--purge-before-date 2000-01-02".split()) + commands.parse_args("plan --purge-before-date 2000-01-02".split()) @patch("cou.commands.setattr") From da826a3720aa1a716c5a3ec4c52e08b7b97ffdc4 Mon Sep 17 00:00:00 2001 From: Chi Wai Chan Date: Mon, 2 Sep 2024 11:05:18 +0800 Subject: [PATCH 2/2] Apply suggestions --- cou/commands.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cou/commands.py b/cou/commands.py index e021ec1b..de8d31f1 100644 --- a/cou/commands.py +++ b/cou/commands.py @@ -542,9 +542,9 @@ def parse_args(args: Any) -> CLIargs: # pylint: disable=inconsistent-return-sta # validate arguments validation_errors = [] - if parsed_args.purge_before and not getattr(parsed_args, "purge", None): + if parsed_args.purge_before and not parsed_args.purge: validation_errors.append("--purge-before-date requires --purge") - if len(validation_errors) > 0: + if validation_errors: parser.error("\n" + "\n".join(validation_errors)) return parsed_args