On Mon, 2020-05-11 at 15:55 +0200, Erik Skultety wrote:
> 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:
> > > 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.
Sure, expecting the user to provide actual values in the
configuration file is an entirely acceptable constraint :)
I'm not entirely sure how you plan to reconcile that with the fact
that some keys are optional, and user_config.get("key") will return
None both when "key" was set to an empty value and when it was not
present at all... But I'm sure you have something in mind :)
--
Andrea Bolognani / Red Hat / Virtualization
