<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 02/25/2016 12:03 PM, Martin Babinsky
wrote:<br>
</div>
<blockquote cite="mid:56CEDF98.7060809@redhat.com" type="cite">On
02/24/2016 04:30 PM, thierry bordaz wrote:
<br>
<blockquote type="cite">On 01/21/2016 05:04 PM, Martin Babinsky
wrote:
<br>
<blockquote type="cite">On 01/21/2016 01:37 PM, thierry bordaz
wrote:
<br>
<blockquote type="cite">
<br>
<br>
<br>
</blockquote>
Hi Thierry,
<br>
<br>
I have couple of comments to your patch:
<br>
<br>
1.)
<br>
there is a number of PEP8 errors in the patch
<br>
(<a class="moz-txt-link-freetext" href="http://paste.fedoraproject.org/313246/33893701">http://paste.fedoraproject.org/313246/33893701</a>), please fix
them.
<br>
<br>
See <a class="moz-txt-link-freetext" href="http://www.freeipa.org/page/Python_Coding_Style">http://www.freeipa.org/page/Python_Coding_Style</a> for our
<br>
conventions used in Python code.
<br>
<br>
2.)
<br>
+ DNA_BIND_METHOD = "dnaRemoteBindMethod"
<br>
+ DNA_CONN_PROTOCOL = "dnaRemoteConnProtocol"
<br>
+ DNA_PLUGIN_DN = 'cn=Distributed Numeric
Assignment
<br>
Plugin,cn=plugins,cn=config'
<br>
+ dna_config_base = 'cn=Posix IDs,%s' % DNA_PLUGIN_DN
<br>
<br>
Uppercase names are usually reserved for module-level
constants. OTOH,
<br>
local variables should be lowercase. Also you can instantiate
<br>
dna_config_base directly as DN, using 2-member tuples, i. e:
<br>
<br>
"""
<br>
dna_config_base = DN(('cn', 'posix IDs'), ('cn', 'Distributed
Numeric
<br>
Assignment Plugin'), ('cn', 'plugins'), ('cn', 'config'))
<br>
"""
<br>
<br>
When passing DN object to the formatting functions/operators,
it is
<br>
automatically converted to string so no need to hold string
and DN
<br>
object separately. This is done in other places (see
function/methods
<br>
in replication.py).
<br>
<br>
3.)
<br>
<br>
+ for i in range(len(entries)) :
<br>
+
<br>
+ mod = []
<br>
+ if entries[i].single_value.get(DNA_BIND_METHOD)
!= method:
<br>
+ mod.append((ldap.MOD_REPLACE,
DNA_BIND_METHOD, method))
<br>
+
<br>
+ if entries[i].single_value.get(DNA_CONN_PROTOCOL)
!=
<br>
protocol:
<br>
+ mod.append((ldap.MOD_REPLACE,
DNA_CONN_PROTOCOL,
<br>
protocol))
<br>
<br>
<br>
please use idiomatic Python when handling list of entries,
e.g.:
<br>
<br>
"""
<br>
for entry in entries:
<br>
mod = []
<br>
if entry.single_value.get(DNA_BIND_METHOD) != method:
<br>
...
<br>
"""
<br>
<br>
4.) I think that this method should in DSInstance class since
it deals
<br>
with directory server configuration. Service is a parent
object of all
<br>
other service installers/configurators and should contain only
methods
<br>
common to more children.
<br>
<br>
5.) Since the method is called from every installer, it could
be
<br>
beneficial to call it in DSInstance.__common_post_setup() as a
part of
<br>
Directory server installation. Is there any reason why this is
not the
<br>
case?
<br>
<br>
6.)
<br>
<br>
+ while attempt != MAX_WAIT:
<br>
+ try:
<br>
+ entries = conn.get_entries(sharedcfgdn,
<br>
scope=ldap.SCOPE_ONELEVEL, filter='dnaHostname=%s' %
self.fqdn)
<br>
+ break
<br>
+ except errors.NotFound:
<br>
+ root_logger.debug("So far enable not find DNA
shared
<br>
config entry for dnaHostname=%s under %s. Retry in 2sec" %
(self.fqdn,
<br>
sharedcfgdn))
<br>
+ attempt = attempt + 1
<br>
+ time.sleep(2)
<br>
+ continue
<br>
+
<br>
+ # safety checking
<br>
+ # there is no return, if there are several entries,
as a
<br>
workaround of #5510
<br>
+ if len(entries) != 1:
<br>
<br>
I am quite afraid what would happen if the server does not
return any
<br>
entries until 30 s timeout. The code will then continue to the
<br>
condition which can potentially test an uninitialized variable
and
<br>
blow up with 'NameError'. This should be handled more
robustly, e. g.
<br>
raise an exception when a timeout is reached and no entries
were
<br>
returned.
<br>
<br>
7.)
<br>
<br>
+ if len(mod) > 0:
<br>
<br>
A Pythonic way to test for non-empty container is
<br>
<br>
"""
<br>
if mods:
<br>
# do stuff
<br>
"""
<br>
<br>
since an empty list/dict/set evaluates to False and non-empty
<br>
containers are True.
<br>
<br>
<br>
8.)
<br>
<br>
+ entry = conn.get_entry(entries[i].dn)
<br>
+ if entry.single_value.get(DNA_BIND_METHOD) !=
method:
<br>
+ root_logger.error("Fail to set
SASL/GSSAPI bind
<br>
method to %s" % (entries[i].dn))
<br>
+ if entry.single_value.get(DNA_CONN_PROTOCOL)
!=
<br>
protocol:
<br>
+ root_logger.error("Fail to set LDAP
protocol to
<br>
%s" % (entries[i].dn))
<br>
<br>
rather than re-fetching the modified entry and testing what
happened,
<br>
you can just catch an exception raised by unsuccessfull mod
and log an
<br>
error like this:
<br>
<br>
"""
<br>
try:
<br>
conn.modify_s(entry.dn, mod)
<br>
except Exception as e:
<br>
root_logger.error("Failed to modify entry {}:
{}".format(entry, e))
<br>
"""
<br>
<br>
as a matter of fact, if the modify_s operation would fail for
some
<br>
reason, an ldap exception would be raised and you checks would
not
<br>
even be executed.
<br>
<br>
9.)
<br>
The debug message on line 219 should read "Unable to find DNA
shared
<br>
config entry for dnaHostname=%s so far. Retry in 2 sec.". The
errors
<br>
at the end of the method should have "Failed" instead of
"Fail".
<br>
<br>
</blockquote>
Hi Martin,
<br>
<br>
Finally tested... here is the updated patch. Thanks for you
patience
<br>
<br>
<br>
thanks
<br>
thierry
<br>
</blockquote>
<br>
Hi Thierry,
<br>
<br>
the patch works as expected. I have some more nitpicks though:
<br>
<br>
1.) Please fix the following pep8 errors:
<br>
<br>
<a class="moz-txt-link-freetext" href="http://paste.fedoraproject.org/329086/56397841/">http://paste.fedoraproject.org/329086/56397841/</a>
<br>
<br>
you can check whether you recent commit conforms to PEP8 by
running
<br>
<br>
"git show -U0 | pep8 --diff"
<br>
<br>
2.)
<br>
+ self.step("update DNA shared config entry",
self.update_dna_shared_config)
<br>
<br>
I would rather change the message to "Updating DNA shared config
entry" since all other messages use continuous tense.
<br>
<br>
3.)
<br>
+ else:
<br>
+ raise RuntimeError("Could not get dnaHostname
entries in {} seconds".format(max_wait * 2))
<br>
<br>
Please use root_logger.error() and then return as is used
elsewhere in the method. We do not want a runaway exception
crashing upgrade.
<br>
<br>
</blockquote>
<font face="Times New Roman, Times, serif">Hi Martin,<br>
<br>
Updated/tested the patch with your help/recommendations. pep8 is
clear now :-)<br>
<br>
thanks<br>
thierry<br>
</font>
</body>
</html>