[Freeipa-devel] [Test][Patch-0049, 0050] Certs in ID overrides test
Martin Basti
mbasti at redhat.com
Tue Aug 30 11:58:56 UTC 2016
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
2)
+ new_args = " ".join(new_args + args)
you don't need this, run_command takes list as argument too
new_args.extend(args)
3)
To make it more usable you should add raiseonerr as kwarg to
run_certutil (True as default)
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)
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__
3)
I have question, why do you need AD for this test? AFAIK you can use ID
overrides without AD
Martin^3
More information about the Freeipa-devel
mailing list