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

[Oleg Fayans]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0049.2-Added-interface-to-certutil.patch
Type: text/x-patch
Size: 1165 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160906/fe8b0d2b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0050.3-Automated-test-for-certs-in-idoverrides-feature.patch
Type: text/x-patch
Size: 6498 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160906/fe8b0d2b/attachment-0001.bin>