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

Wed Aug 26 09:44:10 UTC 2015

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

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0002.7-Integration-tests-topology-plugin.patch
Type: text/x-patch
Size: 16450 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150826/50c62288/attachment.bin>