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

[Martin Basti]

I got this:

https://paste.fedoraproject.org/256746/43999380/

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/20150819/32b3d2b7/attachment.htm>