On Wed, 2020-05-06 at 14:05 +0200, Erik Skultety wrote:
> class Config:
>
> + def __init__(self):
> +
> + # Load the template config containing the defaults first, this must
> + # always succeed.
> + # NOTE: we should load this from /usr/share once we start packaging
> + # lcitool
> + base = Util.get_base()
> + with open(os.path.join(base, "config.yaml"), "r") as fp:
> + 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?
> + try:
> + with open(self._get_config_file("config.yaml"), "r") as fp:
> + user_config = yaml.load(fp, Loader=yaml.Loader)
> + except Exception as e:
> + print("Missing or invalid config.yaml file: {}".format(e),
> + file=sys.stderr)
> + sys.exit(1)
As mentioned last time, this is not the correct place or way to
handle this exception. I know you don't like how we process failures
right now :) but the way to address that is to post a patch that
eradicates the current implementation and replaces it with a better
one in one fell swoop, not to introduce alternative ways to do error
handling in a few select places.
I have posted a patch[1] that implements the suggestion I gave last
time, and that allows you to simply raise an Exception here.
> + # Validate the user provided config and use it to override the default
> + # settings
> + self._validate(user_config)
> + self._update(user_config)
Incidentally, since these two functions can also fail, with your
approach you'd have reported a nice error message if the
configuration file is missing altogether but *not* if it exists but
is invalid.
> + @staticmethod
> + def _validate_section(config, section, keys):
> +
> + 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:
I'm no Pythonista, but this it seems to follow the mantra "explicit
is better than implicit" more closely... You've written more Python
than me, though, so if you think using get() is superior I don't have
a problem leaving the code as is :)
> + for key in keys:
> + if config.get(section).get(key, None) is None:
> + raise KeyError("Missing mandatory key '{}.{}'".format(section,
> + key))
The very same considerations apply here.
One additional thing: since YAML is quite flexible, I think it would
be sensible (if a little paranoid) to also have something like
if not (instanceof(config[section][key], str) or
instanceof(config[section][key], int)):
raise Exception(
"Invalid type for key '{}.{}'".format(section, key)
)
otherwise we'd happily accept something like
---
install:
root_password:
- let
- me
- in
as valid.
> + @staticmethod
> + def _validate(config):
The first thing you need to check here is whether config is not None,
otherwise an empty configuration file will result in
Traceback (most recent call last):
File "./lcitool", line 1090, in <module>
Application().run()
File "./lcitool", line 452, in __init__
self._config = Config()
File "./lcitool", line 156, in __init__
self._validate(user_config)
File "./lcitool", line 195, in _validate
Config._validate_section(config, "install", ["root_password"])
File "./lcitool", line 183, in _validate_section
if config.get(section, None) is None:
AttributeError: 'NoneType' object has no attribute 'get'
> + # verify the [install] section and its mandatory options
> + Config._validate_section(config, "install", ["root_password"])
> +
> + # 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"])
[1] https://www.redhat.com/archives/libvir-list/2020-May/msg00315.html
--
Andrea Bolognani / Red Hat / Virtualization
