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³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
