Re: [libvirt-ci PATCH 06/12] lcitool: Introduce methods to load and validate the YAML config

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.