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

Wed Aug 26 12:53:14 UTC 2015

Hi,

No more short links :)

On 08/26/2015 11:50 AM, Tomas Babej wrote:
>
>
> 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
>

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