Fix #8 Add basic stdin support to Subprocess and Paramiko #20

espositocloud · 2018-04-12T14:31:54Z

What do these changes do?

Add basic stdin support to Subprocess and Paramiko.

Are there changes in behavior for the user?

The user can provide some content for stdin that will be available to the process.

Related issue number

Fix #8

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes

coveralls · 2018-04-12T14:44:18Z

Pull Request Test Coverage Report for Build 83

10 of 19 (52.63%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-1.0%) to 98.06%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
exec_helpers/exec_result.py	4	5	80.0%
exec_helpers/_ssh_client_base.py	5	9	55.56%
exec_helpers/subprocess_runner.py	1	5	20.0%

Totals
Change from base Build 77:	-1.0%
Covered Lines:	910
Relevant Lines:	928

💛 - Coveralls

coveralls · 2018-04-12T14:44:18Z

Pull Request Test Coverage Report for Build 73

2 of 3 (66.67%) changed or added relevant lines in 1 file are covered.
5 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.5%) to 98.48%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
exec_helpers/exec_result.py	2	3	66.67%

Files with Coverage Reduction	New Missed Lines	%
exec_helpers/proc_enums.py	1	99.07%
exec_helpers/subprocess_runner.py	4	91.09%

Totals
Change from base Build 72:	-0.5%
Covered Lines:	907
Relevant Lines:	921

💛 - Coveralls

penguinolog · 2018-04-12T14:51:10Z

I missed 2nd part for SSH (it should look like copy-paste from Subprocess now, just stdin input in execute_async)

penguinolog

Let's Update ExecResult arguments order (the single object, where it's technically allowed) and reduce encode/decode operations

penguinolog · 2018-04-12T14:44:31Z

doc/source/ExecResult.rst

    Command execution result.

-    .. py:method:: __init__(cmd, stdout=None, stderr=None, exit_code=ExitCodes.EX_INVALID)
+    .. py:method:: __init__(cmd, stdout=None, stderr=None, exit_code=ExitCodes.EX_INVALID, stdin=None)


probably for ExecResult we can use urder: stdin, stdout, stderr as human-expected

Indeed that's more natural. I added it at the end only for consistency with the rest (where stdin is the latest for backward compatibility).

ExecResult class is exported for typing purposes only. It was not expected for re-use outside, except reading

penguinolog · 2018-04-12T14:47:54Z

exec_helpers/subprocess_runner.py

            )
+            if stdin is not None:
+                if not isinstance(stdin, six.text_type):
+                    stdin = six.u(stdin)


better to decode for ExecResult inside ExecResult and encode for stdin only if required (less transformations)

penguinolog · 2018-04-12T14:49:27Z

test/test_subprocess_runner.py

+    def test_check_stdin(self, check_call, logger):
+        print_stdin = 'cat <&0'
+        stdin = 'input data'
+        if not isinstance(stdin, six.text_type):


we have full control over tests - better to test both cases:
stdin = b'input data and stdin = u'input data'

penguinolog · 2018-04-13T08:00:18Z

doc/source/ExecResult.rst


        :param cmd: command
        :type cmd: ``str``
+        :param stdout: STDIN


copy-paste error: should be :param stdin: STDIN and on the next line :type stdin: ...

penguinolog · 2018-04-13T08:01:40Z

doc/source/SSHClient.rst

        :param open_stderr: open STDERR stream for read
        :type open_stderr: bool
+        :param stdin: pass STDIN text to the process
+        :type stdin: ``typing.Optional[str]``


typing.Optional[bytes] and need to check for bytearray support (both Subprocess and SSH)

penguinolog · 2018-04-13T08:02:31Z

doc/source/Subprocess.rst

        .. versionchanged:: 1.2.0

-            open_stdout and open_stderr flags
+            open_stdout, open_stderr and stdin flags


stdin separately - it's not flag, but data

penguinolog · 2018-04-13T08:03:09Z

exec_helpers/_ssh_client_base.py


-        stdin = chan.makefile('wb')
+        _stdin = chan.makefile('wb')
+        if stdin is not None:


sudo should be processed before

penguinolog · 2018-04-13T08:04:09Z

exec_helpers/exec_result.py


        self.__cmd = cmd
+        if stdin is not None and not isinstance(stdin, six.text_type):
+            stdin = six.u(stdin)


stdin = stdin.decode('utf-8')

not six.text_type identifies e.g. non-unicode strings

>>> b'dede'.decode('utf-8') 'dede'

Decode non-unicode to unicode

and for exec_result errors='backslashreplace' is actual parameter, because we can have true binary data

penguinolog · 2018-04-13T15:14:14Z

exec_helpers/_ssh_client_base.py

-            paramiko.ChannelFile,
-            typing.Optional[paramiko.ChannelFile],
-            typing.Optional[paramiko.ChannelFile],
+            typing.Any,


do not change this, typing.Any will be used in the base class later (2.0.0+)

penguinolog · 2018-04-13T15:14:28Z

exec_helpers/_ssh_client_base.py


        :param command: Command for execution
        :type command: str
-        :param get_pty: open PTY on remote machine


this argument is not removed

penguinolog · 2018-04-13T15:14:39Z

exec_helpers/_ssh_client_base.py

+        verbose=False,  # type: bool
+        log_mask_re=None,  # type: typing.Optional[str]
        **kwargs
-    ):  # type: (...) -> _type_execute_async


do not remove this typing

penguinolog · 2018-04-13T15:15:15Z

exec_helpers/_ssh_client_base.py

-        return chan, stdin, stderr, stdout
+        if stdin is not None:
+            if not isinstance(stdin, six.text_type):
+                stdin = six.u(stdin)


use encode/decode

penguinolog · 2018-05-01T13:00:58Z

doc/source/ExecResult.rst

        :param cmd: command
        :type cmd: ``str``
+        :param stdin: STDIN
+        :type stdin: ``str``


typing.Optional[str]

penguinolog · 2018-05-01T13:02:04Z

exec_helpers/_ssh_client_base.py

-        .. versionchanged:: 1.2.0 open_stdout and open_stderr flags
+        .. versionchanged:: 1.2.0
+
+            open_stdout and open_stderr flags


make several records like Subprocess.rst

penguinolog · 2018-05-01T13:04:24Z

exec_helpers/subprocess_runner.py

 from __future__ import unicode_literals

 import collections
+import errno


linux only -> we need separation linux/posix/windows

… Paramiko

espositocloud requested review from dis-xcom and penguinolog as code owners April 12, 2018 14:31

penguinolog requested changes Apr 12, 2018

View reviewed changes

penguinolog requested changes Apr 13, 2018

View reviewed changes

espositocloud force-pushed the fix-8 branch from 6a46034 to 1aa438c Compare April 23, 2018 10:21

espositocloud changed the title ~~Fix #8 Add basic stdin support to Subprocess~~ Fix #8 Add basic stdin support to Subprocess and Paramiko Apr 23, 2018

espositocloud force-pushed the fix-8 branch from 1aa438c to e5f266e Compare May 1, 2018 11:25

penguinolog reviewed May 1, 2018

View reviewed changes

penguinolog previously approved these changes May 1, 2018

View reviewed changes

Fix python-useful-helpers#8 Add basic stdin support to Subprocess and…

544e6fe

… Paramiko

espositocloud dismissed penguinolog’s stale review via 544e6fe May 1, 2018 14:35

espositocloud force-pushed the fix-8 branch from 69f94c5 to 544e6fe Compare May 1, 2018 14:35

penguinolog approved these changes May 1, 2018

View reviewed changes

espositocloud merged commit 5f107c0 into python-useful-helpers:master May 1, 2018

espositocloud deleted the fix-8 branch May 1, 2018 14:51

Fix #8 Add basic stdin support to Subprocess and Paramiko #20

Fix #8 Add basic stdin support to Subprocess and Paramiko #20

Uh oh!

Conversation

espositocloud commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

Uh oh!

coveralls commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 83

💛 - Coveralls

Uh oh!

coveralls commented Apr 12, 2018

Pull Request Test Coverage Report for Build 73

💛 - Coveralls

Uh oh!

penguinolog commented Apr 12, 2018

Uh oh!

penguinolog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

penguinolog Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

espositocloud Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

espositocloud commented Apr 12, 2018 •

edited

Loading

coveralls commented Apr 12, 2018 •

edited

Loading

penguinolog Apr 12, 2018 •

edited

Loading

espositocloud Apr 13, 2018 •

edited

Loading