[Freeipa-devel] [PATCH 0113] ipa-client: Set NIS domain name in the installer

[Tomas Babej]

On 09/26/2013 03:16 PM, Petr Viktorin wrote:
> On 09/26/2013 02:58 PM, Martin Kosek wrote:
>> On 09/26/2013 02:45 PM, Jan Cholasta wrote:
>>> On 26.9.2013 14:38, Martin Kosek wrote:
>>>> On 09/26/2013 02:28 PM, Tomas Babej wrote:
>>>>> On 09/26/2013 12:20 PM, Jan Cholasta wrote:
>> ...
>>>>> I just found --no-nisdomain more descriptive and explicit. If 
>>>>> there is a
>>>>> consensus, I can remove it.
>>>>>
>>>>
>>>> I am not aware of any precedent that would warrant --nisdomain="".
>
> We sort of have precedent in `ipa` in multivalued options, leaving 
> those empty deletes the values.
>
>>> I have seen concerns about the number of ipa-client-install options 
>>> in the past
>>> (not by me).
>>
>> IMHO, we are currently OK on this front. Having options categorized in
>> sections, as we already do, helps.
>>
>>>> IMO --no-nisdomain is more consistent with rest of the options.
>>>
>>> I don't see any other --<option>=<value> and --no-<option> option 
>>> pair in
>>> ipa-client-install, so what consistency are you talking about?
>>
>> I was referring to --no-ssh, --no-ntp and similar. But it is true 
>> that these
>> rather disable entire features than delete a value. I do not punt on 
>> this,
>> --nidomain="" may be OK as well.
>
> IMO empty option values are awkward; --no-nisdomain is more 
> user-friendly, and can be explained more clearly, even though it needs 
> an additional option.
>

OK, we let this rot on the list for a while.

I retest the patch and it still applies and works with the current master.

I think we should keep both options, no-nisdomain is more descriptive 
and an explicit option is more necessary here since we are setting 
nisdomain by default. Hence I would avoid having to use --nisdomain="" 
to disable setting the nisdomain, since it is rather implicit (even if 
we commented on it in the option description).

Option-nitpicking aside, I think this patch is ready for a proper 
functional review.

-- 
Tomas Babej
Associate Software Engeneer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org