[Freeipa-devel] [PATCHES] 0191-0195 Use ipaldap in the client installer & password migration

[Jan Cholasta]

On 8.3.2013 14:14, Petr Viktorin wrote:
> On 03/07/2013 05:42 PM, Jan Cholasta wrote:
>> Patch 191:
>>
>> The patch is missing the ipapython/ipaldap.py file.

On 7.3.2013 18:29, Petr Viktorin wrote:
 > It's there, it's just copied from ipaserver/ipaldap.py with a small
 > change at the bottom.

There is no sign of the file, except in the patch header and the patch 
cannot be applied with git am nor with git apply. But perhaps I'm doing 
something wrong.

>>
>> I think it should go into ipalib instead of ipapython. <rant> It doesn't
>> make sense to keep ipapython and ipalib separate if they depend on each
>> other. We should either merge them or clean up the mess by removing
>> ipalib imports from ipapython. I'm not saying we should do it now, just
>> please don't add new modules to ipapython which import from ipalib.
>> </rant>
>
> This is a bigger problem.
> Conceptually ipaldap should be in ipapython, it just needs the
> problematic errors and text modules. I think we should move those rather
> than ipaldap.

Moving test to ipapython makes sense. I think we should have a separate 
set of errors for ipaldap and translate them to framework errors in ipalib.

>
>> Also I am not very fond of the "ipa" prefix in "ipaldap". The module
>> lives in the namespace of our own package, so there's no need for it to
>> have such a prefix, is there?
>
> It's nice to have a unique name for talking about it.
> Since "ldap" is already a package we use, having another "ldap" would be
> confusing, even if it is properly namespaced.

Since ipaldap should be the sole user of python-ldap after the 
refactoring is done, I don't think there would be any confusion. But 
whatever, I'm fine with both.

>
>> Patch 193:
>>
>> +            scope=conn.SCOPE_BASE,
>> +            filter='objectclass=pkiCA',
>> +            attrs_list=[ca_cert_attr],
>>
>> Can we use a proper filter here please?
>>
>> +    :param conn: Bound LDAPConnection that will be used for searching
>
> Fixed, thanks
>
>> LDAPClient
>>
>> Patch 194:
>>
>> -                ldap.set_option(ldap.OPT_X_TLS_REQUIRE_CERT, True)
>>
>> and
>>
>> -                lh.set_option(ldap.OPT_X_TLS_DEMAND, True)
>>
>> Is removing these options safe?
>
> I re-added them.
>
> I also removed the forgotten debugging `raise`s Martin found.
>
> Adding patch 0196, which disables downloading the schema for discovery.
>
> Updated patches attached. They now depend on Honza's patches 116-119
>

ACK.

Honza

-- 
Jan Cholasta