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
>
signature.asc
Description: OpenPGP digital signature
