[Freeipa-devel] [PATCHES] 0258-0265 Add schema updater based on IPA schema files

[Ana Krivokapic]

On 11/01/2013 03:26 PM, Petr Viktorin wrote:
> On 09/13/2013 06:44 PM, Petr Viktorin wrote:
>> On 08/01/2013 04:52 PM, Petr Viktorin wrote:
>>> Hello,
>>> With these patches, schema updates will be based on the ldif files we
>>> use for installation.
>>>
>>> https://fedorahosted.org/freeipa/ticket/3454
>>>
>>> This is a RFE, here is the design doc:
>>> http://www.freeipa.org/page/V3/Improved_schema_updater
>>>
>>
>> I found and filed a bug in python-ldap[0]: it sometimes ignores parts of
>> schema LDIFs when parsing them.
>> Patch 0275 works around the bug. Please apply on top of 0258-0265 (they
>> still apply cleanly).
>>
>>
>> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1007820
>>
>
> The recent ipaldap patches resulted in a small conflict. Attaching rebased
> patches.
>
>
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel

I have tested the patches and overall they seem to work fine. Some
questions/comments are below.


Patch 258:
You catch `ldap.LOCAL_ERROR` in the `connect()` function, which is called from
`__init__()`, so no need to catch it again in `__init__()`.

Patch 259:
ACK

Patch 260:

>       # Usually the modlist order does not matter.
>       # However, for schema updates, we want 'attributetypes' before
>       # 'objectclasses'.
>       # A simple sort will ensure this.
>       modlist.sort()

Since `modlist` is a list of tuples, it is sorted by the first elements in the
tuples, then by the seconds elements, etc. Which means the resulting list will
be sorted by the modification type first (`MOD_ADD`, `MOD_DELETE`, etc), and by
`attributetypes`/`objectclasses` second. Was this the desired effect?

Patch 261:
Man page updates look good, but several options in the man page have 3 dashes in
the long form instead of 2. I have attached a mini-patch that fixes this along
with a couple of typos in the man page. Feel free to squash it to your patch 261.

Patch 262:
Whitespace warnings.
In `60-trusts.update` there are still some `replace:attributeTypes:` lines. Can
those be removed safely?

Patch 263:

+                if not force_replace:
+                    modlist.append((ldap.MOD_DELETE, key, removes))
+                elif new_values == []: # delete an empty value
+                    modlist.append((ldap.MOD_DELETE, key, removes))

can be combined into one:

+                if not force_replace or not new_values:
+                    modlist.append((ldap.MOD_DELETE, key, removes))

Patch 264:
ACK

Patch 265:
ACK

Patch 275:
ACK

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131107/a170d095/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ipa-ldap-updater-man-page-fixes.patch
Type: text/x-patch
Size: 2938 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131107/a170d095/attachment.bin>