8000 Send show_jid output to stderr by tacerus · Pull Request #67955 · saltstack/salt · GitHub
[go: up one dir, main page]
Skip to content

Send show_jid output to stderr #67955

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Send show_jid output to stderr #67955

wants to merge 1 commit into from

Conversation

tacerus
Copy link
Contributor
@tacerus tacerus commented Apr 10, 2025

What does this PR do?

When using --out=json, all data sent to stdout should be valid JSON to allow for piping into processing tools such as jq. Make this the case with "show_jid" being enabled as well, by printing the JID line to stderr instead of stdout.

What issues does this PR fix or reference?

Fixes #25154 (the issue was closed with --static being suggested as a solution, however this to me is merely a workaround, as it makes for a different output strategy).

Previous Behavior

With show_jid enabled, piping into JSON processing tools leads to parsing exceptions.

New Behavior

stdout can be processed as JSON with show_jid enabled, the same way as with show_jid disabled.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

@tacerus
Send show_jid output to stderr
9425c5a 
When using --out=json, all data sent to stdout should be valid JSON to
allow for piping into processing tools such as `jq`.
Make this the case with "show_jid" being enabled as well, by printing
the JID line to stderr instead of stdout.

Signed-off-by: Georg Pfuetzenreuter <mail@georg-pfuetzenreuter.net>
@tacerus
Copy link
Contributor Author
tacerus commented Apr 10, 2025

I tried adding tests with

salt_cli.run("test.ping --out=json", minion_tgt=salt_minion.id)

in tests/pytests/integration/cli/test_salt.py and also attempted a few variations of placing the out option, but it seems none of these work, as --out=json is already implicitly added in salt_cli.run():

E           ProcessResult
E            Command Line: ['/home/georg/Work/git/salt/.nox/test-parametrized-3-crypto-none-transport-zeromq-coverage-false/bin/python', '/tmp/stsuite/scripts/c
li_salt.py', '--config-dir=/tmp/stsuite/master-GUfyDf/conf', '--timeout=30', '--out=json', '--out-indent=0', '--log-level=critical', 'minion-dCDa3r', 'test.ping
 --out=json']

Can I get regular / unfiltered CLI output in the test suite somehow?

@mattp-
Copy link
Contributor
mattp- commented Jun 2, 2025

this isn't really how you want to go about fixing this. salt.client really shouldn't be printing to stdout or stderr at all, but that's just an unfortunate side effect of how someone wrote this originally. you really want something like this:

diff --git c/salt/cli/salt.py w/salt/cli/salt.py
index a474cfc85ac..585490fd976 100644
--- c/salt/cli/salt.py
+++ w/salt/cli/salt.py
@@ -162,6 +162,7 @@ class SaltCMD(salt.utils.parsers.SaltCMDOptionParser):
                 kwargs["subset"] = self.options.subset
                 kwargs["cli"] = True
             else:
+                kwargs["output"] = self.config.get("output", "nested")
                 cmd_func = self.local_client.cmd_cli

             if self.options.progress:
@@ -191,7 +192,7 @@ class SaltCMD(salt.utils.parsers.SaltCMDOptionParser):
                 ret = {}
                 for full_ret in cmd_func(**kwargs):
                     try:
-                        ret_, out, retcode = self._format_ret(full_ret)
+                        ret_, out, retcode = self._format_ret(full_ret, output=kwargs.get("output", None))
                         retcodes.append(retcode)
                         self._output_ret(ret_, out, retcode=retcode)
                         ret.update(full_ret)
@@ -396,7 +397,7 @@ class SaltCMD(salt.utils.parsers.SaltCMDOptionParser):
             sys.stderr.write("ERROR: No return received\n")
             sys.exit(2)

-    def _format_ret(self, full_ret):
+    def _format_ret(self, full_ret, output=None):
         """
         Take the full return data and format it to simple output
         """
@@ -404,9 +405,15 @@ class SaltCMD(salt.utils.parsers.SaltCMDOptionParser):
         out = ""
         retcode = 0
         for key, data in full_ret.items():
-            ret[key] = data["ret"]
-            if "out" in data:
-                out = data["out"]
+            # theres an unfortunate hardcoded print() in
+            # salt.client.get_cli_event_returns that handles the default and
+            # text based output paths
+            if self.options.show_jid and output in {"json", "yaml"}:
+                ret[key] = data
+            else:
+                ret[key] = data["ret"]
+                if "out" in data:
+                    out = data["out"]
             ret_retcode = self._get_retcode(data)
             if ret_retcode > retcode:
                 retcode = ret_retcode
diff --git c/salt/client/__init__.py w/salt/client/__init__.py
index 5f5d0b24c0d..f01290414ac 100644
--- c/salt/client/__init__.py
+++ w/salt/client/__init__.py
@@ -810,6 +810,7 @@ class LocalClient:
         :returns: A generator
         """
         was_listening = self.event.cpub
+        output = kwargs.pop("output", None)

         try:
             self.pub_data = self.run_job(
@@ -835,6 +836,7 @@ class LocalClient:
                         tgt_type,
                         verbose,
                         progress,
+                        output=output,
                         **kwargs,
                     ):

@@ -1632,6 +1634,7 @@ class LocalClient:
         progress=False,
         show_timeout=False,
         show_jid=False,
+        output=None,
         **kwargs,
     ):
         """
@@ -1639,12 +1642,13 @@ class LocalClient:
         """
         log.trace("func get_cli_event_returns()")

-        if verbose:
-            msg = f"Executing job with jid {jid}"
-            print(msg)
-            print("-" * len(msg) + "\n")
-        elif show_jid:
-            print(f"jid: {jid}")
+        if output not in {"json", "yaml"}:
+            if verbose:
+                msg = f"Executing job with jid {jid}"
+                print(msg)
+                print("-" * len(msg) + "\n")
+            elif show_jid:
+                print(f"jid: {jid}")

         # lazy load the connected minions
         connected_minions = None

can you explore testing this patch and updating your PR?

@tacerus
Copy link
Contributor Author
tacerus commented Jun 4, 2025

Hi @mattp-, thanks for your input and the better solution - I will test, but from briefly reading over your patch, it seems it would cause the jid to not be printed at all if operating with the json or yaml output mode? I understand why, however think it would still be useful to have it available - maybe at least for JSON mode, should we print it as a separate JSON object?

@mattp-
Copy link
Contributor
mattp- commented Jun 4, 2025

no, it would. implicitly the jid is in the 'full_return'/raw load returned over the wire; i should have pasted an output example:

mphillips81@mmp-pw-dev /mnt/dev/repos/upstream-salt (sqla)✗ % uv run salt local -l info test.ping  --out=json --show-jid 2>/dev/null
{
    "local": {
        "ret": true,
        "retcode": 0,
        "jid": "20250604233927813243"
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

All data mixed on STDOUT together should generate valid JSON output
2 participants
@tacerus @mattp-
0