On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> + def __init__(self):
> + path = self._get_config_file("config.toml")
> +
> + try:
> + self.dict = toml.load(path)
I'm not too keen on the name "dict". What about "values"?
> + self._validate()
> +
> + except Exception as ex:
> + raise Exception(
> + "Missing or invalid config.toml file: {}".format(ex)
> + )
This will cause lcitool to blow up pretty spectacularly if you
haven't created a configuration file already:
$ ./lcitool update all all
Traceback (most recent call last):
File "./lcitool", line 141, in __init__
self.dict = toml.load(path)
File "/usr/lib/python3.6/site-packages/toml.py", line 87, in load
with io.open(f, encoding='utf-8') as ffile:
FileNotFoundError: [Errno 2] No such file or directory: '/home/abologna/.config/lcitool/config.toml'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "./lcitool", line 1043, in <module>
Application().run()
File "./lcitool", line 395, in __init__
self._config = Config()
File "./lcitool", line 146, in __init__
"Missing or invalid config.toml file: {}".format(ex)
Exception: Missing or invalid config.toml file: [Errno 2] No such file or directory: '/home/abologna/.config/lcitool/config.toml'
This could technically happen already, but that would have been as
the result of invalid data being committed to the repository, so it
never showed up in practice.
You'll need to refactor the script so that exceptions can be caught
and pretty-printed even when they occur outside of Application.run().
> + # fill in default options
> + self._fill_default_options(self.dict)
You don't need to pass the dictionary as argument - it's already
part of self at this point.
> + def _fill_default_options(cfg):
> + flavor = cfg.get("install").get("flavor", "test")
> + cfg["install"]["flavor"] = flavor
> +
> + if flavor == "gitlab":
> + url = cfg.get("gitlab").get("gitlab_url", "https://gitlab.com";)
> + cfg["gitlab"]["gitlab_url"] = url
It would be neat if, instead of hardcoding our defaults in the
script, we did something like
cfg.get("gitlab").get("gitlab_url", default_gitlab_url)
where default_gitlab_url is obtained from the inventory.
Alternatively, and this is probably a cleaner approach, we could make
it so lcitool reads config.toml from the source directory, which
would be uncommented, first, and then the one in the user's home
directory, adding / overriding only those keys that are found: this
would ensure, among other things, that the comments in the sample
configuration file never get out of sync with the actual defaults,
and also I think the amount of boring code necessary in patch 11.
> + @staticmethod
> + def _validate_section(cfg, section, *args):
I don't like *args very much, and would prefer if this was just a
regular list.
> + if cfg.get(section, None) is None:
> + raise KeyError("Section '[{}]' not found".format(section))
No need for square brackets around the name, you already mentioned
that it's a section we're talking about.
--
Andrea Bolognani / Red Hat / Virtualization
