[Avocado-devel] RFC: Configuration by convention

Wed Dec 4 00:18:47 UTC 2019

On Tue, Dec 03, 2019 at 09:20:59AM -0300, Beraldo Leal wrote:
> 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"?
> 
> 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'
>

Right.

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

Right.

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

Right, there's no behavior for *sysinfo collection* (after all parsing
is done) when it's set to None.  You're really unveiling special logic
that is specific to the `sysinfo` command line parsing (which involves
the configuration file).  IMO, the confusion to get to the point of dual
or tri state, reveals this custom logic is problematic.

I know the Job API is not the focus here, but consider that we're now
exposing the sysinfo configuration (forget the configuration file)
through a new interface (yes, the Job API).  What happens now when
Avocado's "job" code checks for the *given* configuration (a
dictionary) and doesn't find sysinfo explicitly set?  That's the hard
question to answer here IMO, that will set the direction right for the
builtin behavior of a feature.

> 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 like the "all code", and the consistent approach for all options.
Sysinfo should not be different (and have configuration file parsing
alongside with command line parsing) and what depending on whom you
ask, has 3 different states.

Just for the sake of comparison, this is very old and outdated code
but I think it reflects some of the principles here.  For instance,
what you call in #1 "source code knows the defaults value", I believe
are something "builtin defaults", defined here:

https://github.com/clebergnu/arc/blob/master/arc/defaults.py

Notice the description of the module:

   """
   This module holds a collection of data that is set as the default
   value for a number of items that control the behavior of both the
   API and the CLI tool.
   """

Both API and CLI tool usage is covered by the same "builtin" defaults,
defined in the lowest level code possible.  Then, it states:

   """
   For API users, when a given function or method is not called with
   and explicit value (usually having a parameter that at the function
   call level defaults to 'None'), these values are automatically
   used.

   For the CLI tool, these values are set as defaults when an explicit
   value is not provided either on the configuration file or on the
   command line.
   """

So the behavior when an option is ommited should be consistent across
API and CLI users too.  Finally,  the CLI tool, as a bonus, shows both
the builtin defaults:

   https://github.com/clebergnu/arc/blob/master/arc/cli/args/parser.py#L62

And configured defaults:

   https://github.com/clebergnu/arc/blob/master/arc/cli/args/parser.py#L67

Again, the important aspect is that the behavior is consistent and
completely outside the parsing of one one specific option.

Sorry if I overwhelmed this discussion with too much info and not
enough objectiveness, but giving alternative examples was the best I
could think for now.

- Cleber.

> 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?
> 
> Again, sorry for the insistence, let me know if I'm missing something.
> 
> Regards,
> Beraldo
> 
> 