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

Mon Nov 11 11:32:34 UTC 2013

On 11/07/2013 02:34 PM, Ana Krivokapic wrote:
> 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.

>
> 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__()`.

I've added a comment instead of the try/catch

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

I've added a sort key; it should be safer now.

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

Nice catch! Squashed.

> Patch 262:
> Whitespace warnings.

I didn't see any with my settings; could you be more specific?

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

Yes! I've checked they match the ldif, and removed them.

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

Done

> Patch 264:
> ACK
>
> Patch 265:
> ACK
>
> Patch 275:
> ACK

Thanks for the review!
Updated patches attached.

-- 
Petr³

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0258.3-ldapupdate-Factor-out-connection-code.patch
Type: text/x-patch
Size: 4880 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131111/5404204e/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0259.3-dsinstance-Move-the-list-of-schema-filenames-to-a-co.patch
Type: text/x-patch
Size: 2098 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131111/5404204e/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0260.3-Add-schema-updater-based-on-IPA-schema-files.patch
Type: text/x-patch
Size: 13148 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131111/5404204e/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0261.3-Update-the-man-page-for-ipa-ldap-updater.patch
Type: text/x-patch
Size: 3638 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131111/5404204e/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0262.3-Remove-schema-modifications-from-update-files.patch
Type: text/x-patch
Size: 60196 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131111/5404204e/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0263.3-Remove-schema-special-casing-from-the-LDAP-updater.patch
Type: text/x-patch
Size: 10094 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131111/5404204e/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0264.3-Make-schema-files-conform-to-new-updater.patch
Type: text/x-patch
Size: 27262 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131111/5404204e/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0265.3-Add-formerly-update-only-schema.patch
Type: text/x-patch
Size: 10413 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131111/5404204e/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0275.3-Unify-capitalization-of-attribute-names-in-schema-fi.patch
Type: text/x-patch
Size: 11040 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131111/5404204e/attachment-0008.bin>