On 08/26/2015 11:44 AM, Oleg Fayans wrote: > Hi Martin, > > On 08/20/2015 11:18 AM, Martin Basti wrote: >> >> >> On 08/20/2015 10:26 AM, Martin Basti wrote: >>> >>> >>> On 08/19/2015 04:17 PM, Martin Basti wrote: >>>> I got this: >>>> >>>> https://paste.fedoraproject.org/256746/43999380/ >>> 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'. >> Confirm I got same error. > Fixed. It was a regex error. > >>> >>>> >>>> On 08/19/2015 09:00 AM, Oleg Fayans wrote: >>>>> Hi Martin, >>>>> >>>>> As discussed, here is a new version with pep8-related fixes >>>>> >>>>> >>>>> On 08/14/2015 10:44 AM, Oleg Fayans wrote: >>>>>> Hi Martin, >>>>>> >>>>>> Already noticed that. Implemented the named groups as Tomas advised. >>>>>> Added the third test for >>>>>> 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 >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> On 08/13/2015 05:06 PM, Martin Basti wrote: >>>>>>> >>>>>>> >>>>>>> On 08/11/2015 03:36 PM, Oleg Fayans wrote: >>>>>>>> Hi Martin, >>>>>>>> >>>>>>>> On 08/11/2015 02:02 PM, Martin Basti wrote: >>>>>>>>> NACK, comments inline. >>>>>>>>> >>>>>>>>> On 11/08/15 13:25, Oleg Fayans wrote: >>>>>>>>>> Hi Martin, >>>>>>>>>> >>>>>>>>>> Thanks for the review! >>>>>>>>>> >>>>>>>>>> On 08/10/2015 07:08 PM, Martin Basti wrote: >>>>>>>>>>> Thank you for patch, I have a few nitpicks: >>>>>>>>>>> >>>>>>>>>>> 1) >>>>>>>>>>> On 10/08/15 13:05, Oleg Fayans wrote: >>>>>>>>>>>> +def create_segment(master, leftnode, rightnode): >>>>>>>>>>>> + """create_segment(master, leftnode, rightnode) >>>>>>>>>>> Why do you add the name of method in docstring? >>>>>>>>>> My bad, fixed. >>>>>>>>> >>>>>>>>> still there >>>>>>>>> >>>>>>>>> + tokenize_topologies(command_output) >>>>>>>>> + takes an output of `ipa topologysegment-find` and >>>>>>>>> returns an >>>>>>>>> array of >>>>>>>>> >>>>>>>> Fixed, sorry. >>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> 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)) >>>>>>>>>>> ... >>>>>>>>>> >>>>>>>>>> Fixed. Created a decorator that checks the type of arguments >>>>>>>>> >>>>>>>>> This does not scale well. >>>>>>>>> If we will want to add new argument that is not host object, you >>>>>>>>> need >>>>>>>>> change it again. >>>>>>>> >>>>>>>> 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 :) >>>>>>>>> >>>>>>>>> This might be used as function with specified variables that >>>>>>>>> have to be >>>>>>>>> host objects >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> 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 >>>>>>>>>>> >>>>>>>>>> Fixed >>>>>>>>>>> >>>>>>>>>>> 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] >>>>>>>>>>> >>>>>>>>>> Fixed >>>>>>>>>>> >>>>>>>>>>> 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) >>>>>>>>>> >>>>>>>>>> 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? :) >>>>>>>>> >>>>>>>>> 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. >>>>>>>> >>>>>>>> I got it. Fixed. >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> 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) >>>>>>>>>>> >>>>>>>>>> Done. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> 1) >>>>>>>>> >>>>>>>>> +# >>>>>>>>> >>>>>>>>> empty comment here (several times) >>>>>>>> >>>>>>>> Removed >>>>>>>>> >>>>>>>> >>>>>>> >>>>>>> NACK >>>>>>> >>>>>>> you changed it wrong >>>>>>> >>>>>>> group() returns everything, you need use group(1) to return >>>>>>> content in >>>>>>> braces. >>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>>> >>>> >>>> >>>> >>> >>> >>> >> > > >
Please, do not use third-party URL shorteners from within our source code. Tomas -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code