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. _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
