[Freeipa-devel] [PATCH] 0748 Handle encoding for ipautil.run

[Jan Cholasta]

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.

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0748.5-Refactor-ipautil.run.patch
Type: text/x-patch
Size: 69718 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151214/2de28e7d/attachment.bin>