Fix call to Popen.communicate #636
Conversation
| @@ -60,7 +60,7 @@ def remove_lvm_physical_volume(block_device): | |||
| ''' | |||
| p = Popen(['pvremove', '-ff', block_device], | |||
| stdin=PIPE) | |||
| p.communicate(input='y\n') | |||
| p.communicate(input=b'y\n') | |||
There was a problem hiding this comment.
Whilst I'm generally in agreement here, I'm a little worried this might be incompatible across versions of python3. e.g. the original change was 8 years ago, so it's worked up till today. I'm going to nack this from landing until we've verified that it works across py3.6 -> py3.9 (?py 3.10?).
There was a problem hiding this comment.
Yes, that does sound better, and it's similar to what create_logical_volume does via lvcreate. What do you think, @ajkavanagh ?
There was a problem hiding this comment.
I'm +1 on passing --yes and replacing the use of Popen with subprocess.check_call.
| @@ -60,7 +60,7 @@ def remove_lvm_physical_volume(block_device): | |||
| ''' | |||
| p = Popen(['pvremove', '-ff', block_device], | |||
| stdin=PIPE) | |||
| p.communicate(input='y\n') | |||
| p.communicate(input=b'y\n') | |||
There was a problem hiding this comment.
I'm +1 on passing --yes and replacing the use of Popen with subprocess.check_call.
No, but I've been using it as part of running the functional tests locally for the cinder-lvm charm. |
The call is wrongly passing a string when only bytes-like are allowed.
This PR modified the call to no longer use 'Popen', but it was missing the corresponding fix in the unit test for said function.
freyes
force-pushed
the
fix-popen-communicate
branch
from
ff66ab1 to
e44a15e
Compare
|
Rebased on top of master. |
The call is wrongly passing a string when only bytes-like are allowed.