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

[Jan Cholasta]

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)

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?

Honza

-- 
Jan Cholasta