[Freeipa-devel] [PATCHES 247-259] ID views - management part

[Petr Viktorin]

On 09/10/2014 03:10 PM, Petr Viktorin wrote:
> On 08/01/2014 12:30 PM, Tomas Babej wrote:
>> Hi,
>>
>> the following set of patches implements the ID view creation and
>> management of views and ID overrides in IPA.
>>
>> Pending questions:
>> 1.) The patch 253 implements basic managed permissions for ID views and
>> ID overrides. Do we want to have a separate permission for assigning ID
>> views?
>> 2.) Performance: idview-apply command takes ~100 seconds for hostgroups
>> with 1000 member hosts. I am able to lower that by 20-30% using raw ldap
>> calls, is bypassing the framework worth the performance gain? We'll lose
>> the possiblity to hook on exception callbacks.

Oh, missed this for some reason, sorry. But I replied inline.
I believe calling Commands from Commands is bad; if you need some common 
functionality it should be put in a Python function.
Do exception callbacks help here in any way?

>>
>> The commands also need more helpful documentation, I am working on that.
>>
>
> Not tested yet, but here are my comments on the patches:
>
> 0247:
> The change to copy-schema-to-ca is not necessary. This is only used for
> servers installed with Dogtag 9; for such installs new schema is added
> online (to 99-user.ldif), so it will be replicated to the CA DS correctly.
>
> Did you register the new OIDs?
>
> 0248-0249 look good
>
> 0250:
> With so many names imported from one module, I find it more readable to
> `from ipalib.plugins import baseldap`, and use qualified names later.
> Same in the 2 other patches.
> This is personal preference/tip, feel free to disagree :)
>
> 0521:
> Could you also kill the backslash in _hostname_validator?
>
> 0252:
> Typo in __doc__, should be "This functionality is primarily used"…
>
> {idview,idoverride}.takes_params: flags needs to be a set -- here you've
> specified 11 single-letter flags. (Don't we all love Python's iterable
> strings.)
>
> Only one `ipa help` topic will be created for idview and idoverride. Is
> that intended?
>
> The pattern_errmsg in idoverride's uid should be expanded to fully
> describe the pattern.
>
> 0253 looks good
>
> 0254:
> The DN refactoring is done; I don't think the asserts in pre_callbacks
> are necessary any more.
>
> 0258:
> idview_apply: typo in hostgroup doc, should be "hosts to apply the ID
> view to" and "after running the idview-apply command".
> Typos in output params, should be e.g. "this ID view"
>
> idview_apply.execute: Is there a reason for calling idview_show instead
> of get_dn_if_exists?
>
> idview_apply.execute:
> failed['hostgroup'].append((hostgroup, str(e))) -- str(e) doesn't
> include the name of the exception, which is usually important
>
> Calling a Command (here, host_mod) from another Command is, at the very
> least, quite slow, with all the validation and detailed output. It's
> also a debugging nightmare when things go wrong. And the _exc_wrapper is
> an even worse debugging nightmare.
> If you need some functionality in the host plugin that you need, put it
> in a function and call that. Otherwise use ldap directly.
>
> Do we have some precedent for the 'No host was affected.' message?
> Consumers of the JSON API shouldn't need to cpecial-case this string.
>
> By subclassing idview_unapply from idview_apply you're violating
> Liskov's substitution principle. Make a common base class for them.
>
> 0259:
> show_id_overrides: cn is a MUST attribute in ipa*Override; use
> view.single_value['cn'] instead of get(). In enumerate_hosts, is None
> okay if cn is missing?
>
>


-- 
Petr³