<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> Petr, thanks for the review, your input is, as always, much to the point. My responses are inline. Also, I'm attaching a new patchset, rebased on master, please, have a look at that. Most of the patches have at least minor changes, since I rebased another ~15 patches into this patchset, and there are additional 9 patches on top. Patch 254 was deprecated, as we decided to go with more explicit way of having two separate commands for user and group ID overrides. Also, the test suite (around 80 tests) for ID views is attached - patch 270. It depends on patch 269, sent in separate thread. Tomas <div class="moz-cite-prefix">On 09/10/2014 03:10 PM, Petr Viktorin wrote: </div> <blockquote cite="mid:54104DE3.2050309@redhat.com" type="cite">On 08/01/2014 12:30 PM, Tomas Babej wrote: <blockquote type="cite">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. </blockquote> 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. </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> Removed. <meta name="Description" content="Copy-Paste Buffer"> <meta name="Generator" content="Zim"> <blockquote cite="mid:54104DE3.2050309@redhat.com" type="cite"> Did you register the new OIDs? </blockquote> Yep. <blockquote cite="mid:54104DE3.2050309@redhat.com" type="cite"> 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 :) </blockquote> I was just trying to be consistent here, rest of the IPA plugins seem to use non-qualified names for plugins. Even tough the main reason is probably that thay use evil star imports :) Let's keep it this way for now. <blockquote cite="mid:54104DE3.2050309@redhat.com" type="cite"> 0521: Could you also kill the backslash in _hostname_validator? </blockquote> Done. <blockquote cite="mid:54104DE3.2050309@redhat.com" type="cite"> 0252: Typo in __doc__, should be "This functionality is primarily used"… </blockquote> Done. <blockquote cite="mid:54104DE3.2050309@redhat.com" type="cite">{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.) </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> No longer applies. <meta name="Description" content="Copy-Paste Buffer"> <meta name="Generator" content="Zim"> <blockquote cite="mid:54104DE3.2050309@redhat.com" type="cite"> Only one `ipa help` topic will be created for idview and idoverride. Is that intended? </blockquote> Yes, but the documentation needs to be extended, making a note so that we don't forget that. <blockquote cite="mid:54104DE3.2050309@redhat.com" type="cite"> The pattern_errmsg in idoverride's uid should be expanded to fully describe the pattern. </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> If we want to expand it, then it should be corrected in user.py plugin too (I have taken it from there). Still, it's a tradeoff between conciseness and and exactness, do we really want to specify that user login may be at most 253 letters long or the $ character may only be used as terminal one? Or the fact that - can't be used at the beggining? This is probably the way it was since forever in user plugin. <meta name="Description" content="Copy-Paste Buffer"> <meta name="Generator" content="Zim"> <blockquote cite="mid:54104DE3.2050309@redhat.com" type="cite"> 0253 looks good 0254: The DN refactoring is done; I don't think the asserts in pre_callbacks are necessary any more. </blockquote> Fixed in other places. BTW, I killed this patch. <blockquote cite="mid:54104DE3.2050309@redhat.com" type="cite"> 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" </blockquote> Fixed the second and third complaint ( IMHO this is clear from context, but I added "this" to be explicit), I'm not so sure about the first one though. Can we get a native speaker input on that? <blockquote cite="mid:54104DE3.2050309@redhat.com" type="cite"> idview_apply.execute: Is there a reason for calling idview_show instead of get_dn_if_exists? </blockquote> Fixed. <blockquote cite="mid:54104DE3.2050309@redhat.com" type="cite"> idview_apply.execute: failed['hostgroup'].append((hostgroup, str(e))) -- str(e) doesn't include the name of the exception, which is usually important </blockquote> Fixed. <blockquote cite="mid:54104DE3.2050309@redhat.com" type="cite"> 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. </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> I don't think so, this was just me being over user-friendly. I think we can remove it, and I have done so. <meta name="Description" content="Copy-Paste Buffer"> <meta name="Generator" content="Zim"> <blockquote cite="mid:54104DE3.2050309@redhat.com" type="cite"> By subclassing idview_unapply from idview_apply you're violating Liskov's substitution principle. Make a common base class for them. </blockquote> Done. <blockquote cite="mid:54104DE3.2050309@redhat.com" type="cite"> 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? </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> This was actually an error, fixed in updated version of patches. New code uses 'ipaanchoruuid' (which is a MUST) and converts it (with your objections addressed.) As for the hosts, the 'cn' attribute is required by the nsHost objectclass. <meta name="Description" content="Copy-Paste Buffer"> <meta name="Generator" content="Zim"> <meta name="Description" content="Copy-Paste Buffer"> <meta name="Generator" content="Zim"> <blockquote cite="mid:54104DE3.2050309@redhat.com" type="cite"> </blockquote> <pre class="moz-signature" cols="72">-- Tomas Babej Associate Software Engineer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org </pre> </body> </html>