subp cleanup and typing #4555
subp cleanup and typing #4555
Conversation
|
I know you said not ready for review yet, but broadly I'm -1 on many of the changes. In particular, I don't think that subp args or ProcessExecutionError should be restricted to bytes. I see no reason why we should need to translate to bytes when calling it. That just puts more complexity at the call sites when the complexity can be encapsulated in the subp call or the exception class, and it leaks bytes too much into the inside of the application. If I were to go either way, I'd go for only accepting strings rather than only accepting bytes, but I think it's nice that we can take either and generally do the right thing. Do you see broken/unhelpful typing because of the current state? What was the impetus for this change? Also, looks like I let in a small typing "regression" during one of my rebases of the snap PR. Adding that back in will probably help dealing with subp responses. |
|
Thanks for the early feedback on this one @TheRealFalcon. Much of this was early braindump solution to being dissatisfied with type handling in the subp module, since it makes the logic unnecessarily difficult to follow when debugging issues related to this. I'll keep your feedback in mind, but respond when I'm actually ready for review. |
holmanb
force-pushed
the
holmanb/subp-cleanup
branch
from
5d07db0 to
248f5c9
Compare
holmanb
force-pushed
the
holmanb/subp-cleanup
branch
2 times, most recently
from
7254a02 to
9de4ab2
Compare
holmanb
force-pushed
the
holmanb/subp-cleanup
branch
2 times, most recently
from
e15c9db to
f18f5eb
Compare
…nical#4555) The only consumer of this kwarg was cc_snap, which was printing output directly to stderr rather than using intended logging code, which it shouldn't be doing.
…ical#4555) This parameter has a single callsite. Configuration modules shouldn't send output directly to stdout / stderr.
Added in Python 3.3, we can safely use this Python feature.
Remove code for unused argument types.
holmanb
force-pushed
the
holmanb/subp-cleanup
branch
from
f18f5eb to
417bb33
Compare
…al#4555) - Remove redundant code from the few callsites that used this parameter. - Eliminate tests for behavior that was never used.
…ical#4555) This parameter has a single callsite. Configuration modules shouldn't send output directly to stdout / stderr.
Added in Python 3.3, we can safely use this Python feature.
Remove code for unused argument types.
…al#4555) - Remove redundant code from the few callsites that used this parameter. - Eliminate tests for behavior that was never used.
holmanb
force-pushed
the
holmanb/subp-cleanup
branch
from
417bb33 to
b418e3d
Compare
…al#4555) - Remove redundant code from the few callsites that used this parameter. - Eliminate tests for behavior that was never used.
holmanb
force-pushed
the
holmanb/subp-cleanup
branch
from
b418e3d to
95cb638
Compare
…nical#4555) The only consumer of this kwarg was cc_snap, which was printing output directly to stderr rather than using intended logging code, which it shouldn't be doing.
…ical#4555) This parameter has a single callsite. Configuration modules shouldn't send output directly to stdout / stderr.
holmanb
force-pushed
the
holmanb/subp-cleanup
branch
from
95cb638 to
70632f0
Compare
Added in Python 3.3, we can safely use this Python feature.
Remove code for unused argument types.
|
Some really nice changes here, thanks! First pass - most commits look good, but I'm uncomfortable with Add type information to args commit. It's not entirely correct: but even if we do satisfy pyright, there's enough subp calls where the args can be combined from multiple different sources that I don't think I'd be comfortable with the changes until we have test coverage of all the call points. If we want to annotate it, I think we should use |
Good point. I'd like to see pyright / mypy benefit from knowing the expected types of Future work:Per the document you linked above, if we can get away from requiring support for byte inputs to subp someday (outside of the |
holmanb
force-pushed
the
holmanb/subp-cleanup
branch
from
541e449 to
9d5fe5d
Compare
cloudinit/config/cc_snap.py
Outdated
| @@ -174,7 +173,7 @@ def run_commands(commands): | |||
| for command in fixed_snap_commands: | |||
| shell = isinstance(command, str) | |||
| try: | |||
| subp.subp(command, shell=shell, status_cb=sys.stderr.write) | |||
| subp.subp([command], shell=shell) | |||
There was a problem hiding this comment.
Did you look for history on this status_cb? There might've been a reason we're sending those logs to stderr. I'm guessing it's fine, but lets do our due diligence if we haven't yet.
Also, making this a list is in the wrong commit (remove env parameter), and I'd rather not change it if we're still supporting strings for now.
There was a problem hiding this comment.
Did you look for history on this status_cb? There might've been a reason we're sending those logs to stderr. I'm guessing it's fine, but lets do our due diligence if we haven't yet.
I did, yes. This parameter came with the original addition of the cc_snap module, and no justification for this parameter was given in the commit message, nor clues left in the code as to why this was deemed necessary.
I don't see how this additional text or the output location would significantly change a user's debugging experience. With and without status_cb, successful calls end up in the logs (albeit without the extra Begin/End log line). With and without status_cb, failed commands end up with trackbacks in the logs.
Also, making this a list is in the wrong commit (remove env parameter), and I'd rather not change it if we're still supporting strings for now.
Oops, fixed.
| devnull_fp.close() | ||
|
|
||
| # Just ensure blank instead of none. | ||
| if capture or combine_capture: |
There was a problem hiding this comment.
Don't we still want this in the capture case?
There was a problem hiding this comment.
That's not necessary (it never was).
The docs:
Popen.communicate(input=None, timeout=None)
... If streams were opened in text mode, input must be a string. Otherwise, it must be bytes.
communicate() returns a tuple (stdout_data, stderr_data). The data will be strings if streams were opened in text mode; otherwise, bytes.
Note that if you want to send data to the process’s stdin, you need to create the Popen object with stdin=PIPE. Similarly, to get anything other than None in the result tuple, you need to give stdout=PIPE and/or stderr=PIPE too.
Popen.stdout
If the stdout argument was PIPE, this attribute is a readable stream object as returned by open(). Reading from the stream provides output from the child process. If the encoding or errors arguments were specified or the text or universal_newlines argument was True, the stream is a text stream, otherwise it is a byte stream. If the stdout argument was not PIPE, this attribute is None.
Popen.stderr
If the stderr argument was PIPE, this attribute is a readable stream object as returned by open(). Reading from the stream provides error output from the child process. If the encoding or errors arguments were specified or the text or universal_newlines argument was True, the stream is a text stream, otherwise it is a byte stream. If the stderr argument was not PIPE, this attribute is None.
The relevant bits:
if capture:
stdout = subprocess.PIPE
stderr = subprocess.PIPE
...
try:
sp = subprocess.Popen(
bytes_args,
stdout=stdout,
stderr=stderr,
stdin=stdin,
env=env,
shell=shell,
cwd=cwd,
)
(out, err) = sp.communicate(data)The encoding or errors arguments were not specified and text and universal_newlines argument are not True, therefore stdout and stderr are byte streams, and the data returned in the tuple will be bytes.
The None-check code that exists now, I believe, was due to combined_capture using stderr = subprocess.STDOUT, and therefore returning None in the first element of the tuple. That plus maybe the assumption that the same was possible for the first argument in the tuple for some reason and when using capture. tl;dr Not sure why the current code is this way, but it doesn't look right.
There was a problem hiding this comment.
@TheRealFalcon I suppose it would probably be better to include a note in the commit message summarizing this (or making it its own separate commit) so that this doesn't look like a mistake in the git log. I'll do that on merge, if you are fine with that.
| cmd2 = "bogus command" | ||
| cmd3 = 'echo "MOM" >> %s' % outfile | ||
| commands = [cmd1, cmd2, cmd3] | ||
| @pytest.mark.allow_all_subp |
There was a problem hiding this comment.
Nice conversion to pytest, but our docs still say that all unit tests should live in a class. I'd be fine removing that (and making an exception here if we remove it right away), but I've tried to keep to that rule until now.
There was a problem hiding this comment.
I'm fine with being flexible with that rule if no class grouping makes sense (otherwise it is just boilerplate), but for now I'll just toss them in a class.
|
@TheRealFalcon Thanks for the review! I think I've addressed everything. |
Thanks!
Agreed, no rush on this one. |
The only consumer of this kwarg was cc_snap, which was printing output directly to stderr rather than using intended logging code, which it shouldn't be doing.
This parameter has a single callsite. Configuration modules shouldn't send output directly to stdout / stderr. Note: Regarding the code below the comment "Just ensure blank instead of none" 'None' output was only possible for the stderr tuple member output of .communicate() when stderr was directed to STDOUT. This only occurred in the case of the 'combine_capture' argument, and only for stderr, not stdout. This removes the unnecessary assignments for the 'capture' argument, as well as the no-longer-necessary assignments for 'capture_parameter'. See the docs[1] for more details. [1] https://docs.python.org/3/library/subprocess.html
Added in Python 3.3, we can safely use this Python feature.
Remove code for unused argument types.
…l#4555) This parameter always receives the same argument in runtime code and adds no value. Remove it.
- Remove redundant code from the few callsites that used this parameter. - Eliminate tests for behavior that was never used.
holmanb
force-pushed
the
holmanb/subp-cleanup
branch
from
a716209 to
85dc3b7
Compare
|
while you're refactoring this hot mess, is there any way we can add the exit status to the SubpResult? it is in the ProcessExecutionError but it's not very easily accessible |
The only consumer of this kwarg was cc_snap, which was printing output directly to stderr rather than using intended logging code, which it shouldn't be doing.
This parameter has a single callsite. Configuration modules shouldn't send output directly to stdout / stderr. Note: Regarding the code below the comment "Just ensure blank instead of none" 'None' output was only possible for the stderr tuple member output of .communicate() when stderr was directed to STDOUT. This only occurred in the case of the 'combine_capture' argument, and only for stderr, not stdout. This removes the unnecessary assignments for the 'capture' argument, as well as the no-longer-necessary assignments for 'capture_parameter'. See the docs[1] for more details. [1] https://docs.python.org/3/library/subprocess.html
Added in Python 3.3, we can safely use this Python feature.
Remove code for unused argument types.
This parameter always receives the same argument in runtime code and adds no value. Remove it.
Improve code quality and reduce maintenance burden by removing boilerplate, old unused copy-pasted code, and redundant features. Update code to use current Python3 features. Add typing information to subp args. This PR reduces code complexity, improves static analysis via typing, and eliminates the need to test internal features that we didn't even use.