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

[Jan Cholasta]

On 26.8.2015 10:51, Petr Spacek wrote:
> 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.

Sorry, but no. The installers are supposed to be callable from many 
different kinds of often incompatible environments. How exactly would 
you imagine e.g. a Python API to look like given it should use the same 
conventions as CLI? Like this:

     server_install(['no_ntp', ('forwarder', '1.2.3.4'), ('forwarder', 
'5.6.7.8')])

? I would very much prefer if it looked like this:

     server_install(ntp=False, forwarders=['1.2.3.4', '5.6.7.8'])

which is pretty much the same I suggested for config files and better 
reflects the semantics of setting configuration options.

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


-- 
Jan Cholasta