refactor: Add log_std(out|err) bools to repo_utils that execute a subprocess#2817
Conversation
| log_stdout = True, | ||
| log_stderr = True, |
There was a problem hiding this comment.
One alternative API option is to use a single string arg. Eg:
log_std = "both" # one of ["both", "stdout", "stderr"](I'd prefer an enum but this is starlark not python 🫠)
LMK which API people would prefer or if you have another option in mind. And please suggest a better arg name than log_std haha.
There was a problem hiding this comment.
I think the log_xxx API is fine. These are internal helpers, so easy to change if we want.
| ("stdout", result.stdout), | ||
| ("stderr", result.stderr), | ||
| ("stdout", result.stdout if log_stdout else "<log_stdout = False; skipping>"), | ||
| ("stderr", result.stderr if log_stderr else "<log_stderr = False; skipping>"), |
There was a problem hiding this comment.
Any tips on how this should be tested?
There was a problem hiding this comment.
I don't think we have any unit tests of these.
| {#v0-0-0-added} | ||
| ### Added | ||
| * Nothing added. | ||
| * Repo utilities `execute_unchecked`, `execute_checked`, and `execute_checked_stdout` now |
There was a problem hiding this comment.
The repo_utils apis are internal, so no need to document this in the user-facing changelog.
There was a problem hiding this comment.
Ack 👍, I'll keep that in mind for next time.
| ("stdout", result.stdout), | ||
| ("stderr", result.stderr), | ||
| ("stdout", result.stdout if log_stdout else "<log_stdout = False; skipping>"), | ||
| ("stderr", result.stderr if log_stderr else "<log_stderr = False; skipping>"), |
There was a problem hiding this comment.
I don't think we have any unit tests of these.
| log_stdout = True, | ||
| log_stderr = True, |
There was a problem hiding this comment.
I think the log_xxx API is fine. These are internal helpers, so easy to change if we want.
While making a local patch to work around #2640, I found that I had a need for running a subprocess (
gcloud auth print-access-token) viarepo_utils.execute_checked_stdout. However, doing so would log that access token when debug logging was enabled viaRULES_PYTHON_REPO_DEBUG=1. This is a security concern for us, so I hacked in an option to allow a particularexecute_(un)checked(_stdout)call to disable logging stdout, stderr, or both.I figure this might be useful to others so I thought I'd upstream it.
execute_(un)checked(_stdout)now supportlog_stdoutandlog_stderrbools that default toTrue(which is the same behavior as before this PR.When the subprocess writes to stdout and
log_stdout = False, the logged message will show:If the subprocess does not write to stdout, the debug log shows the same as before:
The above also applies for stderr, with text adjusted accordingly.