[Avocado-devel] RFC: Configuration by convention

Tue Dec 3 20:00:47 UTC 2019

On Mon, Dec 02, 2019 at 03:56:44PM -0300, Beraldo Leal wrote:
> Hi Lukáš,
> 
> My comments are below. If I didn't reply to a specific topic is because
> I'm waiting for more replies to take into consideration.
> 
> On Mon, Dec 02, 2019 at 04:57:28PM +0100, Lukáš Doktor wrote:
> > Dne 02. 12. 19 v 15:30 Beraldo Leal napsal(a):
> > > On Thu, Nov 28, 2019 at 04:00:17PM +0100, Lukáš Doktor wrote:
> > >> Dne 21. 11. 19 v 22:23 Beraldo Leal napsal(a):
> > >>> Motivation
> > >>> ##########
> > >>>
> > >>> An Avocado Job is primarily executed through the `avocado run` command line.
> > >>> The behavior of such an Avocado Job is determined by parsing the following
> > >>> settings (listed in parsed order):
> > >>>
> > >>>  1) Default values in source code
> > >>>  2) Configuration file contents
> > >>>  3) Command-line options
> > >>>
> > >>
> > >> I'm missing in this RFC some kind of mapping the above. I think if we
> > >> are to do those intrusive changes, we should spend some time on
> > >> specifying the relations. (but maybe I just missed it)
> > > 
> > > Not sure if I understood. What kind of relations do you suggest? Maybe
> > > now, I'm missing something. The idea was to simply show the order that
> > > options are parsed and overridden, just to give some context.
> > > 
> > 
> > While talking about standardization of args/conf options I though it'd
> > be nice to define some standard mapping to override configuration
> > options from cmdline arguments. What I mean is eg the `--sysinfo`.
> > Obviously it whouldn't fit everything, but would be nice to know that:
> > 
> > --sysinfo $VALUE
> > 
> > is the same as
> > 
> > ```
> > [sysinfo.collect]
> > enabled = $VALUE
> > ```
> > 
> > and maps to:
> > 
> > {sysinfo: $VALUE}
> > 
> > job param. Obviously there would be exceptions, but we could create
> > guidelines to cover most cases. Additionally even discussing this
> > might help identify the sections/arguments.
> 
> Yes, I see and agree with you. The idea was to add this in a
> "Configuration Reference" section in one of our guides. That was what I
> tried to explain in the "How to Teach This".
>

Good, looks like there's agreement that we'll need examples in the
documentation.

> > 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?
> 
> The default option, IMO, should be in source code, not /etc. In that
> way, the user can have a minimum config file or an empty one. And I
> think that is what is implemented, today.
>

I think this is being looked at from different perspectives.  To
summarize what I've learned so far, using "sysinfo" command line
option:

 1) The setting it controls is boolean (so it's either on or off)
 2) Default comes from the configuration file
 3) Combining 1 and 2 leads to different possible states

It looks like options such as `--keep-tmp` did not follow the same
approach because it was assumed that no one would ever want to change
the configuration level default (and have it respected by omitting the
command line option).  Looking at the situation now, It feels like
this *increased* the complexity.

What I think is important here is to keep things as consistent and as
simple as possible.  This means making `--sysinfo` behavior equal to
`--keep-tmp` and similar options.

Wether we decide to use:

 * '--knob=on' or '--knob=off'
 * '--enable-knob' or '--disable-knob'

Is less important to me than having a consistent and predictable
approach (and let's not forget the Job API, because there may be more
obvious paths to follow here).

> > >>     avocado run --loaders LOADERS [LOADERS ...]
> > >>
> > >> is that acceptable?
> > > 
> > > Yes, and I liked this one, it is better than my suggestions.
> > > 
> > 
> > Great, thanks and +1 to this one. Also note there is one caveat, it
> > must not be used for main arguments, otherwise you won't be able to
> > specify the `subcommand`. This is why the `--show` accepts only a
> > single argument and uses custom split (`,`). Something like:
> > 
> >     avocado --show foo bar baz run passtest.py
> > 
> > is not possible, because the argparse can't decide which is the
> > subcommand and which is the `--show` argument. For subcommand options
> > it is possible, because one can use `--` to indicate end of the
> > options:
> > 
> >     avocado run --loaders foo bar -- passtest.py
> > 
> > or simply add another option that consumes given amount of arguments
> > (but it is hackish :-) ):
> > 
> >     avocado run --loaders foo bar --sysinfo on passtest.py
> 
> Noted and I will take this in consideration. Thanks.
> 
> > >>> Presentation
> > >>> ------------
> > >>>
> > >>> As the avocado trend is to have more and more plugins, I believe that to make
> > >>> it easier for the user to find where each configuration is, we should split the
> > >>> file into smaller files, leaving one file for each plugin. Avocado already
> > >>> supports that with the conf.d directory. What do you think?
> > >>>
> > >>
> > >> I'd go with core+core-plugins and plugins, therefor basically the
> > >> current situation. I don't think we need to extract the core-plugins
> > >> from the core-configuration (talking about the essential set of
> > >> plugins like "run").
> > > 
> > > Not sure if I understood your "formula". Could you please provide an
> > > example? And what it is considered "core", in your option?
> > > 
> > 
> > core-plugins are all plugins that are always installed (therefor the
> > `avocado.plugins` namespace). So basically we'd only create separate
> > files for `optional_plugins` (yaml_loader) or the plugins that live
> > outside of our repository (eg. avocado-vt).
> 
> I see. Let's wait for more comments on this.
>

The plugabble aspect of a lot of functionality that I believe Avocado
can not live without (say the plugin that implements the "run"
command) is a consequence of a design that allows other functionality
that Avocado may be able to live without, either provided by us or by
others.  In this sense, there's indeed a soft "core-plugin" concept.

My take on the configuration file being broken down is similar to the
one on the Python modules... if it helps the user experience without
causing hassle (and duplicate boilerplate code) than it should be
done.  I think a quick patch showing the break down of config files
could go a long way towards proving the user experience aspect.

- Cleber.