Re: [Freeipa-devel] [PATCH 0043] Properly handle ipa-replica-install when its zone is not managed by IPA
Ana Krivokapic wrote: On 03/29/2013 04:00 PM, Tomas Babej wrote: 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%20tocorpus=0smoothing=3year_start=1800year_end=2000 http://books.google.com/ngrams/chart?content=need%20not%2Cneeds%20not%2Cdoes%20not%20need%20to%2Cdoesn%20%27%20t%20need%20tocorpus=0smoothing=3year_start=1800year_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. OK, makes sense. 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. ACK Pushed to master and ipa-3-1 rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0043] Properly handle ipa-replica-install when its zone is not managed by IPA
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%20tocorpus=0smoothing=3year_start=1800year_end=2000
http://books.google.com/ngrams/chart?content=need%20not%2Cneeds%20not%2Cdoes%20not%20need%20to%2Cdoesn%20%27%20t%20need%20tocorpus=0smoothing=3year_start=1800year_end=2000
For more detailed explanation, see:
http://english.stackexchange.com/questions/29409/why-use-need-not-instead-of-do-not-need-to
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.
ACK from the functional perspective.
--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.
From e2eb28cbcd85bc34293966b200279aa0376001a0 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 to 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
Re: [Freeipa-devel] [PATCH 0043] Properly handle ipa-replica-install when its zone is not managed by IPA
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%20tocorpus=0smoothing=3year_start=1800year_end=2000
http://books.google.com/ngrams/chart?content=need%20not%2Cneeds%20not%2Cdoes%20not%20need%20to%2Cdoesn%20%27%20t%20need%20tocorpus=0smoothing=3year_start=1800year_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,
-
Re: [Freeipa-devel] [PATCH 0043] Properly handle ipa-replica-install when its zone is not managed by IPA
On 03/29/2013 04:00 PM, Tomas Babej wrote: 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%20tocorpus=0smoothing=3year_start=1800year_end=2000 http://books.google.com/ngrams/chart?content=need%20not%2Cneeds%20not%2Cdoes%20not%20need%20to%2Cdoesn%20%27%20t%20need%20tocorpus=0smoothing=3year_start=1800year_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. OK, makes sense. 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. ACK -- Regards, Ana Krivokapic Associate Software Engineer FreeIPA team Red Hat Inc. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
