-
-
Notifications
You must be signed in to change notification settings - Fork 586
refactor: Add log_std(out|err) bools to repo_utils that execute a subprocess #2817
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.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any tips on how this should be tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have any unit tests of these.
@@ -77,7 +77,9 @@ END_UNRELEASED_TEMPLATE | |||
|
|||
{#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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repo_utils apis are internal, so no need to document this in the user-facing changelog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have any unit tests of these.
log_stdout = True, | ||
log_stderr = True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_stdout
andlog_stderr
bools 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.