Use Popen in run_cmd and pipe outputs to logger #263
Conversation
There was a problem hiding this comment.
Nice changes! Please correct the behavior when the command fails and we are good to merge.
Hm, I just realized we don't have any tests for the function. Could you please throw one or two tests over here? https://github.com/user-cont/conu/blob/master/tests/integration/test_utils.py
| universal_newlines=True, | ||
| **kwargs) | ||
| else: | ||
| subprocess.check_call(cmd, **kwargs) |
There was a problem hiding this comment.
you should also check for return code of the command; if it's > 0, an exception should be raised
There was a problem hiding this comment.
Okay got it. That makes sense.
conu/utils/__init__.py
Outdated
| @@ -123,13 +123,13 @@ def run_cmd(cmd, return_output=False, ignore_status=False, **kwargs): | |||
| """ | |||
| logger.debug('command: "%s"' % ' '.join(cmd)) | |||
| try: | |||
| process = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, | |||
| universal_newlines=True, **kwargs) | |||
| output = process.communicate()[0] | |||
There was a problem hiding this comment.
I was hoping that communicate would iteratively print output after every newline; unfortunately, it returns all at once.
I guess it's fine for now, we can improve it later.
As a good example here is:
run_cmd(["bash", "-c", "for x in `seq 1 10`; do echo $x; sleep 1; done"])
|
@TomasTomecek Done! Made changes as you recommended! Please review it. |
tests/integration/test_utils.py
Outdated
|
|
||
|
|
||
| def test_run_cmd(): | ||
| ret = run_cmd(["sh", "-c", "for x in `seq 1 5`; do echo $x; sleep 1; done"]) |
There was a problem hiding this comment.
please let's sleep for 0.01 so we don't waste time
There was a problem hiding this comment.
Ah yeah of course. On it.
Edit. Done! Please review.
| else: | ||
| return cpe.returncode | ||
| return process.returncode |
There was a problem hiding this comment.
this is a bit confusing to me, but we can leave it for now
|
[test] |
|
moar [test] |
|
Once the CI is green, we can merge; nicely done! |
Fixes #211
Old behaviour
New behaviour