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

[Petr Vobornik]

On 19.6.2014 16:55, Martin Basti wrote:
> 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.
>

ACK, pushed to master:

* 49068ade92b3fa4874b3107923bbd5b84e1a04f3 Separate master and forward 
DNS zones
* 266015c3e28c04894cd3a52ea2f99c25eef2c6a9 Prevent commands to modify 
different type of a zone
* 727f5f33732df252fa99d5c168d6727589ee6076 Create BASE zone class
* 11c250a6125da300f85880e090e5db1659078daa Tests DNS: forward zones


-- 
Petr Vobornik