On 14.12.2015 10:49, Petr Viktorin wrote:
On 12/14/2015 10:29 AM, Jan Cholasta wrote:
On 9.12.2015 12:04, Petr Viktorin wrote:
On 12/04/2015 12:49 PM, Jan Cholasta wrote:
On 4.12.2015 12:43, Petr Viktorin wrote:
On 12/04/2015 12:15 PM, Jan Cholasta wrote:
On 4.12.2015 10:53, Petr Viktorin wrote:
On 12/04/2015 08:51 AM, Jan Cholasta wrote:
On 3.12.2015 15:42, Petr Viktorin wrote:
On 12/03/2015 10:55 AM, Jan Cholasta wrote:
[...]
If we do encode/decode in run(), I think there should be a way to
override the default encoding. My idea was to either accept/return
only
bytes and let the caller handle encoding themselves, or to handle
encoding in run(), but be able to override the defaults.
Given we handle encoding in run(), I would imagine it like this:
def run(args, stdin=None, raiseonerr=True, nolog=(),
env=None,
capture_stdout=False, skip_output=False, cwd=None,
runas=None, timeout=None, suplementary_groups=[],
capture_stderr=False, encode_stdout=False,
encode_stderr=False, encoding=None):
i.e. nothing is captured by default, when stdout or stderr are
captured
they are returned as bytes by default, when stdout or stderr are
returned as text they are decoded using the locale encoding by
default,
or the encoding can be explicitly specified.
Do you think this is feasible?
Feasible, yes.
In the majority of cases where the output is needed, we want text
encoded with locale.getpreferredencoding(), so I'd rather make
that the
default rather than bytes.
Or we could make "encode_stdout" imply "capture_stdout", so you
wouldn't
have to always specify both. (It would be better named
"encoded_stdout"
in that case.)
I think it should be OK to decode by default. (IMO it would be
best to
reverse the logic and name it "raw_stdout", with False default.
Do we
need "raw_stderr" at all? I don't think we do.)
Actually, now that I think about it: if the result was a namedtuple
subclass, we can always set extra raw_stdout/raw_stderr attributes on
it. (Unless skip_output=True, of course.)
result = run(['generate_binary_data'])
use(result.raw_stdout)
# and for backcompat,
rc, _out, _err = result
Good idea.
Perhaps we should use the same names as CalledProcessError for the
attributes, i.e. "returncode", "output", and "error_output",
"raw_output", "raw_error_output" for the new attributes?
(Actually, since "returncode" is not "return_code", it should probably
be "returncode", "output", "erroroutput", "raw_output",
"raw_erroroutput".)
OK. It's also good to use different names than Popen's stdout/stderr,
which are file-like objects rather than strings. But then,
capture_stdout/capture_stderr should be
capture_output/capture_error_output.
Here is the new patch.
I also added output_log and error_log to the result to abstract the
following common pattern:
result = run([...])
if result.returncode != 0:
stderr = result.error_output
if six.PY3:
error = stderr.encode(locale.getpreferredencoding(),
'replace')
self.log.critical('...: %s', stderr)
1) certutil -L -r outputs raw DER cert, so this will explode in Python 3
when pem == False:
else:
args.append('-r')
try:
- cert, err, returncode = self.run_certutil(args)
+ result = self.run_certutil(args, capture_output=True)
except ipautil.CalledProcessError:
raise RuntimeError("Failed to get %s" % nickname)
- return cert
+ return result.output
def has_nickname(self, nickname):
try:
2) There are some PEP8 transgressions:
$ git show -U0 | pep8 --diff
./ipaplatform/base/services.py:297:22: E127 continuation line
over-indented for visual indent
./ipapython/ipautil.py:279:1: E302 expected 2 blank lines, found 1
./ipapython/ipautil.py:280:40: E128 continuation line under-indented for
visual indent
./ipapython/ipautil.py:283:1: E302 expected 2 blank lines, found 1
./ipapython/ipautil.py:330:80: E501 line too long (80 > 79 characters)
./ipapython/ipautil.py:437:43: E127 continuation line over-indented for
visual indent
./ipapython/ipautil.py:442:43: E127 continuation line over-indented for
visual indent
./ipapython/kernel_keyring.py:57:11: E225 missing whitespace around
operator
./ipaserver/install/cainstance.py:626:80: E501 line too long (80 > 79
characters)
./ipaserver/install/ipa_backup.py:498:17: E128 continuation line
under-indented for visual indent
./ipaserver/install/ipa_backup.py:521:21: E122 continuation line missing
indentation or outdented
./ipaserver/install/ipa_backup.py:530:17: E122 continuation line missing
indentation or outdented
./ipaserver/install/ipa_backup.py:605:52: E225 missing whitespace around
operator
./ipaserver/install/ipa_backup.py:606:17: E122 continuation line missing
indentation or outdented
./ipaserver/install/krbinstance.py:303:80: E501 line too long (84 > 79
characters)
./ipatests/test_ipapython/test_keyring.py:97:33: E222 multiple spaces
after operator
3) The patch needs a rebase.
I took the liberty of fixing all of the above, see the attached patch.
ACK from me. If you are OK with the changes, I will push the patch.
I am, ACK. Thank you!
Pushed to master: 099cf98307d4b2f0ace5d5e28754f264808bf59d
--
Jan Cholasta
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
