[Freeipa-devel] [Test][Patch-0049, 0050] Certs in ID overrides test

[Oleg Fayans]

Ping for review.

On 09/06/2016 01:57 PM, Oleg Fayans wrote:
> The test is updated to clean up after itself
>
> On 09/06/2016 12:57 PM, Oleg Fayans wrote:
>> Hi Martin,
>>
>> Thanks for the review. The updated patches are attached. Please, see my
>> comments below
>>
>> On 08/30/2016 01:58 PM, Martin Basti wrote:
>>>
>>>
>>> On 22.08.2016 13:18, Oleg Fayans wrote:
>>>> ping for review
>>>>
>>>> On 08/02/2016 01:11 PM, Oleg Fayans wrote:
>>>>> Hi Martin,
>>>>>
>>>>> I did! Thank you!
>>>>>
>>>>> On 08/02/2016 12:31 PM, Martin Basti wrote:
>>>>>>
>>>>>>
>>>>>> On 01.08.2016 22:46, Oleg Fayans wrote:
>>>>>>> The test was redesigned so that it actually tests against an AD
>>>>>>> user.
>>>>>>> cleanly applies, passes lint and passes
>>>>>>>
>>>>>>> https://paste.fedoraproject.org/399504/00843641/
>>>>>>
>>>>>> Okay
>>>>>>
>>>>>> Did you forget to send patches?
>>>>>>
>>>>>> Martin^2
>>>>>>>
>>>>>>>
>>>>>>> On 06/28/2016 01:40 PM, Oleg Fayans wrote:
>>>>>>>> Patch-0050 rebased against latest upstream branch
>>>>>>>>
>>>>>>>> On 06/28/2016 10:45 AM, Oleg Fayans wrote:
>>>>>>>>> Passing test output:
>>>>>>>>>
>>>>>>>>> https://paste.fedoraproject.org/385774/71035231/
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>> NACK for 0049.1
>>>
>>> 1)
>>> PEP8: you must use 2 empty lines between functions
>>
>> Fixed
>>
>>>
>>> 2)
>>> +    new_args = " ".join(new_args + args)
>>>
>>> you don't need this, run_command takes list as argument too
>>> new_args.extend(args)
>>
>> The list-based approach does not work with shell redirects which are
>> heavily used in the certs_id_idoverrides test. Thus, this trick is
>> really needed
>>
>>>
>>> 3)
>>> To make it more usable you should add raiseonerr as kwarg to
>>> run_certutil (True as default)
>>
>> Done
>>
>>>
>>> NACK for 0050.2
>>>
>>> 1)
>>> +        tasks.run_certutil(master, ['-L', '-n', cls.adcert1, '-a', '>',
>>> +                                    cls.adcert1_file], cls.reqdir)
>>> +        tasks.run_certutil(master, ['-L', '-n', cls.adcert2, '-a', '>',
>>> +                                    cls.adcert2_file], cls.reqdir)
>>>
>>> IMO thus should raise an error if failed, but previously you set
>>> raiseonerr=False (multiple times)
>>
>> Agreed. Done
>>
>>>
>>> 2)
>>> +        cls.ad = cls.ad_domains[0].ads[0]
>>> +        cls.ad_domain = cls.ad.domain.name
>>> +        cls.aduser = "testuser@%s" % cls.ad_domain
>>> +        cls.adcert1 = 'MyCert1'
>>> +        cls.adcert2 = 'MyCert2'
>>> +        cls.adcert1_file = cls.adcert1 + '.crt'
>>> +        cls.adcert2_file = cls.adcert2 + '.crt'
>>>
>>> New definitions of variables/constants should be directly in class not
>>> in install method, adding new class variables in classmethod is the same
>>> evil as adding instance variables outside __init__
>>
>> Fair point. Fixed
>>
>>>
>>> 3)
>>> I have question, why do you need AD for this test? AFAIK you can use ID
>>> overrides without AD
>>
>> Correct. You can, but the workflow would be slightly different. For
>> example, you can not issue and sign cert requests for AD-users the way
>> you would do it for local users. We want to have tests that can be taken
>> by end-users as example how to use our software, that's why it is better
>> to be as close to real-world use-cases as it is possible.
>>
>>>
>>> Martin^3
>>>
>>
>>
>>
>
>
>

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.