[Avocado-devel] RFC: Configuration by convention

[Beraldo Leal]

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.

> > There is no convention on the naming pattern used either on configuration files
> > or on command-line options. Besides the name convention, there is also a lack
> > of convention for some argument types. For instance::
> > 
> >  $ avocado run -d
> > 
> > and::
> > 
> >  $ avocado run --sysinfo on
> > 
> > Both are boolean variables, but with different "execution model" (the former
> > doesn't need arguments and the latter needs `on` or `off` as argument).
> > 
> 
> Actually we do follow the pattern for booleans. The "--sysinfo" is a tri-state.

I tried to see the third state but I might be missing something:

--
  $ grep "\-\-sysinfo" *  -R
  plugins/run.py: parser.add_argument('--sysinfo', choices=('on', 'off'),
--

Also from `avocado run --help`:

--
  --sysinfo {on,off}   Enable or disable system information (hardware
                       details, profilers, etc.). Current: on
--

And here "Current" should be "Default".

Could you please, point me to the third state?

Keep in mind that I used `--sysinfo` just as an example, there are few
other cases: `--keep-tmp`, `--ignore-missing-references` and
`--failfast`.


> > Besides the basic ones, here are some recommendations we should try to follow
> > and pay attention to:
> > 
> >   1. Option-arguments should not be optional (Guideline 7, from POSIX). So we
> >      should avoid this::
> >      
> >         avocado run --loaders [LOADERS [LOADERS ...]]
> > 
> 
> Well you might want to specify no loaders (to override the default),
> although the only usecase I see is self-testing. But how about:
> 
>     avocado run --loaders LOADERS [LOADERS ...]
> 
> is that acceptable?

Yes, and I liked this one, it is better than my suggestions.

> >   2. **Lists**: When an option argument has multiple values we should use the
> >        space as the separator.
> > 
> 
> This basically means:
> 
>     avocado run --loaders LOADERS [LOADERS ...]
> 
> right?

Yes, you are right.

> > Presentation
> > ~~~~~~~~~~~~
> > 
> > Finding options easily, either in the manual or in the help, favor usability
> > and avoids chaos.
> > 
> > We can arrange the display of these options in alphabetical order within each
> > section.
> > 
> 
> I'd love to (more-less), but sometimes people forget. It's hard to
> enforce this. Also there are exceptions where we want to make some
> options more visible, but in majority cases it should be A-Z.

I think that if we have the options in alphabetical order, this will be
used as an example and new options will tend to be in order too.

Also, If we agree with this suggestion and the general idea of this RFC,
I would suggest storing this RFC on something more concrete/permanent,
such as a "blueprint" and keeping in one of ours repositories, linking
to our dev/contributor guide.

If, during the PR, the new argument option it is not in the order that
we expect, we can suggest this small change. But I understand that this
should be a common agreement.

I'm pointing this here because, for a new developer and/or user, it
might be "painful" to find the right option without any logical order.

> > Standards for Config File Interface
> > -----------------------------------
> > 
> > .. note:: Many other config file options could be used here, but since that
> >           this is another discussion, I'm assuming that we are going to keep
> >           using `configparser` for a while.
> > 
> > As one of the main motivations of this RFC is to create a convention to avoid
> > chaos and make the job execution API use as straightforward as possible, I
> > believe that the config file should be as close as possible to the dictionary
> > that will be passed to this API.
> > 
> > For this reason, this may be the most critical point of this RFC. We should
> > create a pattern that is intuitive for the developer to convert from one format
> > to another without much juggling.
> > 
> > Nested Sections
> > ~~~~~~~~~~~~~~~
> > 
> > While the current `configparser` library does not support nested sections,
> > Avocado can use the dot character as a convention for that. i.e:
> > `[runner.output]`.
> > 
> > This convention will be important soon, when converting a dictionary into a
> > config file and vice-versa.
> > 
> 
> This is the only mentioning of args->config mapping. Can you please
> elaborate a bit more?

Sorry if I was not clear, maybe I got your point now. But it is
straightforward:

All default values (in source code) are overridden by config file
options, and then by the argument options (in this order). Probably we
will not have 1:1 mapping of all config file options to argument
options. So far I understood, the command-line argument options are the
basic ones and the most used.

> > And since almost everything in Avocado is a plugin, each plugin section should
> > **not** use the "plugins" prefix and **must** respect the reserved sections
> > mentioned before. Currently, we have a mix of sections that start with
> > "plugins" and sections that don't.
> > 
> 
> So basically
> 
> [vt]
> 
> vt-related-option
> 
> [vt.generic]
> 
> generic-vt-related-option
> 
> [runner]
> 
> runner-related-option
> 
> 
> yes, the plugins section seems redundant as many parts are actually
> implemented as plugins.

Yes, you got the point.


> > Plugin section name
> > ~~~~~~~~~~~~~~~~~~~
> > 
> > I am not quite sure here and would like to know the opinion of those who are
> > the longest in the project. Perhaps this is a little controversial point. But I
> > believe we can touch here to improve our convention.
> > 
> > Most plugins currently have the same name as the python module. Example: human,
> > diff, tap, nrun, run, journal, replay, sysinfo, etc.
> > 
> > These are examples of "good" names.
> > 
> > However, some other plugins do not follow this convention. Ex: runnable_run,
> > runnable_run_recipe, task_run, task_run_recipe, archive, etc.
> > 
> > I believe that having a convention here helps when writing more complex tests,
> > configfiles, as well as easily finding plugins in various parts of the project,
> > either on a manual page or during the installation procedure.
> > 
> > I understand that the name of the plugin is different from the module name in
> > python, but anyway, should we follow PEP8 in this case?
> > 
> >         From PEP8: Modules should have short, all-lowercase names. Underscores
> >         can be used in the module name if it improves readability. Python
> >         packages should also have short, all-lowercase names, although the use
> >         of underscores is discouraged.
> > 
> 
> I'm not sure I understand properly this section. Can you please
> elaborate a bit more? Is the "_" -> "-" the problem you want to
> avoid?.

Let's get the `human` example:

Python module name: human
Plugin name: human

Let's get the `task_run_recipe` example:

Python module name: task_run_recipe
Plugin name: task-run-recipe

Let's get another example:

Python module name: archive
Plugin name: zip_archive

It is not a *big problem* just a convention in order to avoid chaos in
the near future. It is a bit more clear now?

> > Regarding boolean values, `getboolean()` can accept `yes/no`, `on/off`,
> > `true/false` or `1/0`. But we should adopt one style and stick with it. I
> > would suggest using `true/false`.
> > 
> 
> I'd encourage people to use one, but we should attempt to accept all.

Yes, I agree. And the good news is that `getboolean()` already support
all options.


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

> > Security Implications
> > #####################
> > 
> > Avocado users should have the warranty that their jobs are running on isolated
> > environment.
> > 
> > We should consider this and keep in mind that any moves here should continue
> > with this assumption.
> > 
> 
> I don't really understand this section, can you expand it (or remove
> it, if not necessary?)

Maybe we can remove it. It is just a disclaimer in case someone brings
a completely new approach.

> > How to Teach This
> > #################
> > 
> > We should provide a complete configuration reference guide section
> > in ourThis section doesn't seem important to me.  User's
> > Documentation.
> > 
> > In the future, the Job API should also be very well detailed so sphinx could
> > generate good documentation on our Test Writer's Guide.
> > 
> > Besides a good documentation, there is no better way to learn than by example.
> > If our plugins, options and settings follow a good convention it will serve as
> > template to new plugins.
> > 
> > If these changes are accepted by the community and implemented, this RFC could
> > be adapted to become a section on one of our guides, maybe something like the a
> > Python PEP that should be followed when developing new plugins.
> > 
> 
> IIUC this section just says we should keep thinks as they are, right?
> If so than it doesn't need to be here, does it? Maybe one thing to add
> is that this basically follows the job API, right? And it's outcome
> should be well defined Job "args", right? Then the outcome of this
> should be a written standard of mandatory and optional sections of the
> "args" and all the relations between "args" and "options". Written,
> stable and flexible enough to suite all extra plugins needs.

I'm sorry it was not clear. I must have expressed myself wrong. This is
not about the Job API yet. Just a preparation for that.

This RFC is about a convention for our configuration options:

  1. "implicit" (default values);
  2. "more permanent" (config files);
  3. "temporary/runtime" (argument options)

This section is about how to teach this new convention to new users. I
could not find a detailed configuration reference on our docs, with all
default options, for instance.

It was not my intention to discuss the future job API on this RFC. This
will be discussed on a different RFC. And I'm already taking some notes
to do this soon.

> Thank you, Beraldo, for opening this discussion.
> Lukáš


Thank you for spending some time on this, I appreciate.

Regards,
Beraldo