On 03/29/2013 03:48 PM, Ana Krivokapic wrote:
On 03/29/2013 03:11 PM, Tomas Babej wrote:
On 03/29/2013 02:15 PM, Ana Krivokapic wrote:
On 03/26/2013 04:59 PM, Tomas Babej wrote:
Hi,

The ipa-replica-install script tries to add replica's A and PTR
records to the master DNS, if master does manage DNS. However,
master need not to manage replica's zone. Properly handle this use
case.

https://fedorahosted.org/freeipa/ticket/3496



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

The patch works well and fixes the issue.

Just a couple of nitpicks:

1) "However, master need not to manage replica's zone." -- This sentence sounds a little strange to me, but I am not a native speaker so I may be wrong about that.

The phrase should be ok. I assume you're worried about "need not" construct, which may sound a bit unusal as opposed to, for example, "does not need to".

One could argue that it sounds archaic. However, consider the following chart, which clearly proves the opposite:

http://books.google.com/ngrams/chart?content=need%20not%2Cneeds%20not%2Cdoes%20not%20need%20to%2Cdoesn%20'%20t%20need%20to&corpus=0&smoothing=3&year_start=1800&year_end=2000 <http://books.google.com/ngrams/chart?content=need%20not%2Cneeds%20not%2Cdoes%20not%20need%20to%2Cdoesn%20%27%20t%20need%20to&corpus=0&smoothing=3&year_start=1800&year_end=2000>

For more detailed explanation, see:

http://english.stackexchange.com/questions/29409/why-use-need-not-instead-of-do-not-need-to

Actually, the part that sounded weird to me is the "to" that comes after "need not" in your commit message. Also, the stackexchange link you provided states: "This /need/ is a *modal verb*: it always requires an infinitive without /to/;".

Sorry that I wasn't clear about this in my first email.
Yes, that's a mistake on my part, thanks fot catching that. Fixed the commit message.


2) There are three PEP8 501 errors introduced by the patch, but given the recent discussion on this subject, I think it is really up to you if you want to take the time to fix these.

Sure I do. Thanks for the catch. Updated patch attached.
There is still one line with E501:

install/tools/ipa-replica-install:303:80: E501 line too long (80 > 79 characters)
I left that one so intentionally. Imho, it would only mangle the line unnecessarily, the line is exactly 80 characters long with no nice point where to break it.



ACK from the functional perspective.

--
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.



--
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

>From f8ab59bb882ada801ae15d4951edd91431ca0d2b Mon Sep 17 00:00:00 2001
From: Tomas Babej <tba...@redhat.com>
Date: Tue, 26 Mar 2013 16:09:24 +0100
Subject: [PATCH] Properly handle ipa-replica-install when its zone is not
 managed by IPA

The ipa-replica-install script tries to add replica's A and PTR
records to the master DNS, if master does manage DNS. However,
master need not manage replica's zone. Properly handle this use
case.

https://fedorahosted.org/freeipa/ticket/3496
---
 install/tools/ipa-replica-install | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/install/tools/ipa-replica-install b/install/tools/ipa-replica-install
index 94d60bec64697f775c0303b38f481129d554a0f4..7a50398cfdb17a33a68d4973b71fbb95fc641667 100755
--- a/install/tools/ipa-replica-install
+++ b/install/tools/ipa-replica-install
@@ -299,13 +299,23 @@ def install_dns_records(config, options):
     # before our DS server is installed.
     with temporary_ldap2_connection(
             config.master_host_name, config.dirman_password):
-        bind = bindinstance.BindInstance(dm_password=config.dirman_password)
-        reverse_zone = bindinstance.find_reverse_zone(config.ip)
+        try:
+            bind = bindinstance.BindInstance(dm_password=config.dirman_password)
+            reverse_zone = bindinstance.find_reverse_zone(config.ip)
+
+            bind.add_master_dns_records(config.host_name, config.ip_address,
+                                        config.realm_name, config.domain_name,
+                                        reverse_zone, options.conf_ntp,
+                                        options.setup_ca)
+        except errors.NotFound, e:
+            root_logger.debug('Replica DNS records could not be added '
+                              'on master: %s', str(e))
+
+        # we should not fail here no matter what
+        except Exception, e:
+            root_logger.info('Replica DNS records could not be added '
+                             'on master: %s', str(e))
 
-        bind.add_master_dns_records(config.host_name, config.ip_address,
-                                    config.realm_name, config.domain_name,
-                                    reverse_zone, options.conf_ntp,
-                                    options.setup_ca)
 
 def check_dirsrv():
     (ds_unsecure, ds_secure) = dsinstance.check_ports()
-- 
1.7.11.7

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

Reply via email to