<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Thanks for the review.<br>
<br>
Sending updated patchset, responses are inline.<br>
<br>
<div class="moz-cite-prefix">On 09/23/2014 10:07 AM, Petr Viktorin
wrote:<br>
</div>
<blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">On
09/16/2014 01:21 PM, Tomas Babej wrote:
<br>
<blockquote type="cite">Petr,
<br>
<br>
thanks for the review, your input is, as always, much to the
point.
<br>
<br>
My responses are inline. Also, I'm attaching a new patchset,
rebased on
<br>
master, please, have a look at that.
<br>
<br>
Most of the patches have at least minor changes, since I rebased
another
<br>
~15 patches into this patchset, and there are additional 9
patches on
<br>
top. Patch 254 was deprecated, as we decided to go with more
explicit
<br>
way of having two separate commands for user and group ID
overrides.
<br>
<br>
Also, the test suite (around 80 tests) for ID views is attached
- patch
<br>
270. It depends on patch 269, sent in separate thread.
<br>
<br>
Tomas
<br>
<br>
On 09/10/2014 03:10 PM, Petr Viktorin wrote:
<br>
<blockquote type="cite">On 08/01/2014 12:30 PM, Tomas Babej
wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
the following set of patches implements the ID view creation
and
<br>
management of views and ID overrides in IPA.
<br>
<br>
Pending questions:
<br>
1.) The patch 253 implements basic managed permissions for
ID views and
<br>
ID overrides. Do we want to have a separate permission for
assigning ID
<br>
views?
<br>
2.) Performance: idview-apply command takes ~100 seconds for
hostgroups
<br>
with 1000 member hosts. I am able to lower that by 20-30%
using raw ldap
<br>
calls, is bypassing the framework worth the performance
gain? We'll lose
<br>
the possiblity to hook on exception callbacks.
<br>
<br>
The commands also need more helpful documentation, I am
working on that.
<br>
<br>
</blockquote>
<br>
Not tested yet, but here are my comments on the patches:
<br>
<br>
</blockquote>
</blockquote>
<br>
0247 looks good
<br>
<br>
In 0248 you deleted a newline at EOF in 71-idviews.update
<br>
<br>
0249 looks good
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">
<br>
0250:
<br>
With so many names imported from one module, I find it more
readable
<br>
to `from ipalib.plugins import baseldap`, and use qualified
names
<br>
later. Same in the 2 other patches.
<br>
This is personal preference/tip, feel free to disagree :)
<br>
</blockquote>
<br>
I was just trying to be consistent here, rest of the IPA plugins
seem to
<br>
use non-qualified names for plugins. Even tough the main reason
is
<br>
probably that thay use evil star imports :) Let's keep it this
way for now.
<br>
</blockquote>
<br>
Yes, the rest of the plugins are using star imports; no
consistency to be kept here.
<br>
I think that after these patches nobody will touch it again, so
"for now" is basically forever.
<br>
<br>
0521 looks good
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">0252:
<br>
</blockquote>
</blockquote>
<br>
<blockquote type="cite">
<blockquote type="cite">The pattern_errmsg in idoverride's uid
should be expanded to fully
<br>
describe the pattern.
<br>
</blockquote>
<br>
If we want to expand it, then it should be corrected in user.py
plugin
<br>
too (I have taken it from there). Still, it's a tradeoff between
<br>
conciseness and and exactness, do we really want to specify that
user
<br>
login may be at most 253 letters long or the $ character may
only be
<br>
used as terminal one? Or the fact that - can't be used at the
beggining?
<br>
This is probably the way it was since forever in user plugin.
<br>
</blockquote>
<br>
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.
<br>
<br>
Should I file a ticket?
<br>
<br>
0253 looks good
<br>
0255-0256 looks good
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">0258:
<br>
idview_apply: typo in hostgroup doc, should be "hosts to apply
the ID
<br>
view to" and "after running the idview-apply command".
<br>
Typos in output params, should be e.g. "this ID view"
<br>
</blockquote>
<br>
Fixed the second and third complaint ( IMHO this is clear from
context,
<br>
but I added "this" to be explicit), I'm not so sure about the
first one
<br>
though. Can we get a native speaker input on that?
<br>
</blockquote>
<br>
Actually both just need an article, doesn't matter much if it's
"the" or "this".
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">
<br>
By subclassing idview_unapply from idview_apply you're
violating
<br>
Liskov's substitution principle. Make a common base class for
them.
<br>
</blockquote>
<br>
Done.
<br>
</blockquote>
<br>
In the wrong patch, it seems :)
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">
<br>
0259:
<br>
show_id_overrides: cn is a MUST attribute in ipa*Override; use
<br>
view.single_value['cn'] instead of get(). In enumerate_hosts,
is None
<br>
okay if cn is missing?
<br>
</blockquote>
<br>
This was actually an error, fixed in updated version of patches.
New
<br>
code uses 'ipaanchoruuid' (which is a MUST) and converts it
(with your
<br>
objections addressed.)
<br>
</blockquote>
<br>
If it's MUST, use simply `single_value['ipaanchoruuid']` instead
of get().
<br>
<br>
</blockquote>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<p>
Yes, this is fixed in a later patch.
</p>
<meta name="Description" content="Copy-Paste Buffer">
<meta name="Generator" content="Zim">
<br>
<br>
<blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">
<br>
0260:
<br>
The get() method on dict-like objects returns None (or a given
default) if the item is missing.
<br>
If sAMAccountName is MUST, use regular dict item access.
<br>
If sAMAccountName is MAY, then you're not handling the None you
might get.
<br>
</blockquote>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<p>
It is a must, fixed in 260.
</p>
<meta name="Description" content="Copy-Paste Buffer">
<meta name="Generator" content="Zim">
<br>
<br>
<blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">
<br>
Also look at the other uses of get() to see if you're doing the
right thing.
<br>
</blockquote>
<br>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<p>
Patch 276 makes sure this does not happen.
</p>
<meta name="Description" content="Copy-Paste Buffer">
<meta name="Generator" content="Zim">
<br>
<br>
<blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">
<br>
I've looked at help(pysss_nss_idmap.getnamebysid). Shouldn't you
use pysss_nss_idmap.NAME_KEY and TYPE_KEY?
<br>
</blockquote>
<br>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<p>
Right. Also, there forgotten debubugging comment somehow lurked
in, I got that fixed too.
</p>
<meta name="Description" content="Copy-Paste Buffer">
<meta name="Generator" content="Zim">
<br>
<blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">
<br>
<br>
0261:
<br>
Again you're using get() on MUST attributes.
<br>
</blockquote>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<p>
Fixed.
</p>
<meta name="Description" content="Copy-Paste Buffer">
<meta name="Generator" content="Zim">
<blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">
<br>
0262:
<br>
ipalib/constants.py - No newline at end of file
<br>
</blockquote>
<br>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<p>
Fixed.
</p>
<meta name="Description" content="Copy-Paste Buffer">
<meta name="Generator" content="Zim">
<br>
<blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">
<br>
Is IPA_ANCHOR_PREFIX and SID_ANCHOR_PREFIX to be used outside
idviews.py? Do we want it in constants?
<br>
<br>
<blockquote type="cite">+ if
anchor.startswith(IPA_ANCHOR_PREFIX):
<br>
+ uuid = anchor.split(IPA_ANCHOR_PREFIX)[1].strip()
<br>
</blockquote>
</blockquote>
<br>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<p>
I thought of constants.py as a global store for all plugin
constants.
</p>
<meta name="Description" content="Copy-Paste Buffer">
<meta name="Generator" content="Zim">
<br>
<blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">
<br>
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:
<br>
anchor[len(IPA_ANCHOR_PREFIX):]
<br>
Same with SID_ANCHOR_PREFIX below.
<br>
</blockquote>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<p>
This cannot happen unless someone edits the raw LDAP directly, but
still, you're correct here, better not get used to using/seeing
this.<br>
Fixed in later patch, thanks.
</p>
<meta name="Description" content="Copy-Paste Buffer">
<meta name="Generator" content="Zim">
<br>
<br>
<blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">
<br>
In get_dn, why do you resolve only the last key?
<br>
</blockquote>
<br>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<p>
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.
</p>
<meta name="Description" content="Copy-Paste Buffer">
<meta name="Generator" content="Zim">
<br>
<blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">
<br>
In set_anchoruuid_from_dn, an entry is a dict of lists, use either
single_value or put the dn[0].value in brackets.
<br>
</blockquote>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<p>
This actually needs to stay this way, since LDAPUpdate expects a
single value, not a list. I added a comment.
</p>
<meta name="Description" content="Copy-Paste Buffer">
<br>
<br>
<blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">
<br>
0263:
<br>
<br>
Note that permission_filter_objectclasses are ORed together; I
don't think you want ipaOverrideAnchor there.
<br>
<br>
</blockquote>
Right, fixed.<br>
<br>
<blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">You
forgot to regenerate ACI.txt in this patch.
<br>
<br>
</blockquote>
<br>
True, fixed.<br>
<br>
<blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">0264
looks OK
<br>
<br>
0265:
<br>
<br>
Instead of:
<br>
<br>
<blockquote type="cite">+ objectclass, name_attr = (
<br>
+ ('posixaccount', 'uid')
<br>
+ if self.override_object == 'user' else
<br>
+ ('ipausergroup', 'cn')
<br>
+ )
<br>
</blockquote>
<br>
prefer:
<br>
<br>
objectclass, name_attr = {
<br>
'user': ('posixaccount', 'uid'),
<br>
'group': ('ipausergroup', 'cn'),
<br>
}[self.override_object]
<br>
<br>
This generalizes better to more cases, and fails loudly on bad
input.
<br>
<br>
</blockquote>
Fixed.<br>
<br>
<blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">You're
misusing get() again.
<br>
<br>
</blockquote>
Fixed.<br>
<br>
<blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">0266:
<br>
<br>
To build strings, prefer formatting:
<br>
return '%s%s:%s' % (IPA_ANCHOR_PREFIX, domain, uuid)
<br>
rather than adding the separator to one of the strings and
concatenating.
<br>
<br>
</blockquote>
Fixed.<br>
<br>
<blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">Instead
of:
<br>
anchor.split(':')[-1]
<br>
prefer:
<br>
anchor.rpartition(':')[-1]
<br>
which does less work.
<br>
<br>
</blockquote>
What is an ocean but a multitude of drops :) Fixed.<br>
<br>
<blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">0267:
<br>
resolve_object_to_anchor: For the error message, use
Object[...].handle_not_found()
<br>
<br>
</blockquote>
Fixed.<br>
<br>
<blockquote cite="mid:54212A37.3050503@redhat.com" type="cite">0268
looks good
<br>
<br>
0270:
<br>
get_idoverride_dn: Use re.escape() when inserting literal strings
in regexes.
<br>
<br>
</blockquote>
Fixed, thanks.<br>
<br>
<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>