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

[Ana Krivokapic]

On 11/11/2013 12:32 PM, Petr Viktorin wrote:
> 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.

Hmm, the key you added still retains the default sorting behavior - it will sort
by the first elements of the tuples first. Since we need sorting by the second
elements, I think the key here should be: key=lambda m: m[1].lower()

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

$ git am
~/freeipa-pviktori-0262.3-Remove-schema-modifications-from-update-files.patch
Applying: Remove schema modifications from update files
/home/ana/freeipa/.git/rebase-apply/patch:497: new blank line at EOF.
+
/home/ana/freeipa/.git/rebase-apply/patch:693: new blank line at EOF.
+
warning: 2 lines add whitespace errors.


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


-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.