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

[Martin Basti]

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.
>
>>
>> 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/d103c2c6/attachment.htm>