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