On Mon, May 11, 2020 at 03:27:37PM +0200, Erik Skultety wrote:
> 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
Actually, None cannot be part of the list of recognized values, because it will
mess with the _update function and populate an invalid setting to the playbook.
So, I suggest we mandate that when an option is specified, it cannot be empty.
By doing that, the dictionary update() works like expected and we don't have
to mess with further dictionary comprehensions to filter out keys with empty
values for which we're able to provide a default.
--
Erik Skultety
