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

Add support for config files in TOML and YAML formats #73

Merged

Conversation

phlogistonjohn
Copy link
Collaborator

@phlogistonjohn phlogistonjohn commented Apr 6, 2023

Depends on: #69

JSON is great in that it is standardized, tons of languages support it, and it's pretty simple to parse and work with. It is pretty terrible to edit by hand though. This PR adds support for both TOML and YAML as formats for our sambacc configuration. The intent here is to make writing a config by hand simpler and thus using samba-container directly without management by the operator, for example, easier.

TOML is probably the nicest to work with once you memorize the rules around brackets (tables vs. arrays of tables). This module is included in python, however, the python version must be 3.11 or later.

YAML is a superset of JSON that is much easier to write and is extremely popular, if a bit over-complicated.

The file format to use is selected based on the file name's extension. To use TOML the file name must end with ".toml". To use YAML the file name must end with either ".yml" or ".yaml".

These changes build upon the work to use jsonschema. Despite the name the jsonschema can be applied to both these formats as it's really describing the underlying strucutre of the data (not the formatting) and both new formats are largely json-compatible, with similar structural and basic data types.

TODO:

  • Actually test it in a built samba container
  • Document the new formats with examples and descriptions
  • Update test deps to run (not skip) the newly added unit tests

@phlogistonjohn phlogistonjohn self-assigned this Apr 6, 2023
@dpulls
Copy link

dpulls bot commented Apr 6, 2023

🎉 All dependencies have been resolved !

@mergify
Copy link

mergify bot commented Apr 6, 2023

This pull request now has conflicts with the target branch. Please resolve these conflicts and force push the updated branch.

@mergify
Copy link

mergify bot commented Apr 21, 2023

This pull request now has conflicts with the target branch. Please resolve these conflicts and force push the updated branch.

Python 3.11 and later includes support for reading TOML files.
Add support for reading TOML based configuration to sambacc.

Signed-off-by: John Mulligan <[email protected]>
The YAML format is extremely popular and much easier to write by hand
than JSON.  Add support for YAML based configuration files to sambacc.

Signed-off-by: John Mulligan <[email protected]>
In versions of python prior to 3.11, we can fall back to the
tomli library for toml support.

Signed-off-by: John Mulligan <[email protected]>
Skip the toml related test cases only if neither tomli or tomllib
are available.

Signed-off-by: John Mulligan <[email protected]>
The toml extra declares we optionally depend on tomli on python versions
older than Python 3.11.
The yaml extra declares we optionally depend on PyYAML.

Signed-off-by: John Mulligan <[email protected]>
I'm confident that the python packages available on fedora 37, and
presumably later versions, support all the deps we need for all our
extras. Thus this change adds RPM Recommends for our extras but only for
fedora 37 and later.

Signed-off-by: John Mulligan <[email protected]>
Mention that sambacc support yaml and toml and give brief examples
of both.

Signed-off-by: John Mulligan <[email protected]>
@phlogistonjohn phlogistonjohn changed the title [WIP] add support for config files in TOML and YAML formats Add support for config files in TOML and YAML formats Apr 24, 2023
@phlogistonjohn phlogistonjohn marked this pull request as ready for review April 24, 2023 20:19
Copy link
Collaborator

@synarete synarete left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines 86 to 88
if fname.endswith(".toml"):
return ConfigFormat.TOML
return ConfigFormat.JSON
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but I would prefer using splitext and compare with any of valid enum values of ConfigFormat. Just an idea.

Copy link
Contributor

@obnoxxx obnoxxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks! good to move away from json as the only config format imho.

@mergify mergify bot merged commit e7850e0 into samba-in-kubernetes:master Apr 25, 2023
@phlogistonjohn phlogistonjohn deleted the jjm-more-config-fmts branch April 25, 2023 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants