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

[Jan Cholasta]

On 24.1.2014 20:31, Petr Viktorin wrote:
> 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 :/

+1, I wish there was more time for this.

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

Thanks for the review.

-- 
Jan Cholasta