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

[Tomas Babej]

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