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

[Martin Babinsky]

On 07/30/2015 08:55 AM, 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
>
>>>
>>> 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
>

Attaching updated patch.

-- 
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0051.1-IPA-server-and-replica-installers-can-accept-options.patch
Type: text/x-patch
Size: 5496 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150803/9a522c1b/attachment.bin>