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

[Petr Viktorin]

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