<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<snip /><br>
<br>
Be careful, reviewed on friday! :-)<br>
<br>
1)<br>
whitespace error + pep8 error<br>
patch:76: trailing whitespace.<br>
# there is reverse zone for every ip address <br>
warning: 1 line adds whitespace errors.<br>
<br>
./ipaserver/install/bindinstance.py:640:9: E265 block comment should
start with '# '<br>
<br>
<br>
2) (server install)<br>
+ for ip in ip_addresses:<br>
+ for rev_zone in reverse_zones:<br>
+ if bindinstance.verify_reverse_zone(rev_zone, str(ip)):<br>
+ break<br>
+ else:<br>
+ sys.exit(1)<br>
<br>
Please add there error message instead 1 to exit function<br>
<br>
3) (server install)<br>
Code above, bindinstance.verify_reverse_zone(rev_zone, str(ip)):<br>
IMHO this will cause errors (not tested) try:<br>
rev-zones: [10.10.10.in-addr.arpa., 16.172.in-addr.arpa.]<br>
ip_addreses: [10.10.10.1, 172.16.0.1]<br>
<br>
it should be any() of zone can handle ip <br>
<br>
4) (replica-install)<br>
I'm not sure about behavior of combination ip addresses, and reverse
zones,<br>
In original code, if user specify reverse zone, the ip address must
match.<br>
<br>
In patched version, you just add new zones for ip-addresses which
doesn't math the user zones.<br>
<br>
IMO we should check if all addresses belongs to user specified
zones, otherwise raise an error.<br>
But I'm open to suggestions.<br>
<br>
5)<br>
Code for ipa-replica-install, and ipa-server-install, differs in
parts where we expecting same behavior<br>
- ip addresses and reverse zones<br>
<br>
6)<br>
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
<a href="https://engineering.redhat.com/irc-services.txt"></a>+ #
there is at least one ip address in every zone<br>
+ if options.reverse_zones:<br>
+ for reverse_zone in options.reverse_zones:<br>
+ for ip in config.ips:
<----------------------------------------------------------------------------------
A<br>
+ if bindinstance.verify_reverse_zone(reverse_zone,
ip):<br>
+
reverse_zones.append(bindinstance.normalize_zone(reverse_zone))<br>
+ break<br>
+ else:<br>
+ sys.exit(1)
<------------------------------------------------------------------------------------------------*<br>
+ # there is reverse zone for every ip address <br>
+ for ip in config.ips: <br>
+ for reverse_zone in options.reverse_zones:
<------------------------------------------------------- B<br>
+ if bindinstance.verify_reverse_zone(reverse_zone, ip):
<br>
+ if reverse_zone not in reverse_zones: <br>
+
reverse_zones.append(bindinstance.normalize_zone(reverse_zone))<br>
+ break<br>
+ else:
<------------------------------------------------------------------------------------------------------------
C<br>
+ reverse_zone = bindinstance.find_reverse_zone(ip)<br>
+ if reverse_zone is None and not options.no_reverse:<br>
+ reverse_zone = util.get_reverse_zone_default(ip)
<----------------------------------------- D1<br>
+ if not options.unattended and
bindinstance.create_reverse(): <------------------------- D<br>
+ reverse_zone =
bindinstance.read_reverse_zone(reverse_zone, ip)<br>
+
reverse_zones.append(bindinstance.normalize_zone(reverse_zone))
<------------- D2<br>
<br>
You added all possible zones in step A, B step is not needed.<br>
If user doesn't specify reverse_zones you can just jump to C step<br>
*add error message<br>
C: elif not options.no_reverse<br>
D: you asked user if wants to create zone, but you don't care about
answer, and in step D2 append zone from D1<br>
note: --no-reverse and --reverse-zones cant be used together, you
can use it in new code, test it before cycle<br>
<br>
7)<br>
# Always use force=True as named is not set up yet<br>
add_zone(self.domain, self.zonemgr,
dns_backup=self.dns_backup,<br>
- ns_hostname=api.env.host,
ns_ip_address=nameserver_ip_address,<br>
- force=True)<br>
+ ns_hostname=api.env.host, ns_ip_address=None,
force=True)<br>
+ #for ns_ip_address in nameserver_ip_address:<br>
+ # add_zone(self.domain, self.zonemgr,
dns_backup=self.dns_backup,<br>
+ # ns_hostname=api.env.host,
ns_ip_address=ns_ip_address,<br>
+ # force=True)<br>
<br>
Please remove commented section<br>
<br>
You can remove 'ns_ip_address=None' from function<br>
<br>
8)<br>
+ if set(options.ip_addresses) <= set(ips):<br>
+ ips = options.ip_addresses<br>
+ else:<br>
+ print >>sys.stderr, "Error: the hostname
resolves to an IP address that is different"<br>
+ print >>sys.stderr, "from the one provided on
the command line. Please fix your DNS"<br>
+ print >>sys.stderr, "or /etc/hosts file and
restart the installation."<br>
+ sys.exit(1)<br>
<br>
Could you write those extra addresses to output? We need to improve
usability of our error messages<br>
<br>
<br>
<pre class="moz-signature" cols="72">--
Martin Basti</pre>
</body>
</html>