Repository: ansible/ansible-navigator (language: python)

Issue:
Update actions using `run_command` in mode `stdout` to use runner's return code
https://github.com/ansible/ansible-runner/pull/696/files



Discussion / hints:

Fix patch:
diff --git a/src/ansible_navigator/action_defs.py b/src/ansible_navigator/action_defs.py
new file mode 100644
index 000000000..ddaa0be8b
--- /dev/null
+++ b/src/ansible_navigator/action_defs.py
@@ -0,0 +1,25 @@
+"""Pre-defined objects used by actions."""
+
+
+from typing import NamedTuple
+from typing import Union
+
+
+class RunReturn(NamedTuple):
+    """The base return object for any action's run method."""
+
+    #: A message to display immediately before exiting
+    message: str
+    #: The return code for the for the process
+    return_code: int
+
+
+class RunInteractiveReturn(RunReturn):
+    """The return object for an action's run interactive method."""
+
+
+class RunStdoutReturn(RunReturn):
+    """The return object for an action's run stdout method."""
+
+
+ActionReturn = Union[RunReturn, RunInteractiveReturn, RunStdoutReturn]
diff --git a/src/ansible_navigator/actions/__init__.py b/src/ansible_navigator/actions/__init__.py
index 12cb100da..603c49006 100644
--- a/src/ansible_navigator/actions/__init__.py
+++ b/src/ansible_navigator/actions/__init__.py
@@ -16,6 +16,7 @@
 from typing import Callable
 from typing import Union
 
+from ..action_defs import RunStdoutReturn
 from ..app_public import AppPublic
 from ..configuration_subsystem import ApplicationConfiguration
 from ..ui_framework import Interaction
@@ -28,7 +29,10 @@
 
 kegexes: Callable = actions.kegexes_factory(__package__)
 
-run_action_stdout: Callable[[str, ApplicationConfiguration], int] = actions.run_stdout_factory(
+run_action_stdout: Callable[
+    [str, ApplicationConfiguration],
+    RunStdoutReturn,
+] = actions.run_stdout_factory(
     __package__,
 )
 
diff --git a/src/ansible_navigator/actions/_actions.py b/src/ansible_navigator/actions/_actions.py
index c4070f6e7..1d3e0543e 100644
--- a/src/ansible_navigator/actions/_actions.py
+++ b/src/ansible_navigator/actions/_actions.py
@@ -15,6 +15,8 @@
 from typing import List
 from typing import Tuple
 
+from ..action_defs import RunStdoutReturn
+
 
 # ``mypy``/``pylint`` idiom for py36 compatibility
 # https://github.com/python/typeshed/issues/3500#issuecomment-560958608
@@ -167,7 +169,7 @@ def run_interactive_factory(package: str) -> Callable:
     return functools.partial(run_interactive, package)
 
 
-def run_stdout(package: str, action: str, *args: Any, **_kwargs: Any) -> Any:
+def run_stdout(package: str, action: str, *args: Any, **_kwargs: Any) -> RunStdoutReturn:
     """Call the given action's ``run_stdout()`` method.
 
     :param package: The name of the package
diff --git a/src/ansible_navigator/actions/builder.py b/src/ansible_navigator/actions/builder.py
index 2f5f7d66f..de4459e4a 100644
--- a/src/ansible_navigator/actions/builder.py
+++ b/src/ansible_navigator/actions/builder.py
@@ -5,12 +5,12 @@
 """
 import os
 import shutil
-import sys
 
 from typing import Optional
 from typing import Tuple
 
 from ..action_base import ActionBase
+from ..action_defs import RunStdoutReturn
 from ..configuration_subsystem import ApplicationConfiguration
 from ..configuration_subsystem.definitions import Constants
 from ..runner import Command
@@ -30,21 +30,20 @@ def __init__(self, args: ApplicationConfiguration):
         """
         super().__init__(args=args, logger_name=__name__, name="builder")
 
-    def run_stdout(self) -> Optional[int]:
-        """Run in mode stdout.
+    def run_stdout(self) -> RunStdoutReturn:
+        """Execute the ``builder`` request for mode stdout.
 
-        :returns: The return code or None. If the response from the
-                  runner invocation is None, indicates there is no
-                  console output to display.
+        :returns: The return code or 1. If the response from the runner invocation is None,
+            indicates there is no console output to display, so assume an issue and return 1
+            along with a message to review the logs.
         """
         self._logger.debug("builder requested in stdout mode")
         response = self._run_runner()
-        if response:
-            _, err, ret_code = response
-            if err:
-                sys.stdout.write(err)
-            return ret_code
-        return None
+        if response is None:
+            self._logger.error("Unexpected response: %s", response)
+            return RunStdoutReturn(message="Please review the log for errors.", return_code=1)
+        _out, error, return_code = response
+        return RunStdoutReturn(message=error, return_code=return_code)
 
     def _run_runner(self) -> Optional[Tuple]:
         """Spin up runner.
diff --git a/src/ansible_navigator/actions/config.py b/src/ansible_navigator/actions/config.py
index 49f47efd4..f9a9f073b 100644
--- a/src/ansible_navigator/actions/config.py
+++ b/src/ansible_navigator/actions/config.py
@@ -15,6 +15,7 @@
 from .._yaml import Loader
 from .._yaml import yaml
 from ..action_base import ActionBase
+from ..action_defs import RunStdoutReturn
 from ..app_public import AppPublic
 from ..configuration_subsystem import ApplicationConfiguration
 from ..runner import AnsibleConfig
@@ -143,17 +144,20 @@ def run(self, interaction: Interaction, app: AppPublic) -> Union[Interaction, No
         self._prepare_to_exit(interaction)
         return None
 
-    def run_stdout(self) -> Union[None, int]:
+    def run_stdout(self) -> RunStdoutReturn:
         """Execute the ``config`` request for mode stdout.
 
-        :returns: Nothing or the error code
+        :returns: The return code or 1. If the response from the runner invocation is None,
+            indicates there is no console output to display, so assume an issue and return 1
+            along with a message to review the logs.
         """
         self._logger.debug("config requested in stdout mode")
         response = self._run_runner()
-        if response:
-            _, _, ret_code = response
-            return ret_code
-        return None
+        if response is None:
+            self._logger.error("Unexpected response: %s", response)
+            return RunStdoutReturn(message="Please review the log for errors.", return_code=1)
+        _out, error, return_code = response
+        return RunStdoutReturn(message=error, return_code=return_code)
 
     def _take_step(self) -> None:
         """Take a step based on the current step or step back."""
diff --git a/src/ansible_navigator/actions/doc.py b/src/ansible_navigator/actions/doc.py
index 621c5d953..c7e7965ef 100644
--- a/src/ansible_navigator/actions/doc.py
+++ b/src/ansible_navigator/actions/doc.py
@@ -13,6 +13,7 @@
 from typing import Union
 
 from ..action_base import ActionBase
+from ..action_defs import RunStdoutReturn
 from ..app_public import AppPublic
 from ..configuration_subsystem import ApplicationConfiguration
 from ..configuration_subsystem import Constants as C
@@ -127,19 +128,22 @@ def run(self, interaction: Interaction, app: AppPublic) -> Union[Interaction, No
         self._prepare_to_exit(interaction)
         return next_interaction
 
-    def run_stdout(self) -> Union[None, int]:
-        """Execute the ``config`` request for mode stdout.
+    def run_stdout(self) -> RunStdoutReturn:
+        """Execute the ``doc`` request for mode stdout.
 
-        :returns: Nothing or the error code
+        :returns: The return code or 1. If the response from the runner invocation is None,
+            indicates there is no console output to display, so assume an issue and return 1
+            along with a message to review the logs.
         """
         self._plugin_name = self._args.plugin_name
         self._plugin_type = self._args.plugin_type
         self._logger.debug("doc requested in stdout mode")
         response = self._run_runner()
-        if response:
-            _, _, ret_code = response
-            return ret_code
-        return None
+        if response is None:
+            self._logger.error("Unexpected response: %s", response)
+            return RunStdoutReturn(message="Please review the log for errors.", return_code=1)
+        _out, error, return_code = response
+        return RunStdoutReturn(message=error, return_code=return_code)
 
     def _run_runner(self) -> Union[None, dict, Tuple[str, str, int]]:
         # pylint: disable=too-many-branches
diff --git a/src/ansible_navigator/actions/exec.py b/src/ansible_navigator/actions/exec.py
index b54024e31..9dc76b563 100644
--- a/src/ansible_navigator/actions/exec.py
+++ b/src/ansible_navigator/actions/exec.py
@@ -5,9 +5,9 @@
 from typing import List
 from typing import Optional
 from typing import Tuple
-from typing import Union
 
 from ..action_base import ActionBase
+from ..action_defs import RunStdoutReturn
 from ..configuration_subsystem import ApplicationConfiguration
 from ..configuration_subsystem.definitions import Constants
 from ..runner import Command
@@ -49,17 +49,20 @@ def __init__(self, args: ApplicationConfiguration):
         """
         super().__init__(args=args, logger_name=__name__, name="exec")
 
-    def run_stdout(self) -> Union[None, int]:
-        """Run in mode stdout.
+    def run_stdout(self) -> RunStdoutReturn:
+        """Execute the ``exec`` request for mode stdout.
 
-        :returns: The return code or None
+        :returns: The return code or 1. If the response from the runner invocation is None,
+            indicates there is no console output to display, so assume an issue and return 1
+            along with a message to review the logs.
         """
         self._logger.debug("exec requested in stdout mode")
         response = self._run_runner()
-        if response:
-            _, _, ret_code = response
-            return ret_code
-        return None
+        if response is None:
+            self._logger.error("Unexpected response: %s", response)
+            return RunStdoutReturn(message="Please review the log for errors.", return_code=1)
+        _out, error, return_code = response
+        return RunStdoutReturn(message=error, return_code=return_code)
 
     def _run_runner(self) -> Optional[Tuple]:
         """Spin up runner.
diff --git a/src/ansible_navigator/actions/inventory.py b/src/ansible_navigator/actions/inventory.py
index de456456d..d19b4396b 100644
--- a/src/ansible_navigator/actions/inventory.py
+++ b/src/ansible_navigator/actions/inventory.py
@@ -13,6 +13,7 @@
 from typing import Union
 
 from ..action_base import ActionBase
+from ..action_defs import RunStdoutReturn
 from ..app_public import AppPublic
 from ..configuration_subsystem import ApplicationConfiguration
 from ..runner import AnsibleInventory
@@ -235,16 +236,19 @@ def run(self, interaction: Interaction, app: AppPublic) -> Union[Interaction, No
         self._prepare_to_exit(interaction)
         return None
 
-    def run_stdout(self) -> int:
+    def run_stdout(self) -> RunStdoutReturn:
         """Execute the ``inventory`` request for mode stdout.
 
-        :returns: The error code
+        :returns: The return code, 0 or 1. If the runner status if failed, return 1
+            along with a message to review the logs.
         """
         self._logger.debug("inventory requested in stdout mode")
         if hasattr(self._args, "inventory") and self._args.inventory:
             self._inventories = self._args.inventory
         self._collect_inventory_details()
-        return 1 if self._runner.status == "failed" else 0
+        if self._runner.status == "failed":
+            return RunStdoutReturn(message="Please review the log for errors.", return_code=1)
+        return RunStdoutReturn(message="", return_code=0)
 
     def _take_step(self) -> None:
         """Take a step based on the current step or step back."""
diff --git a/src/ansible_navigator/actions/run.py b/src/ansible_navigator/actions/run.py
index f9fb476ef..750b03243 100644
--- a/src/ansible_navigator/actions/run.py
+++ b/src/ansible_navigator/actions/run.py
@@ -21,6 +21,7 @@
 from typing import Union
 
 from ..action_base import ActionBase
+from ..action_defs import RunStdoutReturn
 from ..app_public import AppPublic
 from ..configuration_subsystem import ApplicationConfiguration
 from ..runner import CommandAsync
@@ -233,11 +234,17 @@ def mode(self):
             return "stdout_w_artifact"
         return self._args.mode
 
-    def run_stdout(self) -> int:
-        """Run in old school mode, just stdout."""
+    def run_stdout(self) -> RunStdoutReturn:
+        """Execute the ``inventory`` request for mode stdout.
+
+        :returns: The return code from the runner invocation, along with a message to review the
+            logs if not 0.
+        """
         if self._args.app == "replay":
             successful: bool = self._init_replay()
-            return 0 if successful else 1
+            if successful:
+                return RunStdoutReturn(message="", return_code=0)
+            return RunStdoutReturn(message="Please review the log for errors.", return_code=1)
 
         self._logger.debug("playbook requested in interactive mode")
         self._subaction_type = "playbook"
@@ -250,7 +257,13 @@ def run_stdout(self) -> int:
                     self.write_artifact()
                 self._logger.debug("runner finished")
                 break
-        return self.runner.ansible_runner_instance.rc
+        return_code = self.runner.ansible_runner_instance.rc
+        if return_code != 0:
+            return RunStdoutReturn(
+                message="Please review the log for errors.",
+                return_code=return_code,
+            )
+        return RunStdoutReturn(message="", return_code=return_code)
 
     def run(self, interaction: Interaction, app: AppPublic) -> Union[Interaction, None]:
         """run :run or :replay
diff --git a/src/ansible_navigator/cli.py b/src/ansible_navigator/cli.py
index 41b8646f8..376611f6b 100644
--- a/src/ansible_navigator/cli.py
+++ b/src/ansible_navigator/cli.py
@@ -12,6 +12,10 @@
 from pathlib import Path
 from typing import List
 
+from .action_defs import ActionReturn
+from .action_defs import RunInteractiveReturn
+from .action_defs import RunReturn
+from .action_defs import RunStdoutReturn
 from .action_runner import ActionRunner
 from .actions import run_action_stdout
 from .configuration_subsystem import ApplicationConfiguration
@@ -74,19 +78,30 @@ def setup_logger(args: ApplicationConfiguration) -> None:
     logger.info("New ansible-runner instance, logging initialized")
 
 
-def run(args: ApplicationConfiguration) -> int:
-    """run the appropriate app"""
-    try:
-        if args.mode == "stdout":
-            return_code = run_action_stdout(args.app.replace("-", "_"), args)
-            return return_code
-        clear_screen()
-        wrapper(ActionRunner(args=args).run)
-        return 0
-    except KeyboardInterrupt:
-        logger.warning("Dirty exit, killing the pid")
-        os.kill(os.getpid(), signal.SIGTERM)
-        return 1
+def run(args: ApplicationConfiguration) -> ActionReturn:
+    """Run the appropriate subcommand.
+
+    :param args: The current application settings
+    :returns: A message to display and a return code.
+    """
+    if args.mode == "stdout":
+        try:
+            result = run_action_stdout(args.app.replace("-", "_"), args)
+            return result
+        except KeyboardInterrupt:
+            logger.warning("Dirty exit, killing the pid")
+            os.kill(os.getpid(), signal.SIGTERM)
+            return RunStdoutReturn(message="", return_code=1)
+    elif args.mode == "interactive":
+        try:
+            clear_screen()
+            wrapper(ActionRunner(args=args).run)
+            return RunInteractiveReturn(message="", return_code=0)
+        except KeyboardInterrupt:
+            logger.warning("Dirty exit, killing the pid")
+            os.kill(os.getpid(), signal.SIGTERM)
+            return RunInteractiveReturn(message="", return_code=1)
+    return RunReturn(message="", return_code=0)
 
 
 def main():
@@ -140,9 +155,12 @@ def main():
     if args.execution_environment:
         pull_image(args)
 
-    return_code = run(args)
-    if return_code:
-        sys.exit(return_code)
+    run_return = run(args)
+    run_message = f"{run_return.message}\n"
+    if run_return.return_code != 0 and run_return.message:
+        sys.stderr.write(run_message)
+    elif run_return.message:
+        sys.stdout.write(run_message)
 
 
 if __name__ == "__main__":


Test patch:
diff --git a/tests/integration/_action_run_test.py b/tests/integration/_action_run_test.py
index 137efa783..730f00a4c 100644
--- a/tests/integration/_action_run_test.py
+++ b/tests/integration/_action_run_test.py
@@ -11,6 +11,7 @@
 from typing import Optional
 from typing import Tuple
 
+from ansible_navigator.action_defs import RunStdoutReturn
 from ansible_navigator.app_public import AppPublic
 from ansible_navigator.configuration_subsystem import Constants as C
 from ansible_navigator.configuration_subsystem import NavigatorConfiguration
@@ -134,7 +135,7 @@ def run_action_interactive(self) -> Any:
 
         return action
 
-    def run_action_stdout(self, **kwargs) -> Tuple[int, str, str]:
+    def run_action_stdout(self, **kwargs) -> Tuple[RunStdoutReturn, str, str]:
         # pylint: disable=too-many-locals
         """run the action"""
         self._app_args.update({"mode": "stdout"})
@@ -163,7 +164,7 @@ def run_action_stdout(self, **kwargs) -> Tuple[int, str, str]:
             sys.stderr = sys_stderr  # type: ignore
 
             # run the action
-            return_code = action.run_stdout()
+            result = action.run_stdout()
 
             # restore ``stdin``
             sys.stdin = __stdin__
@@ -178,4 +179,4 @@ def run_action_stdout(self, **kwargs) -> Tuple[int, str, str]:
             stderr = sys.stderr.read().decode()  # type: ignore
             sys.stderr = __stderr__
 
-        return return_code, stdout, stderr
+        return result, stdout, stderr
diff --git a/tests/integration/test_stdout_exit_codes.py b/tests/integration/test_stdout_exit_codes.py
index d27f2fc35..616ed9b52 100644
--- a/tests/integration/test_stdout_exit_codes.py
+++ b/tests/integration/test_stdout_exit_codes.py
@@ -34,61 +34,67 @@ def id_test_data(value):
 
 
 class StdoutTest(NamedTuple):
-    """define the stdout test"""
+    """Definition of a stdout test."""
 
+    #: The name of the action
     action_name: str
+    #: Parameters for the action
     action_params: Tuple[Tuple, ...]
-    message: str
+    #: Text to search for
+    present: str
+    #: Expected return code
     return_code: int
+    #: Expected return message
+    message: str = ""
 
 
 fixture_test_data = (
     StdoutTest(
         action_name="config",
         action_params=(("cmdline", ["--help"]),),
-        message="usage: ansible-config",
+        present="usage: ansible-config",
         return_code=0,
     ),
     StdoutTest(
         action_name="config",
         action_params=(("cmdline", ["foo"]),),
-        message="invalid choice: 'foo'",
+        present="invalid choice: 'foo'",
         return_code=2,
     ),
     StdoutTest(
         action_name="doc",
         action_params=(("cmdline", ["--help"]),),
-        message="usage: ansible-doc",
+        present="usage: ansible-doc",
         return_code=0,
     ),
     StdoutTest(
         action_name="doc",
         action_params=(("cmdline", ["--json"]),),
-        message="Incorrect options passed",
+        present="Incorrect options passed",
         return_code=5,
     ),
     StdoutTest(
         action_name="inventory",
         action_params=(("cmdline", ["--help"]),),
-        message="usage: ansible-inventory",
+        present="usage: ansible-inventory",
         return_code=0,
     ),
     StdoutTest(
         action_name="inventory",
         action_params=(("cmdline", ["foo"]),),
-        message="No action selected",
+        present="No action selected",
         return_code=1,
     ),
     StdoutTest(
         action_name="run",
         action_params=(("playbook", PLAYBOOK), ("playbook_artifact_enable", False)),
-        message="success",
+        present="success",
         return_code=0,
     ),
     StdoutTest(
         action_name="run",
         action_params=(("playbook", "foo"), ("playbook_artifact_enable", False)),
-        message="foo could not be found",
+        present="foo could not be found",
         return_code=1,
     ),
 )
@@ -98,11 +104,15 @@ class StdoutTest(NamedTuple):
 @pytest.mark.parametrize("test_data", fixture_test_data, ids=id_test_data)
 def test(action_run_stdout, params, test_data):
     """test for a return code"""
-    actionruntest = action_run_stdout(action_name=test_data.action_name, **params)
-    ret, out, err = actionruntest.run_action_stdout(**dict(test_data.action_params))
-    assert ret == test_data.return_code
+    action_runner = action_run_stdout(action_name=test_data.action_name, **params)
+    run_stdout_return, stdout, stderr = action_runner.run_action_stdout(
+        **dict(test_data.action_params),
+    )
+    assert run_stdout_return.return_code == test_data.return_code
     if test_data.return_code == 0:
-        assert test_data.message in out, (test_data.message, out, err)
+        assert test_data.present in stdout
+        assert test_data.message in run_stdout_return.message
     else:
-        std_stream = out if params["execution_environment"] else err
-        assert test_data.message in std_stream, (test_data.message, out, err)
+        std_stream = stdout if params["execution_environment"] else stderr
+        assert test_data.present in std_stream
+        assert test_data.message in run_stdout_return.message