<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<br>
<div class="moz-cite-prefix">On 08/19/2015 04:17 PM, Martin Basti
wrote:<br>
</div>
<blockquote cite="mid:55D4900C.5040509@redhat.com" type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
I got this:<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://paste.fedoraproject.org/256746/43999380/">https://paste.fedoraproject.org/256746/43999380/</a><br>
</blockquote>
FYI replica install failure. (I will retest it, but I'm pretty sure
that it was clean VM, test for some reason install client first)<br>
<br>
File
"/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py",
line 295, in decorated<br>
func(installer)<br>
File
"/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py",
line 319, in install_check<br>
sys.exit("IPA client is already configured on this system.\n"<br>
<br>
2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed,
exception: SystemExit: IPA client is already configured on this
system.<br>
Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.<br>
2015-08-19T14:14:15Z ERROR IPA client is already configured on this
system.<br>
Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.<br>
<br>
<blockquote cite="mid:55D4900C.5040509@redhat.com" type="cite"> <br>
<div class="moz-cite-prefix">On 08/19/2015 09:00 AM, Oleg Fayans
wrote:<br>
</div>
<blockquote cite="mid:55D429A1.3000605@redhat.com" type="cite">Hi
Martin, <br>
<br>
As discussed, here is a new version with pep8-related fixes <br>
<br>
<br>
On 08/14/2015 10:44 AM, Oleg Fayans wrote: <br>
<blockquote type="cite">Hi Martin, <br>
<br>
Already noticed that. Implemented the named groups as Tomas
advised. <br>
Added the third test for <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica">http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica</a>
<br>
<br>
<br>
<br>
<br>
On 08/13/2015 05:06 PM, Martin Basti wrote: <br>
<blockquote type="cite"> <br>
<br>
On 08/11/2015 03:36 PM, Oleg Fayans wrote: <br>
<blockquote type="cite">Hi Martin, <br>
<br>
On 08/11/2015 02:02 PM, Martin Basti wrote: <br>
<blockquote type="cite">NACK, comments inline. <br>
<br>
On 11/08/15 13:25, Oleg Fayans wrote: <br>
<blockquote type="cite">Hi Martin, <br>
<br>
Thanks for the review! <br>
<br>
On 08/10/2015 07:08 PM, Martin Basti wrote: <br>
<blockquote type="cite">Thank you for patch, I have a
few nitpicks: <br>
<br>
1) <br>
On 10/08/15 13:05, Oleg Fayans wrote: <br>
<blockquote type="cite">+def create_segment(master,
leftnode, rightnode): <br>
+ """create_segment(master, leftnode,
rightnode) <br>
</blockquote>
Why do you add the name of method in docstring? <br>
</blockquote>
My bad, fixed. <br>
</blockquote>
<br>
still there <br>
<br>
+ tokenize_topologies(command_output) <br>
+ takes an output of `ipa topologysegment-find`
and returns an <br>
array of <br>
<br>
</blockquote>
Fixed, sorry. <br>
<blockquote type="cite"> <br>
<blockquote type="cite">
<blockquote type="cite"> <br>
<br>
2) <br>
<br>
+def create_segment(master, leftnode, rightnode): <br>
+ """create_segment(master, leftnode, rightnode)
<br>
+ creates a topology segment. The first argument
is a node to <br>
run the <br>
command on <br>
+ The first 3 arguments should be objects of
class Host <br>
+ Returns a hash object containing segment's
name, leftnode, <br>
rightnode information <br>
+ """ <br>
<br>
I would prefer to add assert there instead of just
document that a <br>
Host <br>
object is needed <br>
assert(isinstance(master, Host)) <br>
... <br>
</blockquote>
<br>
Fixed. Created a decorator that checks the type of
arguments <br>
</blockquote>
<br>
This does not scale well. <br>
If we will want to add new argument that is not host
object, you need <br>
change it again. <br>
</blockquote>
<br>
Agreed. Modified the decorator so that you can specify a
slice of <br>
arguments to be checked and a class to compare to. This
does scale :) <br>
<blockquote type="cite"> <br>
This might be used as function with specified variables
that have to be <br>
host objects <br>
<blockquote type="cite"> <br>
<blockquote type="cite"> <br>
3) <br>
+def destroy_segment(master, segment_name): <br>
+ """ <br>
+ destroy_segment(master, segment_name) <br>
+ Destroys topology segment. First argument
should be object of <br>
class <br>
Host <br>
<br>
Instead of description of params as first, second
etc., you may use <br>
following: <br>
<br>
+def destroy_segment(master, segment_name): <br>
+ """ <br>
+ Destroys topology segment. <br>
+ :param master: reference to master object of
class Host <br>
+ :param segment: name fo segment <br>
and eventually this in other methods <br>
+ :returns: Lorem ipsum sit dolor mit amet <br>
+ :raises NotFound: if segment is not found <br>
<br>
</blockquote>
Fixed <br>
<blockquote type="cite"> <br>
4) <br>
<br>
cls.replicas[:len(cls.replicas) - 1], <br>
<br>
I suggest cls.replicas[:-1] <br>
<br>
In [2]: a = [1, 2, 3, 4, 5] <br>
<br>
In [3]: a[:-1] <br>
Out[3]: [1, 2, 3, 4] <br>
<br>
</blockquote>
Fixed <br>
<blockquote type="cite"> <br>
5) <br>
Why re.findall() and then you just use the first
result? <br>
'leftnode': self.leftnode_re.findall(i)[0] <br>
<br>
Isn't just re.search() enough? <br>
leftnode_re.search(value).group(1) <br>
</blockquote>
<br>
in fact <br>
leftnode_re.findall(string)[0] <br>
and <br>
leftnode_re.search(string).group(1), <br>
Are equally bad from the readability point of view.
The first one is <br>
even shorter a bit, so why change? :) <br>
</blockquote>
<br>
It depends on point of view, because when I reviewed it
yesterday my <br>
brain raises exception that you are trying to add
multiple values to <br>
single value attribute, because you used findall, I
expected that you <br>
need multiple values, and then I saw that index [0] at
the end, and I <br>
was pretty confused what are you trying to achieve. <br>
<br>
And because findall is not effective in case when you
need to find just <br>
one occurrence. <br>
</blockquote>
<br>
I got it. Fixed. <br>
<br>
<blockquote type="cite"> <br>
<br>
<blockquote type="cite"> <br>
<blockquote type="cite"> <br>
<br>
Python 3 nitpick: <br>
I'm not sure if time when we should enforce python
2/3 compability <br>
already comes, but just for record: <br>
instead of open(file, 'r'), please use io.open(file,
'r') (import io <br>
before) <br>
<br>
</blockquote>
Done. <br>
<blockquote type="cite"> <br>
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
1) <br>
<br>
+# <br>
<br>
empty comment here (several times) <br>
</blockquote>
<br>
Removed <br>
<blockquote type="cite"> <br>
</blockquote>
<br>
</blockquote>
<br>
NACK <br>
<br>
you changed it wrong <br>
<br>
group() returns everything, you need use group(1) to return
content in <br>
braces. <br>
</blockquote>
<br>
<br>
<br>
</blockquote>
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
<br>
</body>
</html>