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

[Petr Viktorin]

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."

Sorry, I meant binary data.

>> 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.
> 
>>
>> It's not necessary to dispatch to "_parse_raw" or "_parse_base64 or
>> _parse_openssh" based on type, but I believe this makes the control flow
>> clearer to follow.
>>
>>> Patch 725: ACK
>>
>>
> 
> 


-- 
Petr Viktorin