[Freeipa-devel] [PATCH 0043] Properly handle ipa-replica-install when its zone is not managed by IPA

[Rob Crittenden]

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 at 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.
>
> 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