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:-)
> > > + if config.get(section, None) is None:
> > > + raise KeyError("Section '{}' not found".format(section))
> >
> > This will raise the same error message both when the section is
> > actually missing and when it's present but doesn't contain anything,
> > so the error handling should be
> >
> > raise Exception("Missing or empty section '{}'.format(section))
> >
> > Note the use of Exception instead of KeyError - again, if you want
> > to change the way we do error handling by all means post patches
> > towards that goal, but until then please stick with the current
> > approach.
> >
> > As for the check itself, I think I would like it to look like
> >
> > if section not in config or config[section] is None:
>
> Well, if it was only about key existence, the first part of your boolean
> predicate would be preferable, but once you need to check both existence and
> value, .get() is the recommended way.
>
> > I'm no Pythonista, but this it seems to follow the mantra "explicit
>
> As it turned out, I even was explicit :P since 'None' is the default value
> returned on key non-existence
Okay, I'm fine using your version as long as you update the error
message as described above.
> > > + # we need to check flavor here, because later validations depend on it
> > > + flavor = config.get("install").get("flavor", "test")
> >
> > This hardcodes the default value for flavor in the script instead of
> > reading it from the default configuration file. It should be
> >
> > .get("flavor", self.values["install"]["flavor"])
>
> 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.
--
Andrea Bolognani / Red Hat / Virtualization
