[Avocado-devel] RFC: Configuration by convention

[Lukáš Doktor]

Dne 03. 12. 19 v 13:20 Beraldo Leal napsal(a):
> On Tue, Dec 03, 2019 at 07:28:10AM +0100, Lukáš Doktor wrote:
>>>> The default comes from /etc. You can checkout the `avocado/plugins/run.py` for details:
>>>>
>>>>         sysinfo_default = settings.get_value('sysinfo.collect',
>>>>                                              'enabled',
>>>>                                              key_type='bool',
>>>>                                              default=True)
>>>>         sysinfo_default = 'on' if sysinfo_default is True else 'off'
>>>>         parser.add_argument('--sysinfo', choices=('on', 'off'),
>>>>                             default=sysinfo_default, help="Enable or disable "
>>>>                             "system information (hardware details, profilers, "
>>>>                             "etc.). Current:  %(default)s")
>>>>
>>>> 1. `--sysinfo on` => force-enable
>>>> 2. `--sysinfo off` => force-disable
>>>> 3. no `--sysinfo` => use the value from /etc
>>>
>>> IIUC, the default comes from the source code:
>>>
>>>   settings.get_value(..., default=True)
>>>
>>> If the option is missing on config file and there is no --sysinfo option
>>> on command line, the default will be "True", right?
>>>
>>> So, there are 3 ways to use, but the possible values are "on" or "off",
>>> right? Fix-me if I'm wrong, but what is the third option other than,
>>> "on" and "off"? The key_type is still a bool, right?
>>>
>>
>> 1 -> will be always enabled
>> 2 -> will be always disabled
>> 3 -> depends on /etc configuration. If not present there it uses True
>>
>> So the (3) means it reflects the configuration option. If we make it
>> only a bool (present/missing) there would be no way to force-override
>> the /etc configuration.
> 
> Sorry if I'm insisting on this, but I just would like to make sure that
> I'm not missing anything.
> 
> Regarding the "tri-state" ...
> 
> My question is: What happens, in terms of behavior, if `sysinfo = none`?
> Is there behavior for "none"? You are showing me that the "third state"
> is "none", but what happens if it is "none"?
> 

Let's just have a look at the code again:

        sysinfo_default = settings.get_value('sysinfo.collect',
                                             'enabled',
                                             key_type='bool',
                                             default=True)
        # sysinfo_default will be True of False based on /etc configuration
        sysinfo_default = 'on' if sysinfo_default is True else 'off'
        parser.add_argument('--sysinfo', choices=('on', 'off'),
                            default=sysinfo_default, help="Enable or disable "
                            "system information (hardware details, profilers, "
                            "etc.). Current:  %(default)s")
        # The default will be either "on" or "off" based on the "sysinfo_default"

So you can see that based on /etc configuration the default is changed and immediately reflected in `-h` output.

> Some logic that could help you understanding my findings and thought:
> 
>     1. Configuration file parse is already a bool:
>        * settings.get_value has key_type = bool;
>        * settings.get_value has default = True;
>        * settings.get_value has an "allow_blank" set to False;
>        * At the end of this parsing, the line above will transform
>        anything into "on" or "off":
> 
> 	sysinfo_default = 'on' if sysinfo_default is True else 'off'
> 
>     2. Argument Parse is not an explicit bool, but it is limited to
>     "on/off" with default to "on" or "off". 
>       * At the end of this parsing, we have "on" or "off".
> 

Yes, in this case we decided to use `choices=('on', 'off')`, the third state is generated by different default supplied to argparse. Note that I'd prefer allowing on/off/config (or none) to explicitly say we want to reflect the configuration and then evaluate the configuration option only when necessary. But this is only a minor detail.

> So, IIUC, in terms of behavior, we have only: disabled or enabled. At
> the end of all parsing stuff, it will reflect only on enabled or
> disabled. I could not find any code execution (behavior) based on
> "none". Please, show me what happens when sysinfo = none.
> 
> Regarding the "force-override the /etc configuration" ...
> 
> IMO, we could improve the code here, with some basic and common logic,
> and keeping the "force-override" in /etc/:
> 
>     1. Source code knows the default values, let's say the "config object"
>     into memory;
> 
>     2. Parse configuration file options into the "config object",
>     overwriting the defaults;
> 
>     3. Parse the command line arguments into the "config object",
>     overwriting the configuration file;
> 
>     4. Let the "config object" (already parsed) available to all code.
> 
> I understand that we don't have a 1:1 mapping between config file
> options and argument options, and item 3 is limited. But the current
> state already like that, right?
> 

And now we are getting to the mapping I was talking about in the first review. I'd like the RFC to include at least some recommendations and guidelines with regards to source code, /etc config and arguments. It can't be 1:1, but we could define some categories that would share the same semantics.

> Again, sorry for the insistence, let me know if I'm missing something.
> 

No problem, that is the purpose of RFC, let's talk until everything is clear.
Lukáš

> Regards,
> Beraldo
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/avocado-devel/attachments/20191203/0e725851/attachment.sig>