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

[Martin Basti]

On 06.09.2016 13:57, 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
>>>
>>
>>
>>
>
>
>


1)
I still don't see the reason why AD trust is needed. Default trust ID 
view is added just by ipa-adtrust-install, adding trust is not needed 
for current implementation. You don't need AD for this, IDviews is 
generic feature not just for AD. Is that user configured on AD side?

2)
The test itself looks for me as just API/CLI test. IMO it can be in 
ipatests/test_xmlrpc/test_idviews_plugin.py or 
ipatests/test_xmlrpc/test_add_remove_cert_cmd.py

3)
I don't see any integration with SSSD in that test, just pure IPA CLI 
test, shouldn't be this tested against SSSD here?


Martin^2
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160914/9ff0e50b/attachment.htm>