[Freeipa-devel] [PATCH] 0001-2 ipatests: SOA record Maintenance tests

[Martin Basti]

On 27/03/15 16:34, Aleš Mareček wrote:
> Greetings!
> Martin, thanks for your review and comments!
> I changed the name of the patch and setup my git variables properly. I also re-tested it and got all passed. I'm sending a new patch that is attached.
>
> ----- Original Message -----
>> From: "Martin Basti" <mbasti at redhat.com>
>> To: "Aleš Mareček" <amarecek at redhat.com>, freeipa-devel at redhat.com
>> Sent: Tuesday, March 24, 2015 4:39:21 PM
>> Subject: Re: [Freeipa-devel] [PATCH] 0001 ipatests: SOA record Maintenance tests
>>
>> On 24/03/15 15:06, Aleš Mareček wrote:
>>> Greetings!
>>> This is my very first patch, ticket#4746.
>>>
>>> Have a nice day!
>>>    - alich -
>>>
>>>
>> Thank you for the patch. Just nitpicks:
>>
>> 1)
>> +    cleanup_commands = [
>> +        ('dnszone_del', [zone6], {'continue': True}),
>> +        ('dnszone_del', [zone6b], {'continue': True}),
>> +    ]
>>
>> would be better do it in this way, continue option will to try remove
>> all zones:
>> +    cleanup_commands = [
>> +        ('dnszone_del', [zone6, zone6b], {'continue': True}),
>> +    ]
>>
> Done.
>
>> 2)
>> I'm fine with zone6b, but was there any reason to create zone6b, instead
>> of reusing zone 1 or 2 or 3?
> Because of some updates needs, I didn't want to break anything existing thus I created new.
>
>> 3)
>> Please fix whitespace errors.
>> $ git am
>> freeipa-alich-0001-ipatests-added-tests-for-SOA-record-Maintenance.patch
>> Applying: ipatests - added tests for SOA record Maintenance
>> /home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:482: trailing
>> whitespace.
>>
>> /home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:758: new blank
>> line at EOF.
>> +
>> warning: 2 lines add whitespace errors.
>>
> Done.
> $ git am freeipa-alich-0001-2-Ipatests-DNS-SOA-Record-Maintenance.patch
> Applying: Ipatests DNS SOA Record Maintenance
> $
>
>> 4)
>> I know the dns plugin tests are so far from PEP8, but try to keep PEP8
>> in new code
> Done, only 1 line persisted that I didn't want to break:
> zone6_unresolvable_ns_relative_dnsname = DNSName(zone6_unresolvable_ns_relative)
>
>> Otherwise test works as expected.
>>
>> Martin^2
>>
>> --
>> Martin Basti
>>
>>
> Thanks!
>   - alich -
Thank you, ACK.

-- 
Martin Basti