Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Config customization doesn't work #23

Open
ipwnponies opened this issue Apr 21, 2019 · 6 comments
Open

Config customization doesn't work #23

ipwnponies opened this issue Apr 21, 2019 · 6 comments
Labels

Comments

@ipwnponies
Copy link
Collaborator

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:

config.update(load_config_from_file(config_path, schema))

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.

@ipwnponies ipwnponies added the bug label Apr 21, 2019
@edmundmok
Copy link
Owner

edmundmok commented Apr 21, 2019

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
Copy link
Collaborator Author

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
Copy link
Owner

edmundmok commented Apr 21, 2019

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
Copy link
Collaborator Author

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
Copy link
Owner

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
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants