edmundmok / mealpy

Order your meals on MealPal automatically!
MIT License
28 stars 22 forks source link

Config customization doesn't work #23

Open ipwnponies opened 5 years ago

ipwnponies commented 5 years ago

We want the config feature to read a default config and then apply user customizations, similar to Hiera for Puppet. The config template is shipped with code and not intended to be modified, it serves as a template for users to copy and see all possible values. The the user's config is applied on top: https://github.com/edmundmok/mealpy/blob/a580ab524e95d8e37a0f7484d34add21f842bf41/mealpy/mealpy.py#L55

The bug is that this uses load_config_from_file, which does shema validation, and will fail due to missing values. What we probably want to do instead is naively update user's values and then rerun validation (or not, extra values should be ignored).

We could also have a minimal schema set for values the user must set. e.g. API tokens. But there's no use case for this yet, just dropping the thought for future reference.

edmundmok commented 5 years ago

Oh yes, I didn't encounter this because my personal config was always a copy of the template, thanks for noticing this! strictyaml seems to have optional keys with defaults, which we can use to replace the template file if we want to go that route. That might require a more helpful README file so that users don't have to dig into the code to understand what config options exist.

ipwnponies commented 5 years ago

We would need a second schema, with all optional keys. Being able to have a strict schema, with sane defaults, means the code can assume values exist. Otherwise we'll end up with a mess of .get('DEFAULT') in the code, and it'll be difficult to document default values for the user.

I'm not sure how I feel about using optional keys feature of strictyaml and schema validation features in general. it feels clunky to try to read the yaml schema via python code. Having a complete config template is a great way to self-document. The instructions could be yaml comments right in the config itself.

edmundmok commented 5 years ago

We can specify the defaults within the schema for optional keys, so there won't be any issue about .get and having to specify and defaults. As for readability, I think it would be possible to create our own "dictionary" containing values as tuple of (type, default) instead of using the strictyaml.Map directly, and just pass our custom dictionary into a helper function to generate the less readable schema. Documentation is a good point though, but it's possible that users will go to the README anyway to learn how to use the tool, so it might also be a good idea to centralize all documentation in the README.

ipwnponies commented 5 years ago

I feel using optionals is a flawed approach. It complicates the schema, as a workaround. The schema should remain unchanged, as it's technically correct. Optionals makes it unclear to any developer wihtout tribal knowledge. We also don't want to maintain multiple schemas: one for the canonical schema; and the other for the "all-optionals".

I think we want to use revalidate, haven't looked at it closely though. What I'm envisioning is:

  1. validate the template, which should always be valid
  2. override/merge values from user config
  3. revalidate the final config, ensuring it's still valid.

That way, there should be less custom/duplicate code. It's just plain python assignments, and then reusing the same strictyaml validation + schema setup.

edmundmok commented 5 years ago

There will still only be one schema in my suggestion, except they are all optionals, which come with default values, which mirrors the idea of our "template config".

I think using revalidate will instead use a separate schema since strictyaml needs to know how to read from the custom config.

ipwnponies commented 5 years ago

The schema with default values would work but the downside (imo quite significant) is that users cannot easily read the default values. Instead of reading a "config.template.yaml", they would need to read a strictyaml schema.

strictyaml can read a yaml file without schema.