Conversation
RobDover
left a comment
RobDover
left a comment
There was a problem hiding this comment.
Looks solid, but I noticed a few small things to fix up.
| error_returncodes = [] | ||
| for command_args in list_of_command_args: | ||
| if namespace: | ||
| command_args[0:0] = ['ip', 'netns', 'exec', namespace] |
There was a problem hiding this comment.
I don't really like this syntax (command_args = ['ip', 'netns', 'exec', namespace] + command_args seems more readable to me) but I guess this is the existing code so I'm not super-bothered.
There was a problem hiding this comment.
#maketheworldabetterplace
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| close_fds=True) | ||
| processes.append(p) |
There was a problem hiding this comment.
What about:
processes = [subprocess.Popen(command_args,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
close_fds=True)
for command_args in list_of_command_args]
Then you don't need line 27 and you don't have to keep this inside the for loop.
| if error_returncodes: | ||
| return error_returncodes[0] | ||
| else: | ||
| return 0 |
There was a problem hiding this comment.
This is much more neatly written as:
return next(error_returncodes, 0)
There was a problem hiding this comment.
FYI this doesn't work:
File "/usr/share/clearwater/clearwater-queue-manager/env/local/lib/python2.7/site-packages/metaswitch/clearwater/etcd_shared/plugin_utils.py", line 65, in run_commands
return next(error_returncodes, 0)
TypeError: list object is not an iterator
return next(iter(error_returncodes), 0) is a bit of a mouthful, so I'm going back to the original.
| "stdout {!r}, and stderr {!r}".format(' '.join(command_args), | ||
| p.returncode, | ||
| stdout, | ||
| stderr)) |
There was a problem hiding this comment.
This is going to look ugly if there are multiple lines of stdout/stderr. Is it feasible to make this a bit more multi-line friendly? eg:
_log_error("Command {} failed with return code {}.\n"
"stdout:\n{!r}"
"stderr:\n{!r}")
I can see that this may not work if we're logging in a manner that doesn't interact well with multi-line logs though. If that's the case, how about:
_log_error("Command {} failed with return code {}."
if stdout:
_log_error("stdout: {!r}")
if stderr:
_log_error("stderr: {!r}")
| error_returncodes.append(p.returncode) | ||
| else: | ||
| _log.debug("Command {} succeeded".format(' '.join(command_args))) | ||
| # it succeeded, log out stderr of the command run if present |
There was a problem hiding this comment.
The lower case here sets my teeth on edge. Can you fix it while you're here (or even just get rid of it - I don't think it's adding very much).
|
|
||
| def run_command(command_args, namespace=None, log_error=True): | ||
| def run_commands(list_of_command_args, namespace=None, log_error=True): | ||
| """Runs the given shell command, logging the output and return code. |
There was a problem hiding this comment.
Docstring needs updating.
| _log.warning("Command {} succeeded, with stderr output {!r}". | ||
| format(' '.join(command_args), stderr)) | ||
| processes = [] | ||
| error_returncodes = [] |
There was a problem hiding this comment.
Would prefer this to live next to the stanza of code that uses it.
| return 0 | ||
|
|
||
|
|
||
| def run_command(command_args, namespace=None, log_error=True): |
There was a problem hiding this comment.
No docstring. You're using an interesting design pattern here (the function to run a single command being a call to the one that runs multiple), so it's worth explaining why you've chosen to use it.
There was a problem hiding this comment.
Is it actually that interesting or worth commenting on? It feels like the obvious thing to me - it's just the special case of run_commands where N=1, and anything else would mean duplicated code.
There was a problem hiding this comment.
I'd have thought the obvious way round to do something like this is:
run_command()contains the code for running a single commandrun_commands()contains the code for iteratingrun_command()as necessary
There are good reasons why you chose not to do it that way, but if I were coming to the code fresh I might expect to find how a command is being run under run_command and that's not obvious here.
| for p in processes: | ||
| stdout, stderr = p.communicate() | ||
| if p.returncode != 0: | ||
| # it failed, log the return code and output |
There was a problem hiding this comment.
Similarly, can we tidy this up while we're here?
2 participants
Not tested yet. My plan is to refactor https://github.com/Metaswitch/clearwater-etcd-plugins/blob/master/clearwater_queue_manager/apply_config_plugin.py#L32 to use this.