From 51ed59c506cd0040320d885d5d633ee9f2ae0906 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adria=CC=80=20Sala?= Date: Fri, 27 Feb 2026 13:32:15 +0100 Subject: [PATCH] fix: awxkit help flags for detailed & general help Co-Authored-By: Claude Sonnet 4 --- awxkit/awxkit/cli/client.py | 74 +++++++++++++---- awxkit/awxkit/cli/options.py | 4 +- awxkit/awxkit/cli/resource.py | 4 +- awxkit/test/cli/test_client.py | 135 ++++++++++++++++++++++++++++++++ awxkit/test/cli/test_options.py | 21 +++++ 5 files changed, 220 insertions(+), 18 deletions(-) diff --git a/awxkit/awxkit/cli/client.py b/awxkit/awxkit/cli/client.py index ff1328fbd3..252c87dcce 100755 --- a/awxkit/awxkit/cli/client.py +++ b/awxkit/awxkit/cli/client.py @@ -80,6 +80,28 @@ class CLI(object): def help(self): return '--help' in self.argv or '-h' in self.argv + def _get_non_option_args(self, before_help=False): + """Extract non-option arguments from argv, optionally only those before help flag.""" + if before_help and self.help: + # Find position of help flag + help_pos = next((i for i, arg in enumerate(self.argv) if arg in ('--help', '-h')), len(self.argv)) + args_to_check = self.argv[:help_pos] + else: + args_to_check = self.argv + + return [arg for arg in args_to_check if not arg.startswith('-') and arg != 'awx'] + + def _is_main_help_request(self): + """ + Determine if help request is for main CLI (awx --help) vs subcommand (awx users create --help). + Returns True if this is a main CLI help request that should exit early. + """ + if not self.help: + return False + + # If there are non-option arguments before help flag, this is subcommand help + return len(self._get_non_option_args(before_help=True)) == 0 + def authenticate(self): """Configure the current session for authentication. @@ -224,6 +246,16 @@ class CLI(object): subparsers = self.subparsers[self.resource].add_subparsers(dest='action', metavar='action') subparsers.required = True + # Add manual help handling for resource-level help + # since we disabled add_help=False for resource subparsers + if self.help: + # Check if this is resource-level help (no action specified) + non_option_args = self._get_non_option_args() + if len(non_option_args) == 1 and non_option_args[0] == self.resource: + # Only resource specified, no action - show resource-level help + self.subparsers[self.resource].print_help() + return + # parse the action from OPTIONS parser = ResourceOptionsParser(self.v2, page, self.resource, subparsers) if parser.deprecated: @@ -232,6 +264,18 @@ class CLI(object): description = colored(description, 'yellow') self.subparsers[self.resource].description = description + # parse any action arguments FIRST before attempting to parse + if self.resource != 'settings': + for method in ('list', 'modify', 'create'): + if method in parser.parser.choices: + if method == 'list': + http_method = 'GET' + elif method == 'modify' and 'PUT' in parser.options: + http_method = 'PUT' + else: + http_method = 'POST' + parser.build_query_arguments(method, http_method) + if from_sphinx: # Our Sphinx plugin runs `parse_action` for *every* available # resource + action in the API so that it can generate usage @@ -244,23 +288,15 @@ class CLI(object): self.parser.parse_known_args(self.argv)[0] except SystemExit: pass - else: - self.parser.parse_known_args()[0] - - # parse any action arguments - if self.resource != 'settings': - for method in ('list', 'modify', 'create'): - if method in parser.parser.choices: - if method == 'list': - http_method = 'GET' - elif method == 'modify' and 'PUT' in parser.options: - http_method = 'PUT' - else: - http_method = 'POST' - parser.build_query_arguments(method, http_method) - if from_sphinx: parsed, extra = self.parser.parse_known_args(self.argv) else: + # Try to parse to determine if help is requested for a specific action + try: + self.parser.parse_known_args()[0] + except SystemExit: + # This happens when required arguments are missing, which is expected behavior + # Let argparse handle it and show the appropriate help + raise parsed, extra = self.parser.parse_known_args() if extra and self.verbose: @@ -313,6 +349,7 @@ class CLI(object): self.argv = argv self.parser = HelpfulArgumentParser(add_help=False) self.parser.add_argument( + '-h', '--help', action='store_true', help='prints usage information for the awx tool', @@ -322,6 +359,13 @@ class CLI(object): add_output_formatting_arguments(self.parser, env) self.args = self.parser.parse_known_args(self.argv)[0] + + # Early return for help to avoid server connection, but only for main CLI help + # Allow subcommand help (like 'awx users create --help') to continue processing + if self.help and self._is_main_help_request(): + self.parser.print_help() + sys.exit(0) + self.verbose = self.get_config('verbose') if self.verbose: logging.basicConfig(level='DEBUG') diff --git a/awxkit/awxkit/cli/options.py b/awxkit/awxkit/cli/options.py index a1b3ed45a8..4fc336ff38 100644 --- a/awxkit/awxkit/cli/options.py +++ b/awxkit/awxkit/cli/options.py @@ -126,7 +126,7 @@ class ResourceOptionsParser(object): if method not in action_map: continue method = action_map[method] - parser = self.parser.add_parser(method, help='') + parser = self.parser.add_parser(method, help='', add_help=True) if method == 'list': parser.add_argument( '--all', @@ -152,7 +152,7 @@ class ResourceOptionsParser(object): if 'DELETE' in self.allowed_options: allowed.append('delete') for method in allowed: - parser = self.parser.add_parser(method, help='') + parser = self.parser.add_parser(method, help='', add_help=True) self.parser.choices[method].add_argument( 'id', type=functools.partial(pk_or_name, self.v2, self.resource), help='the ID (or unique name) of the resource' ) diff --git a/awxkit/awxkit/cli/resource.py b/awxkit/awxkit/cli/resource.py index 69612488fe..4e8dc2d77b 100644 --- a/awxkit/awxkit/cli/resource.py +++ b/awxkit/awxkit/cli/resource.py @@ -167,7 +167,9 @@ def parse_resource(client, skip_deprecated=False): if k in DEPRECATED_RESOURCES: kwargs['aliases'] = [DEPRECATED_RESOURCES[k]] - client.subparsers[k] = subparsers.add_parser(k, help='', **kwargs) + # Disable automatic help handling for resource subparsers to prevent + # premature help display before action subparsers are built + client.subparsers[k] = subparsers.add_parser(k, help='', add_help=False, **kwargs) resource = client.parser.parse_known_args()[0].resource if resource in DEPRECATED_RESOURCES.values(): diff --git a/awxkit/test/cli/test_client.py b/awxkit/test/cli/test_client.py index a6ebae0cb7..327afefa87 100644 --- a/awxkit/test/cli/test_client.py +++ b/awxkit/test/cli/test_client.py @@ -1,5 +1,7 @@ import pytest from requests.exceptions import ConnectionError +import sys +from unittest.mock import patch from awxkit.cli import run, CLI @@ -53,3 +55,136 @@ def test_list_resources(capfd, resource): assert "usage:" in out for snippet in ('--conf.host https://example.awx.org]', '-v, --verbose'): assert snippet in out + + +class TestHelpHandling: + """Test suite for improved help handling functionality""" + + def test_get_non_option_args_basic(self): + """Test _get_non_option_args extracts non-option arguments correctly""" + cli = CLI() + cli.argv = ['awx', 'users', 'list', '--verbose'] + + result = cli._get_non_option_args() + assert result == ['users', 'list'] + + def test_get_non_option_args_with_flags(self): + """Test _get_non_option_args ignores option flags""" + cli = CLI() + cli.argv = ['awx', '--conf.host', 'example.com', 'jobs', 'create', '--name', 'test'] + + result = cli._get_non_option_args() + # Should include all non-option arguments (including flag values) except 'awx' + assert result == ['example.com', 'jobs', 'create', 'test'] + + def test_get_non_option_args_before_help(self): + """Test _get_non_option_args with before_help=True stops at help flag""" + cli = CLI() + cli.argv = ['awx', 'users', '--help', 'extra', 'args'] + + result = cli._get_non_option_args(before_help=True) + assert result == ['users'] + + def test_get_non_option_args_before_help_short_flag(self): + """Test _get_non_option_args with before_help=True stops at -h flag""" + cli = CLI() + cli.argv = ['awx', 'projects', '-h', 'should', 'not', 'appear'] + + result = cli._get_non_option_args(before_help=True) + assert result == ['projects'] + + def test_get_non_option_args_no_help_flag(self): + """Test _get_non_option_args when help flag not present""" + cli = CLI() + cli.argv = ['awx', 'organizations', 'list'] + + result = cli._get_non_option_args(before_help=True) + assert result == ['organizations', 'list'] + + def test_is_main_help_request_true(self): + """Test _is_main_help_request returns True for main CLI help""" + cli = CLI() + cli.argv = ['awx', '--help'] + + result = cli._is_main_help_request() + assert result is True + + def test_is_main_help_request_short_flag(self): + """Test _is_main_help_request returns True for main CLI help with -h""" + cli = CLI() + cli.argv = ['awx', '-h'] + + result = cli._is_main_help_request() + assert result is True + + def test_is_main_help_request_false_subcommand(self): + """Test _is_main_help_request returns False for subcommand help""" + cli = CLI() + cli.argv = ['awx', 'users', '--help'] + + result = cli._is_main_help_request() + assert result is False + + def test_is_main_help_request_false_action(self): + """Test _is_main_help_request returns False for action help""" + cli = CLI() + cli.argv = ['awx', 'jobs', 'create', '--help'] + + result = cli._is_main_help_request() + assert result is False + + def test_is_main_help_request_false_no_help(self): + """Test _is_main_help_request returns False when no help flag""" + cli = CLI() + cli.argv = ['awx', 'users', 'list'] + + result = cli._is_main_help_request() + assert result is False + + def test_early_help_return_main_cli(self): + """Test that main CLI help exits early without server connection""" + cli = CLI() + # Verify that _is_main_help_request works correctly + cli.argv = ['awx', '--help'] + assert cli._is_main_help_request() is True + + # Test that parse_args with main help flag should exit + with patch.object(sys, 'exit') as mock_exit: + cli.parse_args(['awx', '--help']) + mock_exit.assert_called_once_with(0) + + def test_no_early_exit_for_subcommand_help(self): + """Test that subcommand help does not exit early""" + with patch.object(sys, 'exit') as mock_exit: + cli = CLI() + # This should not exit early since it's subcommand help + cli.parse_args(['awx', 'users', '--help']) + + mock_exit.assert_not_called() + + def test_help_property_detection(self): + """Test that help property correctly detects help flags""" + cli = CLI() + + cli.argv = ['awx', '--help'] + assert cli.help is True + + cli.argv = ['awx', '-h'] + assert cli.help is True + + cli.argv = ['awx', 'users', '--help'] + assert cli.help is True + + cli.argv = ['awx', 'users', 'list'] + assert cli.help is False + + def test_short_help_flag_added(self): + """Test that -h flag is properly added to argument parser""" + cli = CLI() + cli.parse_args(['awx']) + + # Verify that both -h and --help are recognized + help_actions = [action for action in cli.parser._actions if '--help' in action.option_strings] + assert len(help_actions) == 1 + assert '-h' in help_actions[0].option_strings + assert '--help' in help_actions[0].option_strings diff --git a/awxkit/test/cli/test_options.py b/awxkit/test/cli/test_options.py index 01fc2a4169..b8a7c82e5d 100644 --- a/awxkit/test/cli/test_options.py +++ b/awxkit/test/cli/test_options.py @@ -251,3 +251,24 @@ class TestSettingsOptions(unittest.TestCase): out = StringIO() self.parser.choices['modify'].print_help(out) assert 'modify [-h] key value' in out.getvalue() + + +class TestHelpParameterChanges(unittest.TestCase): + """Test that add_help parameter changes work correctly""" + + def test_add_help_parameter_handling(self): + """Test that add_help=True and add_help=False work as expected""" + # Test add_help=True (for action parsers like list, create) + parser = argparse.ArgumentParser() + subparsers = parser.add_subparsers(dest='action') + + list_parser = subparsers.add_parser('list', help='', add_help=True) + out = StringIO() + list_parser.print_help(out) + help_text = out.getvalue() + assert 'show this help message and exit' in help_text + + # Test add_help=False (for resource parsers like users, jobs) + resource_parser = subparsers.add_parser('users', help='', add_help=False) + help_actions = [action for action in resource_parser._actions if '--help' in action.option_strings] + assert len(help_actions) == 0 # Should be 0 because add_help=False