From fe349a1ccd658d86cfcf10eecdce9d48ece6176c Mon Sep 17 00:00:00 2001 From: Matt Clay Date: Thu, 5 May 2022 09:09:57 -0700 Subject: [PATCH] ansible-test - Enhance the shell command. (#77734) * ansible-test - Add shell --export option. * ansible-test - Support cmd args for shell command. Also allow shell to be used without a valid layout if no delegation is required. * ansible-test - Improve stderr/stdout consistency. By default all output goes to stdout only, with the exception of a fatal error. When using any of the following, all output defaults to stderr instead: * sanity with the `--lint` option -- sanity messages to stdout * coverage analyze -- output to stdout if the output file is `/dev/stdout` * shell -- shell output to stdout This fixes issues two main issues: * Unpredictable output order when using both info and error/warning messages. * Mixing of lint/command/shell output with bootstrapping messages on stdout. * ansible-test - Add changelog fragment. --- .../fragments/ansible-test-shell-features.yml | 7 +++ test/lib/ansible_test/_internal/__init__.py | 10 ++-- .../_internal/cli/commands/shell.py | 12 +++++ .../commands/coverage/analyze/__init__.py | 2 +- .../coverage/analyze/targets/__init__.py | 2 +- .../_internal/commands/sanity/__init__.py | 2 +- .../_internal/commands/shell/__init__.py | 22 ++++++++- test/lib/ansible_test/_internal/config.py | 8 +++- .../lib/ansible_test/_internal/connections.py | 7 +++ .../lib/ansible_test/_internal/docker_util.py | 10 +++- .../ansible_test/_internal/host_profiles.py | 2 +- test/lib/ansible_test/_internal/test.py | 4 +- test/lib/ansible_test/_internal/util.py | 46 ++++++++++++++----- .../lib/ansible_test/_internal/util_common.py | 6 ++- 14 files changed, 111 insertions(+), 29 deletions(-) create mode 100644 changelogs/fragments/ansible-test-shell-features.yml diff --git a/changelogs/fragments/ansible-test-shell-features.yml b/changelogs/fragments/ansible-test-shell-features.yml new file mode 100644 index 00000000000..dbe6890b72f --- /dev/null +++ b/changelogs/fragments/ansible-test-shell-features.yml @@ -0,0 +1,7 @@ +minor_changes: + - ansible-test - Add support for running non-interactive commands with ``ansible-test shell``. + - ansible-test - Add support for exporting inventory with ``ansible-test shell --export {path}``. + - ansible-test - The ``shell`` command can be used outside a collection if no controller delegation is required. + - ansible-test - Improve consistency of output messages by using stdout or stderr for most output, but not both. +bugfixes: + - ansible-test - Sanity test output with the ``--lint`` option is no longer mixed in with bootstrapping output. diff --git a/test/lib/ansible_test/_internal/__init__.py b/test/lib/ansible_test/_internal/__init__.py index e663c45e795..43d10c027d9 100644 --- a/test/lib/ansible_test/_internal/__init__.py +++ b/test/lib/ansible_test/_internal/__init__.py @@ -57,7 +57,7 @@ def main(cli_args=None): # type: (t.Optional[t.List[str]]) -> None display.truncate = config.truncate display.redact = config.redact display.color = config.color - display.info_stderr = config.info_stderr + display.fd = sys.stderr if config.display_stderr else sys.stdout configure_timeout(config) display.info('RLIMIT_NOFILE: %s' % (CURRENT_RLIMIT_NOFILE,), verbosity=2) @@ -66,7 +66,9 @@ def main(cli_args=None): # type: (t.Optional[t.List[str]]) -> None target_names = None try: - data_context().check_layout() + if config.check_layout: + data_context().check_layout() + args.func(config) except PrimeContainers: pass @@ -82,7 +84,7 @@ def main(cli_args=None): # type: (t.Optional[t.List[str]]) -> None if target_names: for target_name in target_names: - print(target_name) # info goes to stderr, this should be on stdout + print(target_name) # display goes to stderr, this should be on stdout display.review_warnings() config.success = True @@ -90,7 +92,7 @@ def main(cli_args=None): # type: (t.Optional[t.List[str]]) -> None display.warning(u'%s' % ex) sys.exit(0) except ApplicationError as ex: - display.error(u'%s' % ex) + display.fatal(u'%s' % ex) sys.exit(1) except KeyboardInterrupt: sys.exit(2) diff --git a/test/lib/ansible_test/_internal/cli/commands/shell.py b/test/lib/ansible_test/_internal/cli/commands/shell.py index 301ff70e905..7d52b39e058 100644 --- a/test/lib/ansible_test/_internal/cli/commands/shell.py +++ b/test/lib/ansible_test/_internal/cli/commands/shell.py @@ -38,10 +38,22 @@ def do_shell( shell = parser.add_argument_group(title='shell arguments') + shell.add_argument( + 'cmd', + nargs='*', + help='run the specified command', + ) + shell.add_argument( '--raw', action='store_true', help='direct to shell with no setup', ) + shell.add_argument( + '--export', + metavar='PATH', + help='export inventory instead of opening a shell', + ) + add_environments(parser, completer, ControllerMode.DELEGATED, TargetMode.SHELL) # shell diff --git a/test/lib/ansible_test/_internal/commands/coverage/analyze/__init__.py b/test/lib/ansible_test/_internal/commands/coverage/analyze/__init__.py index db169fd7a03..16521bef4f1 100644 --- a/test/lib/ansible_test/_internal/commands/coverage/analyze/__init__.py +++ b/test/lib/ansible_test/_internal/commands/coverage/analyze/__init__.py @@ -14,4 +14,4 @@ class CoverageAnalyzeConfig(CoverageConfig): # avoid mixing log messages with file output when using `/dev/stdout` for the output file on commands # this may be worth considering as the default behavior in the future, instead of being dependent on the command or options used - self.info_stderr = True + self.display_stderr = True diff --git a/test/lib/ansible_test/_internal/commands/coverage/analyze/targets/__init__.py b/test/lib/ansible_test/_internal/commands/coverage/analyze/targets/__init__.py index f94b7360ecd..412414050f3 100644 --- a/test/lib/ansible_test/_internal/commands/coverage/analyze/targets/__init__.py +++ b/test/lib/ansible_test/_internal/commands/coverage/analyze/targets/__init__.py @@ -32,7 +32,7 @@ class CoverageAnalyzeTargetsConfig(CoverageAnalyzeConfig): def __init__(self, args): # type: (t.Any) -> None super().__init__(args) - self.info_stderr = True + self.display_stderr = True def make_report(target_indexes, arcs, lines): # type: (TargetIndexes, Arcs, Lines) -> t.Dict[str, t.Any] diff --git a/test/lib/ansible_test/_internal/commands/sanity/__init__.py b/test/lib/ansible_test/_internal/commands/sanity/__init__.py index 7b7e6196fee..ff3a61699d1 100644 --- a/test/lib/ansible_test/_internal/commands/sanity/__init__.py +++ b/test/lib/ansible_test/_internal/commands/sanity/__init__.py @@ -179,7 +179,7 @@ def command_sanity(args): # type: (SanityConfig) -> None for test in tests: if args.list_tests: - display.info(test.name) + print(test.name) # display goes to stderr, this should be on stdout continue for version in SUPPORTED_PYTHON_VERSIONS: diff --git a/test/lib/ansible_test/_internal/commands/shell/__init__.py b/test/lib/ansible_test/_internal/commands/shell/__init__.py index 4cbfca42c22..4bd3fde02a7 100644 --- a/test/lib/ansible_test/_internal/commands/shell/__init__.py +++ b/test/lib/ansible_test/_internal/commands/shell/__init__.py @@ -39,13 +39,18 @@ from ...host_configs import ( OriginConfig, ) +from ...inventory import ( + create_controller_inventory, + create_posix_inventory, +) + def command_shell(args): # type: (ShellConfig) -> None """Entry point for the `shell` command.""" if args.raw and isinstance(args.targets[0], ControllerConfig): raise ApplicationError('The --raw option has no effect on the controller.') - if not sys.stdin.isatty(): + if not args.export and not args.cmd and not sys.stdin.isatty(): raise ApplicationError('Standard input must be a TTY to launch a shell.') host_state = prepare_profiles(args, skip_setup=args.raw) # shell @@ -61,10 +66,25 @@ def command_shell(args): # type: (ShellConfig) -> None if isinstance(target_profile, ControllerProfile): # run the shell locally unless a target was requested con = LocalConnection(args) # type: Connection + + if args.export: + display.info('Configuring controller inventory.', verbosity=1) + create_controller_inventory(args, args.export, host_state.controller_profile) else: # a target was requested, connect to it over SSH con = target_profile.get_controller_target_connections()[0] + if args.export: + display.info('Configuring target inventory.', verbosity=1) + create_posix_inventory(args, args.export, host_state.target_profiles, True) + + if args.export: + return + + if args.cmd: + con.run(args.cmd, capture=False, interactive=False, force_stdout=True) + return + if isinstance(con, SshConnection) and args.raw: cmd = [] # type: t.List[str] elif isinstance(target_profile, PosixProfile): diff --git a/test/lib/ansible_test/_internal/config.py b/test/lib/ansible_test/_internal/config.py index 060962fb2ec..e5320f48f98 100644 --- a/test/lib/ansible_test/_internal/config.py +++ b/test/lib/ansible_test/_internal/config.py @@ -214,8 +214,12 @@ class ShellConfig(EnvironmentConfig): def __init__(self, args): # type: (t.Any) -> None super().__init__(args, 'shell') + self.cmd = args.cmd # type: t.List[str] self.raw = args.raw # type: bool + self.check_layout = self.delegate # allow shell to be used without a valid layout as long as no delegation is required self.interactive = True + self.export = args.export # type: t.Optional[str] + self.display_stderr = True class SanityConfig(TestConfig): @@ -231,7 +235,7 @@ class SanityConfig(TestConfig): self.keep_git = args.keep_git # type: bool self.prime_venvs = args.prime_venvs # type: bool - self.info_stderr = self.lint + self.display_stderr = self.lint or self.list_tests if self.keep_git: def git_callback(files): # type: (t.List[t.Tuple[str, str]]) -> None @@ -270,7 +274,7 @@ class IntegrationConfig(TestConfig): if self.list_targets: self.explain = True - self.info_stderr = True + self.display_stderr = True def get_ansible_config(self): # type: () -> str """Return the path to the Ansible config for the given config.""" diff --git a/test/lib/ansible_test/_internal/connections.py b/test/lib/ansible_test/_internal/connections.py index 8f1ad0f949f..026058abb4f 100644 --- a/test/lib/ansible_test/_internal/connections.py +++ b/test/lib/ansible_test/_internal/connections.py @@ -50,6 +50,7 @@ class Connection(metaclass=abc.ABCMeta): data=None, # type: t.Optional[str] stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] + force_stdout=False, # type: bool ): # type: (...) -> t.Tuple[t.Optional[str], t.Optional[str]] """Run the specified command and return the result.""" @@ -97,6 +98,7 @@ class LocalConnection(Connection): data=None, # type: t.Optional[str] stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] + force_stdout=False, # type: bool ): # type: (...) -> t.Tuple[t.Optional[str], t.Optional[str]] """Run the specified command and return the result.""" return run_command( @@ -107,6 +109,7 @@ class LocalConnection(Connection): stdin=stdin, stdout=stdout, interactive=interactive, + force_stdout=force_stdout, ) @@ -137,6 +140,7 @@ class SshConnection(Connection): data=None, # type: t.Optional[str] stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] + force_stdout=False, # type: bool ): # type: (...) -> t.Tuple[t.Optional[str], t.Optional[str]] """Run the specified command and return the result.""" options = list(self.options) @@ -170,6 +174,7 @@ class SshConnection(Connection): stdin=stdin, stdout=stdout, interactive=interactive, + force_stdout=force_stdout, error_callback=error_callback, ) @@ -217,6 +222,7 @@ class DockerConnection(Connection): data=None, # type: t.Optional[str] stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] + force_stdout=False, # type: bool ): # type: (...) -> t.Tuple[t.Optional[str], t.Optional[str]] """Run the specified command and return the result.""" options = [] @@ -237,6 +243,7 @@ class DockerConnection(Connection): stdin=stdin, stdout=stdout, interactive=interactive, + force_stdout=force_stdout, ) def inspect(self): # type: () -> DockerInspect diff --git a/test/lib/ansible_test/_internal/docker_util.py b/test/lib/ansible_test/_internal/docker_util.py index 2db2c86ac83..0a17bf4c665 100644 --- a/test/lib/ansible_test/_internal/docker_util.py +++ b/test/lib/ansible_test/_internal/docker_util.py @@ -515,6 +515,7 @@ def docker_exec( stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] interactive=False, # type: bool + force_stdout=False, # type: bool data=None, # type: t.Optional[str] ): # type: (...) -> t.Tuple[t.Optional[str], t.Optional[str]] """Execute the given command in the specified container.""" @@ -524,7 +525,8 @@ def docker_exec( if data or stdin or stdout: options.append('-i') - return docker_command(args, ['exec'] + options + [container_id] + cmd, capture=capture, stdin=stdin, stdout=stdout, interactive=interactive, data=data) + return docker_command(args, ['exec'] + options + [container_id] + cmd, capture=capture, stdin=stdin, stdout=stdout, interactive=interactive, + force_stdout=force_stdout, data=data) def docker_info(args): # type: (CommonConfig) -> t.Dict[str, t.Any] @@ -546,15 +548,19 @@ def docker_command( stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] interactive=False, # type: bool + force_stdout=False, # type: bool always=False, # type: bool data=None, # type: t.Optional[str] ): # type: (...) -> t.Tuple[t.Optional[str], t.Optional[str]] """Run the specified docker command.""" env = docker_environment() command = [require_docker().command] + if command[0] == 'podman' and _get_podman_remote(): command.append('--remote') - return run_command(args, command + cmd, env=env, capture=capture, stdin=stdin, stdout=stdout, interactive=interactive, always=always, data=data) + + return run_command(args, command + cmd, env=env, capture=capture, stdin=stdin, stdout=stdout, interactive=interactive, always=always, + force_stdout=force_stdout, data=data) def docker_environment(): # type: () -> t.Dict[str, str] diff --git a/test/lib/ansible_test/_internal/host_profiles.py b/test/lib/ansible_test/_internal/host_profiles.py index 463521f0277..b15d7d356e3 100644 --- a/test/lib/ansible_test/_internal/host_profiles.py +++ b/test/lib/ansible_test/_internal/host_profiles.py @@ -150,7 +150,7 @@ class Inventory: inventory_text = inventory_text.strip() if not args.explain: - write_text_file(path, inventory_text) + write_text_file(path, inventory_text + '\n') display.info(f'>>> Inventory\n{inventory_text}', verbosity=3) diff --git a/test/lib/ansible_test/_internal/test.py b/test/lib/ansible_test/_internal/test.py index 3e149b15bf7..6d2cf2a3440 100644 --- a/test/lib/ansible_test/_internal/test.py +++ b/test/lib/ansible_test/_internal/test.py @@ -265,10 +265,10 @@ class TestFailure(TestResult): message = 'The test `%s` failed. See stderr output for details.' % command path = '' message = TestMessage(message, path) - print(message) + print(message) # display goes to stderr, this should be on stdout else: for message in self.messages: - print(message) + print(message) # display goes to stderr, this should be on stdout def write_junit(self, args): # type: (TestConfig) -> None """Write results to a junit XML file.""" diff --git a/test/lib/ansible_test/_internal/util.py b/test/lib/ansible_test/_internal/util.py index e43e3a31f81..25cca59d40c 100644 --- a/test/lib/ansible_test/_internal/util.py +++ b/test/lib/ansible_test/_internal/util.py @@ -330,6 +330,7 @@ def raw_command( stdin=None, # type: t.Optional[t.Union[t.IO[bytes], int]] stdout=None, # type: t.Optional[t.Union[t.IO[bytes], int]] interactive=False, # type: bool + force_stdout=False, # type: bool cmd_verbosity=1, # type: int str_errors='strict', # type: str error_callback=None, # type: t.Optional[t.Callable[[SubprocessError], None]] @@ -353,6 +354,12 @@ def raw_command( if stdout and not capture: raise InternalError('Redirection of stdout requires capture=True to avoid redirection of stderr to stdout.') + if force_stdout and capture: + raise InternalError('Cannot combine force_stdout=True with capture=True.') + + if force_stdout and interactive: + raise InternalError('Cannot combine force_stdout=True with interactive=True.') + if not cwd: cwd = os.getcwd() @@ -440,7 +447,8 @@ def raw_command( if communicate: data_bytes = to_optional_bytes(data) - stdout_bytes, stderr_bytes = communicate_with_process(process, data_bytes, stdout == subprocess.PIPE, stderr == subprocess.PIPE, capture=capture) + stdout_bytes, stderr_bytes = communicate_with_process(process, data_bytes, stdout == subprocess.PIPE, stderr == subprocess.PIPE, capture=capture, + force_stdout=force_stdout) stdout_text = to_optional_text(stdout_bytes, str_errors) or u'' stderr_text = to_optional_text(stderr_bytes, str_errors) or u'' else: @@ -463,7 +471,14 @@ def raw_command( raise SubprocessError(cmd, status, stdout_text, stderr_text, runtime, error_callback) -def communicate_with_process(process: subprocess.Popen, stdin: t.Optional[bytes], stdout: bool, stderr: bool, capture: bool) -> t.Tuple[bytes, bytes]: +def communicate_with_process( + process: subprocess.Popen, + stdin: t.Optional[bytes], + stdout: bool, + stderr: bool, + capture: bool, + force_stdout: bool +) -> t.Tuple[bytes, bytes]: """Communicate with the specified process, handling stdin/stdout/stderr as requested.""" threads: t.List[WrappedThread] = [] reader: t.Type[ReaderThread] @@ -477,13 +492,13 @@ def communicate_with_process(process: subprocess.Popen, stdin: t.Optional[bytes] threads.append(WriterThread(process.stdin, stdin)) if stdout: - stdout_reader = reader(process.stdout) + stdout_reader = reader(process.stdout, force_stdout) threads.append(stdout_reader) else: stdout_reader = None if stderr: - stderr_reader = reader(process.stderr) + stderr_reader = reader(process.stderr, force_stdout) threads.append(stderr_reader) else: stderr_reader = None @@ -531,10 +546,11 @@ class WriterThread(WrappedThread): class ReaderThread(WrappedThread, metaclass=abc.ABCMeta): """Thread to read stdout from a subprocess.""" - def __init__(self, handle: t.IO[bytes]) -> None: + def __init__(self, handle: t.IO[bytes], force_stdout: bool) -> None: super().__init__(self._run) self.handle = handle + self.force_stdout = force_stdout self.lines = [] # type: t.List[bytes] @abc.abstractmethod @@ -561,7 +577,7 @@ class OutputThread(ReaderThread): def _run(self) -> None: """Workload to run on a thread.""" src = self.handle - dst = sys.stdout.buffer + dst = sys.stdout.buffer if self.force_stdout else display.fd.buffer try: for line in src: @@ -740,7 +756,7 @@ class Display: self.color = sys.stdout.isatty() self.warnings = [] self.warnings_unique = set() - self.info_stderr = False + self.fd = sys.stderr # default to stderr until config is initialized to avoid early messages going to stdout self.rows = 0 self.columns = 0 self.truncate = 0 @@ -752,7 +768,7 @@ class Display: def __warning(self, message): # type: (str) -> None """Internal implementation for displaying a warning message.""" - self.print_message('WARNING: %s' % message, color=self.purple, fd=sys.stderr) + self.print_message('WARNING: %s' % message, color=self.purple) def review_warnings(self): # type: () -> None """Review all warnings which previously occurred.""" @@ -780,23 +796,27 @@ class Display: def notice(self, message): # type: (str) -> None """Display a notice level message.""" - self.print_message('NOTICE: %s' % message, color=self.purple, fd=sys.stderr) + self.print_message('NOTICE: %s' % message, color=self.purple) def error(self, message): # type: (str) -> None """Display an error level message.""" - self.print_message('ERROR: %s' % message, color=self.red, fd=sys.stderr) + self.print_message('ERROR: %s' % message, color=self.red) + + def fatal(self, message): # type: (str) -> None + """Display a fatal level message.""" + self.print_message('FATAL: %s' % message, color=self.red, stderr=True) def info(self, message, verbosity=0, truncate=False): # type: (str, int, bool) -> None """Display an info level message.""" if self.verbosity >= verbosity: color = self.verbosity_colors.get(verbosity, self.yellow) - self.print_message(message, color=color, fd=sys.stderr if self.info_stderr else sys.stdout, truncate=truncate) + self.print_message(message, color=color, truncate=truncate) def print_message( # pylint: disable=locally-disabled, invalid-name self, message, # type: str color=None, # type: t.Optional[str] - fd=sys.stdout, # type: t.IO[str] + stderr=False, # type: bool truncate=False, # type: bool ): # type: (...) -> None """Display a message.""" @@ -816,6 +836,8 @@ class Display: message = message.replace(self.clear, color) message = '%s%s%s' % (color, message, self.clear) + fd = sys.stderr if stderr else self.fd + print(message, file=fd) fd.flush() diff --git a/test/lib/ansible_test/_internal/util_common.py b/test/lib/ansible_test/_internal/util_common.py index baac5b52511..86e0d776bb3 100644 --- a/test/lib/ansible_test/_internal/util_common.py +++ b/test/lib/ansible_test/_internal/util_common.py @@ -127,6 +127,7 @@ class CommonConfig: def __init__(self, args, command): # type: (t.Any, str) -> None self.command = command self.interactive = False + self.check_layout = True self.success = None # type: t.Optional[bool] self.color = args.color # type: bool @@ -136,7 +137,7 @@ class CommonConfig: self.truncate = args.truncate # type: int self.redact = args.redact # type: bool - self.info_stderr = False # type: bool + self.display_stderr = False # type: bool self.session_name = generate_name() @@ -408,6 +409,7 @@ def run_command( stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] interactive=False, # type: bool + force_stdout=False, # type: bool cmd_verbosity=1, # type: int str_errors='strict', # type: str error_callback=None, # type: t.Optional[t.Callable[[SubprocessError], None]] @@ -415,7 +417,7 @@ def run_command( """Run the specified command and return stdout and stderr as a tuple.""" explain = args.explain and not always return raw_command(cmd, capture=capture, env=env, data=data, cwd=cwd, explain=explain, stdin=stdin, stdout=stdout, interactive=interactive, - cmd_verbosity=cmd_verbosity, str_errors=str_errors, error_callback=error_callback) + force_stdout=force_stdout, cmd_verbosity=cmd_verbosity, str_errors=str_errors, error_callback=error_callback) def yamlcheck(python):