[Freeipa-devel] [PATCH] 0105 FIX: LDAP_updater

[Petr Viktorin]

On 09/01/2014 04:31 PM, Martin Basti wrote:
> On 24/07/14 09:06, Martin Basti wrote:
>> On 23/07/14 15:17, Martin Basti wrote:
>>> This patch fixes ordering problem of schema updates
>>>
>>> Martin should it be in IPA 4.0.x ? It requires rebased ldap_python
>>> (will be in Fedora 21)
>>>
>>> Patch attached
>>>
>>>
>> I found a bug there, but before I send updated version, I need to
>> decide how print modlist:
>>
>> 1. Print modlist before each LDAP update (if changes exist), show
>> modlist per incremental update
>>
>> 2. Print modlist only at once with all updates
>>
>> 3. Use [1] for live_run, [2] for test
>>
> Print modlis before each LDAP update
> Updated patch attached.

Thanks! This works nicely, just some comments:

The required python-ldap is quite new, not even built in Koji yet. We 
need to get it to the IPA COPR repo before this is merged.


The _get_oid_dependency_order function is not indented well.

It would be nice if it was a bit more obvious that the loop in 
_get_oid_dependency_order is guaranteed to terminate. Could you assert 
that len(unordered_oids) decreases in each iteration?

> +SCHEMA_ELEMENT_CLASSES_KEYS = (x[0] for x in SCHEMA_ELEMENT_CLASSES)

You're creating a generator here, which is only good for one use. Use a 
list instead.
Also, this is used just once so it doesn't need to be a module-level 
constant.

> +                    # FIXME: We should have a better way to display the modlist,
> +                    # for now display raw output of our internal routine

Not entirely related to your change, but you can drop this comment. 
Since 61887ac we don't use an internal routine.


-- 
Petr³