[Freeipa-devel] [PATCHES] More Python 3 porting

[Petr Viktorin]

On 10/07/2015 10:27 AM, Jan Cholasta wrote:
> On 6.10.2015 12:04, Petr Viktorin wrote:
>> On 10/05/2015 07:56 AM, Jan Cholasta wrote:
>>> On 2.10.2015 13:09, Petr Viktorin wrote:
>>>> On 10/01/2015 03:15 PM, Jan Cholasta wrote:
>>>>> Hi,
>>>>>
>>>>> On 1.10.2015 13:01, Martin Basti wrote:
>>>>>>
>>>>>>
>>>>>> On 09/30/2015 10:25 AM, Petr Viktorin wrote:
>>>>>>> On 09/23/2015 04:46 PM, Petr Viktorin wrote:
>>>>>>>> On 09/22/2015 02:59 PM, David Kupka wrote:
>>>>>>>>> On 18/09/15 17:00, Petr Viktorin wrote:
>>>>>>>>>> Hello,
>>>>>>>>>> Here are more patches that bring IPA closer to Python 3
>>>>>>>>>> compatibility.
>>>> [...]
>>>>>>>
>>>>>> LGTM
>>>>>>
>>>>>> I ran xmlrpc tests, DNSSEC ci tests, backup and restore CI test and
>>>>>> everything works
>>>>>
>>>>> Patches 713-719: ACK
>>>>>
>>>>>
>>>>> Patch 720:
>>>>>
>>>>> You missed:
>>>>>
>>>>> ipa-client/ipa-install/ipa-client-install:32:    from ConfigParser
>>>>> import RawConfigParser
>>>>
>>>>
>>>> Thanks, fixed.
>>>>
>>>>> Patches 721-722: ACK
>>>>>
>>>>>
>>>>> Patch 723:
>>>>>
>>>>> Why the "NoneType = type(None)" in parameters.py? It is used only at:
>>>>>
>>>>> ipalib/parameters.py:388:    type = NoneType  # Ouch, this wont be
>>>>> very
>>>>> useful in the real world!
>>>>
>>>> I believe this is less confusing than `type = type(None)`, but I can
>>>> change that if needed.
>>>
>>> I don't care which one is used TBH, just that it is done consistently
>>> accross the whole patch, and this seemed like the simpler thing to do.
>>
>> OK, changed.
>>
>>>>> Patch 724:
>>>>>
>>>>> The SSHPublicKey class was written with the assumption that "str"
>>>>> means
>>>>> binary data, so unless I'm missing something, you only need to replace
>>>>> "str" with "bytes".
>>>>
>>>> It specifically did take non-binary data as str:
>>>>
>>>> -        if isinstance(key, str) and key[:3] != '\0\0\0':
>>>> -            key = key.decode(encoding)
>>>
>>> I don't follow, this is quite obviously binary data. It reads: "If key
>>> is binary and does not start with 3 null bytes, decode it to text using
>>> the specified encoding."
>>
>> Right, it's text (non-binary) data encoded in str (bytes), so it needs
>> to be encoded.
>>
>>>> I've removed this for Python 3, where text data shouldn't be in bytes.
>>>>
>>>> Since this means the '\0\0\0' check is skipped in __init__ under Python
>>>> 3, I've added it also to _parse_raw.
>>>
>>> When the SSH integration feature was first introduced, SSH public keys
>>> were stored in the raw binary form in LDAP, i.e. not text data. We still
>>> need to support that, so support for binary data and the 3 null check
>>> must remain in SSHPublicKey.
>>
>> Changed, updated patches attached.
> 
> Thanks, ACK.
> 
> I took the liberty of amending patch 718 to silence this pylint false
> positive I was getting on F22:
> 
> ipalib/plugins/otptoken.py:496: [E1101(no-member),
> HTTPSHandler.https_open] Instance of 'HTTPSHandler' has no 'do_open'
> member)

Thanks!

> Pushed to master: f82d3da1e8e5dc1d0716201af5abb724a8e78fde
> 
> BTW, in patch 724, binascii.Error is handled in addition to TypeError
> with base64.b64decode(). There are multiple places where
> base64.b64decode() is used in IPA where only TypeError is handled. Are
> you planning on fixing this as well?

I'll go through them to make sure we're handling them correctly.


-- 
Petr Viktorin