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

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to