Enables toml configs #1320
Enables toml configs #1320
Conversation
There was a problem hiding this comment.
Thanks for creating this PR @AhmedSoliman. The changes look good to me. My only question is about which configuration format we want to support long term and in which format are we putting our example configurations in our docs.
| Some(ext) if ext == "yaml" || ext == "yml" => { | ||
| figment.merge(Yaml::file_exact(config_file)) | ||
| } | ||
| Some(ext) if ext == "toml" || ext == "tml" => { | ||
| figment.merge(Toml::file_exact(config_file)) | ||
| } |
There was a problem hiding this comment.
Are we going to support multiple configuration formats for a longer period? I am wondering whether our users will enjoy the freedom to choose a different format or whether it is confusing if different examples use different configuration formats. I guess my question is should we have a single configuration format?
There was a problem hiding this comment.
I don't think we need two formats, but I'd like us to standardize on toml. If you don't have objections to using toml, then once I finish the transition to the new config structure, we can remove Yaml support.
There was a problem hiding this comment.
If it's easy enough, I would like to retain json/yaml. This allows to easily integrate with Kube (e.g. think the operator CRD could copy paste the yaml configuration from the Custom Resource to the configuration file).
There was a problem hiding this comment.
Also when you change this, please update the e2e code to use toml rather than json for serializing the configuration.
There was a problem hiding this comment.
I don't have a strong preference for either format but I have a preference for one official format if possible. Toml seems to be easier to handle/understand compared to yaml from what I have read in some discussions. So I would be fine with it. Maybe let's also check with the cloud team whether they have some concerns about moving away from yaml.
Regarding the Kube scenario, do you envision that one can configure a Restate cluster via specifying some yaml in the operator CRD? Couldn't one also put some toml snippet (e.g. as a json string) that gets copied over?
There was a problem hiding this comment.
I'll check with the cloud/kube team.
There was a problem hiding this comment.
In all cases, we need this as a transitional step anyway, a followup PR can address removing yaml support if consensus is reached on this.
There was a problem hiding this comment.
For posterity, no issues about moving completely to TOML were raised from the cloud team.
- Use file_exact because file searches parent directories automatically for the config, definitely not the behaviour we want. - If config file was not found, it silently ignored the supplied --config-file argument, this was fixed - Support loading toml files - Makes it clear in logs when we are starting with the built-in default config.
Enables toml configs
Stack created with Sapling. Best reviewed with ReviewStack.