[Freeipa-devel] [PATCHES] 0052-0055 Separate master and forward DNS zones to separate objectClasses

[Martin Basti]

On Thu, 2014-06-19 at 15:16 +0200, Petr Vobornik wrote:
> On 18.6.2014 13:42, Martin Basti wrote:
> > Rebased patches with pep8 fixes attached
> 
> git diff HEAD~4 -U0 | pep8 --diff --ignore=E501,E126,E128,E124
> ./ipalib/plugins/dns.py:1754:9: E265 block comment should start with '# '
> ./ipalib/plugins/dns.py:1755:9: E265 block comment should start with '# '
> ./ipalib/plugins/dns.py:2550:13: E265 block comment should start with '# '
> ./ipalib/plugins/dns.py:2551:16: E713 test for membership should be 'not in'
> ./ipalib/plugins/dns.py:3516:9: E265 block comment should start with '# '
> ./ipalib/plugins/dns.py:3686:12: E713 test for membership should be 'not in'
Thanks, I had an older pep8 version which didn't show me that
> 
> I don't like how the patches are structured. It's hard to review. IMHO 
> you should have started with creation of the base class and then build 
> the forward zone on top of it. Reading a bunch of copy-pasted code with 
> minor changes which is then removed in the next patch is a waste of 
> time.  Current approach is acceptable if the patch set is huge and 
> rebases would be really difficult. Or if it's sent and reviewed 
> separately. But what's done is done.
Sorry for that.

> 
> 1. Is it possible to disable the interactive wizard for dnsrecord-add 
> forward.zone.? It would be nice to show the error right at the beginning.
It is partially possible. It requires to change interactive wizard for
dnsrecord-mod, dnsrecord-del too.
I vote for don't change it.

> Patch 54-5:
> 2. You forgot to remove the 'execute' method from 'dnszone_disable' and 
> 'dnszone_enable'
> 
> 3. 'pre_callback' in 'dnsforwardzone_find' seems to be redundant, it 
> just calls the parent's pre_callback with no additional logic
Thank you, I will remove it
> 
> General question:
> 
> Looking at the help text, especially the `--name=DNSNAMEPARAM`, I wonder 
> if have somewhere documented the formats of various param types.
I dont know, I found nothing.
I'm thinking about, rename it to DNSNAME only

Rebased and fixed patches attached
Thank you.
-- 
Martin^2 Basti
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0052-6-Separate-master-and-forward-DNS-zones.patch
Type: text/x-patch
Size: 21640 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140619/43d66d6d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0053-6-Prevent-commands-to-modify-different-type-of-a-zone.patch
Type: text/x-patch
Size: 11862 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140619/43d66d6d/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0054-6-Create-BASE-zone-class.patch
Type: text/x-patch
Size: 41133 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140619/43d66d6d/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0055-6-Tests-DNS-forward-zones.patch
Type: text/x-patch
Size: 29017 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140619/43d66d6d/attachment-0003.bin>