Allow piped stdin in run_subprocess_with_logging #863
Conversation
Sometimes we need to used as stdin the output from another command (e.g. pass secure settings to elasticsearch-keystore tool is one example). This commit optionally allows passing stdin from another subprocess to run_subprocess_with_logging.
dliappis
added
enhancement
esrally/utils/process.py
Outdated
| @@ -60,13 +60,15 @@ def exit_status_as_bool(runnable, quiet=False): | |||
| return False | |||
|
|
|||
|
|
|||
| def run_subprocess_with_logging(command_line, header=None, level=logging.INFO, env=None): | |||
| def run_subprocess_with_logging(command_line, header=None, level=logging.INFO, stdin=None, env=None): | |||
There was a problem hiding this comment.
I didn't find any existing tests for this, so deferred -- for now -- writing new ones.
And also only log piped output if it exists # Conflicts: # esrally/utils/process.py
dliappis
force-pushed
the
allow-pipes-with-logging-subprocess
branch
from
d6b778c to
2f263cb
Compare
|
Rebased #859 into this one. |
esrally/utils/process.py
Outdated
| env=env, | ||
| stdin=stdin if stdin else None, | ||
| preexec_fn=pre_exec) as command_line_process: | ||
| if stdin: |
There was a problem hiding this comment.
How about we unify the block's body to:
stdout, _ = command_line_process.communicate()
if stdout:
logger.log(level=level, msg=stdout)Note: In any case we should also pass universal_newlines=True to subprocess.Popen to ensure line-endings are properly treated. This did not work before as well (meaning we see \n at the end of each line in the log file) but wasn't so much of a problem because we wrote a log message for each line.
There was a problem hiding this comment.
#communicate() will wait until the process completes. As a result the existing code (including looping through command_line_process.stdout.readline() and "streaming" the results) won't work.
Or are you suggesting replacing the everything we currently have within the with: block with the above 3 lines?
There was a problem hiding this comment.
I am suggesting that we replace the whole block with the above three lines. Given the we use it only for short-lived subprocesses anyway, I think it is ok to "lose" the streaming aspect.
There was a problem hiding this comment.
Ok! With this approach in mind I pushed 0631661
Sometimes we need to used as stdin the output from another command
(e.g. pass secure settings to elasticsearch-keystore tool is
one example).
This commit optionally allows passing stdin from another subprocess
to run_subprocess_with_logging.