Re: [libvirt-ci PATCH 06/12] lcitool: Introduce methods to load and validate the YAML config
On Mon, May 11, 2020 at 02:27:13PM +0200, Andrea Bolognani wrote:
On Mon, 2020-05-11 at 12:50 +0200, Erik Skultety wrote:
> On Thu, May 07, 2020 at 06:20:46PM +0200, Andrea Bolognani wrote:
> > On Wed, 2020-05-06 at 14:05 +0200, Erik Skultety wrote:
> > > + self.values = yaml.load(fp, Loader=yaml.Loader)
> >
> > We're use yaml.safe_load(fp) everywhere else. Any reason why we
> > should need the non-safe version? Why is it necessary to explicitly
> > provide it with a Loader?
>
> "YAMLLoadWarning: calling yaml.load() without Loader=... is deprecated"
> ...but this was a completely wrong choice of API on my end, since rather then
> grepping the code base and copy-pasting, I read the documentation and missed
> the safe_load API which also doesn't suffer from the deprecation warning.
Please don't take this specific situation to mean that copying and
pasting is better than reading documentation in the general case O:-)
Of course...now that I'm reading my response back, it does read differently
(and silly too) that what I actually meant, nevermind :).
...
>
> I'm not hardcoding anything. What's actually being done is we're only
verifying
> the user-provided data, so we should not interact with the default values at
> this point yet, so instead, either '' or None should be returned from the
> .get() method.
Maybe I'm missing something, but
flavor = config.get("install").get("flavor", "test")
if flavor not in ["test", "jenkins", "gitlab"]:
raise ValueError(
"Invalid value '{}' for
'install.flavor'".format(flavor)
)
looks to me like you're trying to fetch the flavor configured by
the user and validate it's one of the three accepted values; however,
the second argument of the get() function makes it so that, if the
value is missing in the user's configuration file, you'll get the
default flavor (test) instead.
This is why I say you're hardcoding the default value in the script:
if tomorrow we decided that "gitlab" should be the default flavor, we
would have to change not only the default configuration file but also
the script; with the approach I suggest, changing the former would be
enough.
That is a correct assumption, but at the same time this code should only verify
that we can recognize the user-provided value, so "test" was just a remnant
from the previous revision... not to be confused with actually selecting a
default, that is handled by loading the default config from the repo in a
different part of the code.
That's why I'm saying that in order to clear this confusion, the default from
the get() here should be None, because that way we know that the user didn't
provide any value for flavor (and we need to fill it in later). Therefore,
None should also be part of the list of recognized values, but serve merely as
a sentinel so we don't end up raising an Exception about an invalid config
value.
--
Erik Skultety