[Freeipa-devel] [PATCHES] 225-230 Drop support for the legacy LDAP API

[Petr Viktorin]

On 01/22/2014 05:47 PM, Jan Cholasta wrote:
> On 20.1.2014 12:23, Petr Viktorin wrote:
>> On 01/14/2014 11:31 AM, Jan Cholasta wrote:
>>> On 10.1.2014 16:02, Petr Viktorin wrote:
>>>> On 01/07/2014 01:54 PM, Jan Cholasta wrote:
>>>>> On 16.12.2013 14:45, Petr Viktorin wrote:
>>>>>> On 12/16/2013 10:22 AM, Jan Cholasta wrote:
>>>>>>> On 13.12.2013 15:16, Petr Viktorin wrote:
>>>>>>>> On 12/10/2013 04:05 PM, Jan Cholasta wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> I believe the time has come to drop support for the legacy (dn,
>>>>>>>>> entry_attrs) tuple API and move to the new LDAPEntry API
>>>>>>>>> exclusively.
>>>>>>>>> The attached patches convert existing code which uses the old
>>>>>>>>> API to
>>>>>>>>> the
>>>>>>>>> new API and remove backward compatibility code from the ipaldap
>>>>>>>>> module.
>>>>>>>>>
>>>>>>>>> Note that there are still some functions/methods which accept
>>>>>>>>> separate
>>>>>>>>> dn and entry_attrs arguments, they will be adapted to LDAPEntry
>>>>>>>>> later.
>>>>>>>>>
>>>>>>>>> Honza
>>>>>>>>
>>>>>>>> The first N-1 patches can be tested,acked,pushed independently,
>>>>>>>> right?
>>>>>>>
>>>>>>> Yes.
>>>>>>>
>>>>>>>> If that's the case, ACK for 225
>>>>>>
>>>>>> Pushed that one to master, 5 more to go.
>>>>>> bc3f3381c6bf0b4941889b775025a60f56318551
>>>>>>
>>>>
>>>> 226 needs a rebase.
>>>>
>>>> 227: in install/tools/ipa-adtrust-install:
>>>>
>>>> +        entry_attrs = conn.make_entry(
>>>> +            dn,
>>>> +            objectclass=['top', 'pkiuser', 'nscontainer'],
>>>> +            usercertificate=cert)
>>>> +        conn.add_entry(entry_attrs)
>>>>
>>>> Shouldn't it be `usercertificate=[cert]` now?  Similarly in ra_cert,
>>>> and
>>>> in ipa-server-install with ipacertificatesubjectbase
>>>>
>>>> Otherwise this looks good.
>>>>
>>>> 228: in ipaserver/install/plugins/update_idranges.py, again we should
>>>> use lists
>>>>
>>>> Otherwise it looks good
>>>>
>>>> 229: ACK
>>>>
>>>
>>> Rebased and fixed everything, updated patches attached.
>>
>> Here, patch 226 breaks tests for selinuxusermap_enable/disable, at
>> least. The EmptyModlist and AlreadyActive/AlreadyInactive error is no
>> longer raised, since the previous entry state is no longer retrieved.
>>
>
> Well, I forgot to test this patchset after patches for
> <https://fedorahosted.org/freeipa/ticket/3488> were pushed, sorry.
>
> Added new patch 235 which makes LDAPUpdate get old entry state from
> LDAP, it fixes most of the issues in *_mod commands.

If getting the entry is really always needed then I see no real reason 
why it's not done at the beginning of execute() and made available 
throughout the command's execution. It would be so much easier to do 
non-trivial work in the callbacks. And more efficient, too, compared to 
the current way of always asking LDAP when the original entry is needed.
Ah well. One day we can rewrite the whole thing :/
For now, ACK

> Fixed the rest of the issues in patches 226-230 and rebased them on top
> of patch 235.

ACK, pushed to master: 5737eaf1348ba101ae227fa79fb4451a2413fc84

-- 
Petr³