On 3.4.2014 15:35, Jan Cholasta wrote:
On 2.4.2014 14:07, Martin Basti wrote:
Patch 30:
2)
+ if isinstance(labels, str):
+ if not labels:
+ raise ValueError('empty string')
...
+ elif isinstance(labels, unicode):
+ if not labels:
+ raise ValueError('empty string')
It might be nicer to:
+ if isinstance(labels, basestring) and not labels:
+ raise ValueError('empty string')
+
+ if isinstance(labels, str):
...
+ elif isinstance(labels, unicode):
Is it expected that you can't create root name?
I would like to imitate dns-python semantics as much as possible.
In [2]: dns.name.from_text("")
Out[2]: <DNS name .>
In [4]: dns.name.Name([])
Out[4]: <DNS name @>
In [5]: dns.name.Name([""])
Out[5]: <DNS name .>
I would like to see more descriptive error texts if you insist on current
semantics.
4)
+ @staticmethod
+ def get_root():
+ return DNSName(dns.name.root)
+
+ @staticmethod
+ def get_origin_sign():
+ return DNSName(u'@')
+
+ @staticmethod
+ def get_rev_zone():
+ return DNSName(u'in-addr.arpa.')
+
+ @staticmethod
+ def get_ip6_rev_zone():
+ return DNSName(u'ip6.arpa.')
I think you should either drop the "get_" prefix from the name, or (even
better) make these global constants.
I would shorten "origin_sign" to just "sign".
Sign of what? Decay? :-) I don't think that sign is descriptive enough, I
would personally stick with origin_sign.
Can you please use tuples of str objects (i.e. what dns.name.Name uses
internally) instead of unicode objects for the initialization? I think it
should be the preferred style of initializing DNSName objects (DN objects do
the same).
Please don't forget to consult dns-python3 and try to make migration from
dns-python to dns-python3 easy.
6)
+ def ToASCII(self, omit_final_dot=False):
+ return super(DNSName,
self).to_text(omit_final_dot=omit_final_dot).decode('ascii')
+
+ def ToUnicode(self, omit_final_dot=False):
+ return super(DNSName,
self).to_unicode(omit_final_dot=omit_final_dot).decode('utf-8')
What was the reason for the unusual naming again? I would prefer PEP-8
compatible names (e.g. "to_ascii" and "to_unicode"), but if the current names
absolutely have to stay, please add a comment with explanation.
I don't like the "omit_final_dot" flag. IMHO it should be dropped and whether
the result includes a final dot or not should depend solely on whether the
name is absolute or relative. You can still use e.g.
"name.derelativize(root).ToUnicode()" to drop the final dot, which is more
explanatory.
Generally, I agree. We can get rid of the relative/absolute name madness by
using final dot everywhere.
8)
+ def is_ip_reverse(self):
Please use is_ip4_reverse() instead. That name will always remind developer
not to forgot to IPv6 :-)
+ if self.is_subdomain(self.get_rev_zone()):
+ return True
+ return False
Patch 31:
2b)
+ except dns.name.NameTooLong:
+ raise ValueError(_('domain name cannot be longer than 255
characters'))
Personally, I would say '255 bytes' instead of '255 characters'. Characters
are tricky when IDN is involved etc.
Patch freeipa-mbasti-0036-DNSName-conversion-in-ipaldap.patch:
@@ -255,6 +256,10 @@ class IPASimpleLDAPObject(object):
'managedtemplate': DN,
'managedbase': DN,
'originscope': DN,
+ 'idnsname': DNSName,
+ 'idnssoamname': DNSName,
+ 'idnssoarname': DNSName,
+ 'dnszoneidnsname': DNSName,
})
Maybe you can add following attributes too:
CNAMERecord
DNAMERecord
MDRecord
NSRecord
PTRRecord
Patch 38:
1)
+_dns_zone_record = DNSName(u'@')
You know you already defined a constant for this in patch 30, right?
It seems that both constants are unnecessary because IMHO
DNSName(u'@')
is more readable and you don't need to dig into code to find out what the
cryptic name means.
4)
+def _hostname_validator_idn(ugettext, value):
+ assert isinstance(value, DNSName)
+
+ req_len = 2
+ if value.is_absolute():
+ req_len = 3
+ if len(value.labels) < req_len:
+ return _('invalid domain-name: not fully qualified')
This test is not correct because FQDN = an absolute name.
This code tries to guess if the name is FQDN or not but we can directly use
is_absolute() test.
4b)
def is_forward_record(zone, str_address):
addr = netaddr.IPAddress(str_address)
@@ -461,6 +444,7 @@ def is_forward_record(zone, str_address):
def add_forward_record(zone, name, str_address):
addr = netaddr.IPAddress(str_address)
+
try:
if addr.version == 4:
api.Command['dnsrecord_add'](zone, name, arecord=str_address)
Personally, I think that 'forward' is confusing (in this context). Could you
rename *_forward_record() functions to *_ipaddr_record(), please? (Maybe in a
separate patch...)
4c)
def add_records_for_host(host, domain, ip_addresses, add_forward=True,
add_reverse=True):
+ assert isinstance(host, DNSName)
+ assert isinstance(domain, DNSName)
The same applies to all places with:
+ assert isinstance(value, DNSName)
scattered all over the patch.
Is this really necessary? I thought that Python usually uses
http://en.wikipedia.org/wiki/Duck_typing
It seems like leftovers from development phase. (Maybe it is not the case for
IPA framework, correct me if I'm wrong :-)
Patch 39:
1)
- Str('hostname',
- _hostname_validator,
- normalizer=_normalize_hostname,
+ DNSNameParam('hostname',
+ #RFC 2317 section 5.2 -- don't have to be FQDN
It seems you are changing behavior here. Such things belong in separate patches.
Note: This is (mostly) related to classless reverse zones.
freeipa-mbasti-0040-Modified-record-and-zone-class-to-support-IDN.patch
* Records data are always returned as string
* Attributes idnsname, idnssoamname, idnssoarname are returned as DNSName, with
option --raw as string
* option --raw returns all IDN domains punycoded
I have mentioned
CNAMERecord
DNAMERecord
MDRecord
NSRecord
PTRRecord
above. Their are also pure DNS names ...
def get_name_in_zone(self, zone, hostname):
[snip]
- be added to a zone
+ be added to a zone. Returns '@' when the hostname is directly in the
zone
I would replace "in the zone" with "at zone apex". Technically, all records
with suffix = zone are "in" the zone.
freeipa-mbasti-0046-DNS-new-tests.patch
+idnzone1_ip = u'192.168.11.1'
[snip]
+revidnzone1 = u'15.168.192.in-addr.arpa.'
Please use addresses from 172.16/12 prefix so IPA tests don't consume more
than one prefix.
More prefixes we use => higher probability that it will clash with somebody's
environment.
I'm looking forward to see this is IPA! Good work!
--
Petr^2 Spacek
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
