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

[Jan Cholasta]

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.

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

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


-- 
Jan Cholasta