<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 23/07/15 16:27, Martin Basti wrote:<br>
</div>
<blockquote cite="mid:55B0F9C7.1050006@redhat.com" type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 23/07/15 11:42, Oleg Fayans wrote:<br>
</div>
<blockquote cite="mid:55B0B723.7070801@redhat.com" type="cite">Forgot
to attach the new version, sorry! <br>
<br>
On 07/23/2015 10:32 AM, Oleg Fayans wrote: <br>
<blockquote type="cite">Hi Martin, <br>
<br>
On 07/22/2015 05:48 PM, Martin Basti wrote: <br>
<blockquote type="cite">On 22/07/15 15:19, Oleg Fayans wrote:
<br>
<blockquote type="cite">Hi Martin, <br>
<br>
Fixed. <br>
<br>
On 07/22/2015 09:26 AM, Martin Basti wrote: <br>
<blockquote type="cite">On 22/07/15 09:23, Oleg Fayans
wrote: <br>
<blockquote type="cite">Hi Martin, <br>
<br>
Patch updated. Thank you for the review! <br>
<br>
On 07/21/2015 05:45 PM, Martin Basti wrote: <br>
<blockquote type="cite">On 20/07/15 14:07, Oleg Fayans
wrote: <br>
<blockquote type="cite">Hi Martin, <br>
<br>
Updated. <br>
<br>
<br>
On 07/20/2015 12:46 PM, Martin Basti wrote: <br>
<blockquote type="cite">On 20/07/15 11:57, Oleg
Fayans wrote: <br>
<blockquote type="cite">+ pwfile =
api.env.dot_ipa + os.sep + ".dmpw" <br>
+ if ipautil.file_exists(pwfile): <br>
+ fp = open(pwfile, "r") <br>
+ dm_password = fp.read().rstrip()
<br>
+ fp.close() <br>
+ else: <br>
</blockquote>
Hello, <br>
<br>
1) Can you use os.path.join() instead of "+
os.sep +" please <br>
<br>
2) Can you use with statement with file? <br>
<br>
with open(pwfile, "r") as f: <br>
dm_password = f.read().rstrip() <br>
<br>
3) Please keep PEP8 in new code <br>
<br>
./ipatests/test_ipaserver/test_topology_plugin.py:30:80:
E501 line too long (102 > 79 characters) <br>
./ipatests/test_ipaserver/test_topology_plugin.py:33:80:
E501 line too long (92 > 79 characters) <br>
./ipatests/test_ipaserver/test_topology_plugin.py:39:80:
E501 line too long (124 > 79 characters) <br>
./ipatests/test_ipaserver/test_topology_plugin.py:44:80:
E501 line too long (92 > 79 characters) <br>
./ipatests/test_ipaserver/test_topology_plugin.py:45:48:
E128 continuation line under-indented for visual
indent <br>
./ipatests/test_ipaserver/test_topology_plugin.py:45:80:
E501 line too long (89 > 79 characters) <br>
./ipatests/test_ipaserver/test_topology_plugin.py:46:48:
E128 continuation line under-indented for visual
indent <br>
./ipatests/test_ipaserver/test_topology_plugin.py:46:80:
E501 line too long (89 > 79 characters) <br>
./ipatests/test_ipaserver/test_topology_plugin.py:58:80:
E501 line too long (87 > 79 characters) <br>
<br>
4) Missing nose import <br>
raise nose.SkipTest("No directory
manager password in %s" % pwfile) <br>
<br>
5) Can you use sets here instead of sorted
lists? <br>
assert(sorted(entry.keys()) ==
sorted(pluginattrs.keys())) <br>
<br>
<br>
Martin^2 <br>
<br>
</blockquote>
<br>
</blockquote>
1) <br>
Sorry, I didn't notice before, but there is missing
header in that file. <br>
<br>
2) <br>
You don't need to specify ldap_uri, you just need to
call ldap2(api), by default api.env.ldap_uri is
used, which is the same as you specified <br>
<br>
3) <br>
Can you indent values of dict which are on newline?
It is readable better. <br>
u'nsslapd-topo-plugin-shared-config-base': <br>
[u'cn=ipa,cn=etc,dc=example,dc=com'], <br>
u'nsslapd-pluginDescription':
[u'ipa-topology-plugin'], <br>
<br>
4) <br>
Please use lower F as variable, in python we use
capital letters for class definitions <br>
with open(pwfile, "r") as F: <br>
dm_password = F.read().rstrip() <br>
<br>
Otherwise it works as expected. <br>
<br>
Martin^2 <br>
<br>
<br>
<br>
<br>
</blockquote>
<br>
</blockquote>
Sorry. <br>
You added there old license format, we now use in new
files new format <br>
<br>
# <br>
# Copyright (C) 2015 FreeIPA Contributors see COPYING
for license <br>
# <br>
<br>
</blockquote>
<br>
</blockquote>
I cannot apply the last patch <br>
<br>
$ git am
freeipa-ofayans-0001.3-test-topologyplugin-is-listed-among-DS-plugins.patch
-3 <br>
Applying: Added test - topology plugin is listed among DS
plugins <br>
fatal: corrupt patch at line 83 <br>
Repository lacks necessary blobs to fall back on 3-way
merge. <br>
Cannot fall back to three-way merge. <br>
<br>
</blockquote>
Fixed. Tested it locally, it applies <br>
<br>
</blockquote>
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
Thank you ACK<br>
<br>
<pre class="moz-signature" cols="72">--
Martin Basti</pre>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
Pushed to master: e5acd01ed2971be779e788937493844a9926bb96<br>
<br>
<pre class="moz-signature" cols="72">--
Martin Basti</pre>
</body>
</html>