<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> Attaching updated patchset with resolved objections from Petr^1 and Petr^3. (three more patches attached) <div class="moz-cite-prefix">On 09/29/2014 04:30 PM, Tomas Babej wrote: </div> <blockquote cite="mid:54296CEC.40906@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> Thanks for the review. Sending updated patchset, responses are inline. <div class="moz-cite-prefix">On 09/23/2014 10:07 AM, Petr Viktorin wrote: </div> <blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">On 09/16/2014 01:21 PM, Tomas Babej wrote: <blockquote type="cite">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 On 09/10/2014 03:10 PM, Petr Viktorin wrote: <blockquote 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: </blockquote> </blockquote> 0247 looks good In 0248 you deleted a newline at EOF in 71-idviews.update 0249 looks good <blockquote type="cite"> <blockquote type="cite"> 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> Yes, the rest of the plugins are using star imports; no consistency to be kept here. I think that after these patches nobody will touch it again, so "for now" is basically forever. 0521 looks good <blockquote type="cite"> <blockquote type="cite">0252: </blockquote> </blockquote> <blockquote type="cite"> <blockquote type="cite">The pattern_errmsg in idoverride's uid should be expanded to fully describe the pattern. </blockquote> 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. </blockquote> I think the message should at be free of false positives. Using e.e. "-test-name-" and getting back "may only include letters, numbers, _, -, . and $" would be extremely frustrating. Should I file a ticket? 0253 looks good 0255-0256 looks good <blockquote type="cite"> <blockquote 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> Actually both just need an article, doesn't matter much if it's "the" or "this". <blockquote type="cite"> <blockquote 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> In the wrong patch, it seems :) <blockquote type="cite"> <blockquote 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> 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.) </blockquote> If it's MUST, use simply `single_value['ipaanchoruuid']` instead of get(). </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> Yes, this is fixed in a later patch. <meta name="Description" content="Copy-Paste Buffer"> <meta name="Generator" content="Zim"> <blockquote cite="mid:54212A37.3050503@redhat.com" type="cite"> 0260: The get() method on dict-like objects returns None (or a given default) if the item is missing. If sAMAccountName is MUST, use regular dict item access. If sAMAccountName is MAY, then you're not handling the None you might get. </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> It is a must, fixed in 260. <meta name="Description" content="Copy-Paste Buffer"> <meta name="Generator" content="Zim"> <blockquote cite="mid:54212A37.3050503@redhat.com" type="cite"> Also look at the other uses of get() to see if you're doing the right thing. </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> Patch 276 makes sure this does not happen. <meta name="Description" content="Copy-Paste Buffer"> <meta name="Generator" content="Zim"> <blockquote cite="mid:54212A37.3050503@redhat.com" type="cite"> I've looked at help(pysss_nss_idmap.getnamebysid). Shouldn't you use pysss_nss_idmap.NAME_KEY and TYPE_KEY? </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> Right. Also, there forgotten debubugging comment somehow lurked in, I got that fixed too. <meta name="Description" content="Copy-Paste Buffer"> <meta name="Generator" content="Zim"> <blockquote cite="mid:54212A37.3050503@redhat.com" type="cite"> 0261: Again you're using get() on MUST attributes. </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> Fixed. <meta name="Description" content="Copy-Paste Buffer"> <meta name="Generator" content="Zim"> <blockquote cite="mid:54212A37.3050503@redhat.com" type="cite"> 0262: ipalib/constants.py - No newline at end of file </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> Fixed. <meta name="Description" content="Copy-Paste Buffer"> <meta name="Generator" content="Zim"> <blockquote cite="mid:54212A37.3050503@redhat.com" type="cite"> Is IPA_ANCHOR_PREFIX and SID_ANCHOR_PREFIX to be used outside idviews.py? Do we want it in constants? <blockquote type="cite">+�� if anchor.startswith(IPA_ANCHOR_PREFIX): +�� uuid = anchor.split(IPA_ANCHOR_PREFIX)[1].strip() </blockquote> </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> I thought of constants.py as a global store for all plugin constants. <meta name="Description" content="Copy-Paste Buffer"> <meta name="Generator" content="Zim"> <blockquote cite="mid:54212A37.3050503@redhat.com" type="cite"> This is a dangerous idiom to use for removing a prefix, since it will silently give wrong results if the string contains the prefix. Prefer: �� anchor[len(IPA_ANCHOR_PREFIX):] Same with SID_ANCHOR_PREFIX below. </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> This cannot happen unless someone edits the raw LDAP directly, but still, you're correct here, better not get used to using/seeing this. Fixed in later patch, thanks. <meta name="Description" content="Copy-Paste Buffer"> <meta name="Generator" content="Zim"> <blockquote cite="mid:54212A37.3050503@redhat.com" type="cite"> In get_dn, why do you resolve only the last key? </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> You can only pass two arguments to these commands, where the first one is the name of the view, which you don't want to resolve. <meta name="Description" content="Copy-Paste Buffer"> <meta name="Generator" content="Zim"> <blockquote cite="mid:54212A37.3050503@redhat.com" type="cite"> In set_anchoruuid_from_dn, an entry is a dict of lists, use either single_value or put the dn[0].value in brackets. </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> This actually needs to stay this way, since LDAPUpdate expects a single value, not a list. I added a comment. <meta name="Description" content="Copy-Paste Buffer"> <blockquote cite="mid:54212A37.3050503@redhat.com" type="cite"> 0263: Note that permission_filter_objectclasses are ORed together; I don't think you want ipaOverrideAnchor there. </blockquote> Right, fixed. <blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">You forgot to regenerate ACI.txt in this patch. </blockquote> True, fixed. <blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">0264 looks OK 0265: Instead of: <blockquote type="cite">+�� objectclass, name_attr = ( +�� ('posixaccount', 'uid') +�� if self.override_object == 'user' else +�� ('ipausergroup', 'cn') +�� ) </blockquote> prefer: �� objectclass, name_attr = { �� 'user': ('posixaccount', 'uid'), �� 'group': ('ipausergroup', 'cn'), �� }[self.override_object] This generalizes better to more cases, and fails loudly on bad input. </blockquote> Fixed. <blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">You're misusing get() again. </blockquote> Fixed. <blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">0266: To build strings, prefer formatting: �� return '%s%s:%s' % (IPA_ANCHOR_PREFIX, domain, uuid) rather than adding the separator to one of the strings and concatenating. </blockquote> Fixed. <blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">Instead of: �� anchor.split(':')[-1] prefer: �� anchor.rpartition(':')[-1] which does less work. </blockquote> What is an ocean but a multitude of drops :) Fixed. <blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">0267: resolve_object_to_anchor: For the error message, use Object[...].handle_not_found() </blockquote> Fixed. <blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">0268 looks good 0270: get_idoverride_dn: Use re.escape() when inserting literal strings in regexes. </blockquote> Fixed, thanks. <pre class="moz-signature" cols="72">-- Tomas Babej Associate Software Engineer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org </pre> </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>