[Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

[Martin Basti]

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'.

>
> 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.
>>>
>>>
>>>
>>
>>
>>
>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150820/a5922e72/attachment.htm>