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".
> 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.
> >> 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.
> > 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.
> >
>
> For me the important part is to have the correct defaults in `--help`
> and list of all setting in `avocado config`. As for the documentation,
> perhaps we can generate the `avocado config` output on readthedocs.io,
> if you think it's important to keep them somewhere, but the runtime
> options are more important to me.
We are on the same page here.
Regards,
Beraldo
