From 1beb3eed45786c7839a9d32a5f4972c8520740a7 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Fri, 29 Dec 2023 12:00:39 +0100 Subject: [PATCH] Cleanup after SSH error return changes Before PR #64542, SSH error returns depended on whether the command was executed non-wrapped or wrapped. The former would always return an error dict including `stdout` and `stderr` keys, while the latter would return a string. Synchronizing this behavior to work like usual introduced a breaking change, which was reverted afterwards. This cleans up unnecessary code after the revert of the mentioned behavior. 1) SSHCommandExecutionErrors are only thrown when retcode > 0 2) `Probably got garbage` is the return of the module which should never be reached (when failing, it's in the returned string, it's never found in a dict key `ret["stderr"]`). --- salt/client/ssh/wrapper/__init__.py | 5 +---- tests/pytests/integration/ssh/test_deploy.py | 9 ++++----- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/salt/client/ssh/wrapper/__init__.py b/salt/client/ssh/wrapper/__init__.py index bb60f58cd1a..082ad5bb880 100644 --- a/salt/client/ssh/wrapper/__init__.py +++ b/salt/client/ssh/wrapper/__init__.py @@ -88,10 +88,7 @@ class SSHCommandExecutionError(SSHException, CommandExecutionError): return super().to_ret() def __str__(self): - ret = self.to_ret() - if self.retcode > 0: - return f"{self._error}: {self.stderr or self.stdout}" - return self._error + return f"{self._error}: {self.stderr or self.stdout}" class SSHPermissionDeniedError(SSHException): diff --git a/tests/pytests/integration/ssh/test_deploy.py b/tests/pytests/integration/ssh/test_deploy.py index 157666c3708..1760f00b2ed 100644 --- a/tests/pytests/integration/ssh/test_deploy.py +++ b/tests/pytests/integration/ssh/test_deploy.py @@ -276,9 +276,10 @@ def test_wrapper_unwrapped_command_exception(salt_ssh_cli): """ ret = salt_ssh_cli.run("check_exception.failure") assert ret.returncode == EX_AGGREGATE + # "Probably got garbage" would be returned as a string (the module return), + # so no need to check assert isinstance(ret.data, dict) assert ret.data - assert "Probably got garbage" not in ret.data["stderr"] assert ( "Error running 'disk.usage': Invalid flag passed to disk.usage" in ret.data["stderr"] @@ -292,9 +293,8 @@ def test_wrapper_unwrapped_command_parsing_failure(salt_ssh_cli): """ ret = salt_ssh_cli.run("check_parsing.failure", "whoops") assert ret.returncode == EX_AGGREGATE - assert ret.data - assert "Probably got garbage" not in ret.data["stderr"] assert isinstance(ret.data, dict) + assert ret.data assert ret.data["_error"] == "Failed to return clean data" assert ret.data["retcode"] == 0 assert ( @@ -310,9 +310,8 @@ def test_wrapper_unwrapped_command_invalid_return(salt_ssh_cli): """ ret = salt_ssh_cli.run("check_parsing.failure", "whoopsiedoodle") assert ret.returncode == EX_AGGREGATE - assert ret.data - assert "Probably got garbage" not in ret.data assert isinstance(ret.data, dict) + assert ret.data assert ret.data["_error"] == "Return dict was malformed" assert ret.data["retcode"] == 0 assert (