[Freeipa-devel] [PATCH 0003] Test validity of URIs in certificate

[Martin Basti]

On 08.08.2016 10:30, Martin Basti wrote:
>
>
>
> On 02.08.2016 14:50, Lenka Doudova wrote:
>>
>>
>>
>> On 07/29/2016 11:43 AM, Lenka Doudova wrote:
>>>
>>>
>>>
>>> On 07/29/2016 11:41 AM, Lenka Doudova wrote:
>>>>
>>>> On 07/28/2016 01:35 PM, Peter Lacko wrote:
>>>>> Hops, fixed.
>>>>>
>>>>> Peter
>>>>>
>>>>>
>>>>> ----- Original Message -----
>>>>> From: "Lenka Doudova"<ldoudova at redhat.com>
>>>>> To:freeipa-devel at redhat.com
>>>>> Sent: Thursday, July 28, 2016 1:32:25 PM
>>>>> Subject: Re: [Freeipa-devel] [PATCH 0003] Test validity of URIs in	certificate
>>>>>
>>>>> Hi,
>>>>>
>>>>> I cannot find any attached patch :)
>>>>>
>>>>> Lenka
>>>>>
>>>>>
>>>>> On 07/28/2016 01:30 PM, Peter Lacko wrote:
>>>>>> Attached you can find a patch adding test for URIs in generated certificate
>>>>>> ipatests/test_xmlrpc/test_cert_plugin.py
>>>>>> Since I'm leaving Red Hat in end of July, I won't be able to modify this patch anymore.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Peter
>>>>>>
>>>>>
>>>>>
>>>> Hi,
>>>>
>>>> NACK. Code looks fine and works well on master branch, but patch 
>>>> does not apply on 4-3 and 4-2 branches.
>>>> Peter left the company but claimed he can fix the patch if 
>>>> necessary, I'll communicate it with him or fix it myself.
>>>>
>>>> Lenka
>>>>
>>>>
>>> Oh, and forgot this one - PEP8 error:
>>> ./ipatests/test_xmlrpc/test_cert_plugin.py:191:80: E501 line too 
>>> long (105 > 79 characters)
>>>
>>> Lenka
>>>
>>>
>> Hi,
>>
>> since Peter has quit already, I took it upon myself to do minor fix 
>> and rebase to the patch.
>> 1) i removed pylint disable comments from the patch, as they were 
>> unnecessary (this also solved PEP8 error)
>> 2) I rebased the patch to be applicable for ipa-4-3 branch.
>> Original functionality of the patch remains unchanged.
>>
>> Both fixed patches attached.
>>
>> Lenka
>>
>>
>
> Hello,
>
> 1)
> This is not needed
> +        global sn
> +
> +        result = api.Command.cert_show(sn, out=unicode(self.certfile))
>
> you need the global statement only for write access. But sn is not assigned in this code block.
>
> 2)
> I prefer to use instance attributes (self.sn) instead of global variables

As we figured out, pytest creates for each test new instance of class, 
so 2) will not fork.
Please fix only 1), sorry.

Martin^2

>
> Martin^2
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160810/c32f50e7/attachment.htm>