[Freeipa-devel] [PATCH 0051] IPA server and replica installers can accept options from config file

[Petr Spacek]

On 30.7.2015 08:55, Jan Cholasta wrote:
> Dne 29.7.2015 v 17:43 Petr Vobornik napsal(a):
>> On 07/29/2015 05:13 PM, Martin Babinsky wrote:
>>> On 07/29/2015 01:25 PM, Jan Cholasta wrote:
>>>> Dne 29.7.2015 v 12:20 Martin Babinsky napsal(a):
>>>>> Initial attempt to implement
>>>>> https://fedorahosted.org/freeipa/ticket/4517
>>>>>
>>>>> Some points to discuss:
>>>>>
>>>>> 1.) name of the config entries: currently the option names are derived
>>>>> from CLI options but have underscores in them instead of dashes. Maybe
>>>>> keeping the CLI option names also for config entries will make it
>>>>> easier
>>>>> for the user to transfer their CLI options from scripts to config
>>>>> files.
>>>>
>>>> NACK. There is no point in generating config names from CLI names, which
>>>> are generated from knob names - use knob names directly.
>>>>
>>> The problem is that in some cases the  cli_name does not map directly to
>>> knob name, leading in different naming of CLI options and config
>>> entries, confusion and mayhem.
> 
> What works for CLI may not work for config files and vice versa. For example,
> this works for CLI:
> 
>     --no-ntp
>     --no-forwarders
>     --forwarder 1.2.3.4 --forwarder 5.6.7.8
> 
> but this works better in config file:
> 
>     ntp = False
>     forwarders =
>     forwarders = 1.2.3.4, 5.6.7.8

Personally I would say that Honza's approach is more eye-candy but IMHO *not*
more usable - and I prefer usability. Alexander's approach requires no other
documentation that `ipa-server-install --help` or even better just
copy&pasting arguments users already have in scripts to a file.

In this case I believe that using anything incompatible with CLI arguments is
not worth because it requires yet another layer of documentation to make it
usable.

BTW GnuPG does the very same thing as Alexander mentioned with
.gnupg/gpg.conf, i.e. the config file simply accepts all options from command
line, with the same names and parameters - and that that is it.

Petr^2 Spacek

>>> These are some offenders from `ipaserver/install/server.py`:
>>> http://fpaste.org/249424/18226114/
>>>
>>> On the other hand, this can be an incentive to finally put an end to
>>> inconsistent option/knob naming across server/replica/etc. installers.
> 
> Yes please.
> 
>>
>> If the names are different than cli names, then they should be made
>> discoverable somehow or be documented.
> 
> IMHO documenting them is easy.
> 
>>
>>>>>
>>>>> 2.) Config sections: there is currently only one valid section named
>>>>> '[global]' in accordance with the format of 'default.conf'. Should we
>>>>> have separate sections equivalent to option groups in CLI (e.g.
>>>>> [basic],
>>>>> [certificate system], [dns])?
>>>>
>>>> No, because they would have to be maintained forever. For example, some
>>>> options are in wrong sections and we wouldn't be able to move them.
>>>>
>>> I'm also more inclined to a single section, at least for now since we
>>> are pressed for time with this RFE.
>>>
>>> That's not to say that we should ditch Alexander's idea about separate
>>> sections with overrides for different hosts. We should consider it as a
>>> future enhancement to this feature once the basic plumbing is in place.
> 
> Right.
> 
>>>>>
>>>>> 3.) Handling of unattended mode when specifying a config file:
>>>>> Currently there is no connection between --config-file and unattended
>>>>> mode. So when you run ipa-server-install using config file, you still
>>>>> get asked for missing stuff. Should '--config-file' automatically imply
>>>>> '--unattended'?
>>>>
>>>> The behavior should be the same as if you specified the options on the
>>>> command line. So no, --config-file should not imply --unattended.
>>>>
>>> That sound reasonable. the code behaves this way already so no changes
>>> here.
>>>
>>>>>
>>>>> There are probably other issues to discuss. Feel free to write
>>>>> email/ping me on IRC.
>>>>>
>>>>
>>>> (I haven't looked at the patch yet.)
>>>>
>>> Please take a look at it ASAP. I am on PTO tomorrow and on Friday, but I
>>> will find time to work at it in the evening if you send me you comments.
> 
> 1) IMO the option should be in the top-level option section, not in a separate
> group (use "parser.add_option()").
> 
> Also maybe rename it to --config, AFAIK that's what is usually used.
> 
> A short name ("-c"?) would be nice too.
> 
> Nitpick: if the option is named --config-file, dest should be "config_file",
> to make it easier to look it up in the code.
> 
> 
> 2) Please don't duplicate the knob retrieval code, store knobs in a list and
> pass that as an argument to parse_config_file.
> 
> 
> 3) I'm not sure about using newline as a list separator. I don't know about
> other IPA components, but SSSD in particular uses commas, maybe we should be
> consistent with that?
> 
> 
> 4) Booleans should be assignable either True or False, i.e. do not use
> _parse_knob to parse them.
> 
> 
> Honza