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" <mba...@redhat.com>
To: "Aleš Mareček" <amare...@redhat.com>, freeipa-devel@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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to