Remove toml assumption #2665
Remove toml assumption #2665
Conversation
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
There was a problem hiding this comment.
@kate-goldenring this implementation is somewhat generic but still makes assumptions that are likely specific to Spin CLI (e.g.,toml). The core factor implementation is now very generic and makes very little assumptions about the embedding runtime. It would be nice if we kept generic code and runtime specific code separate for clarity.
Right now in sqlite, we have the runtime_config module which contains generic runtime_config related code, but then there is a spin submodule which implements the Spin CLI specific runtime config scheme.
In my head, eventually the factor-sqlite crate would only contain the highly generic, runtime agnostic code and there would be another crate factor-sqlite-spin-cli (or similar) that takes a dependency on factor-sqlite and does all of the Spin CLI specific stuff.
Anyway, wanted to flag this as a thought I'm having so we can works towards a solution in the medium term.
There was a problem hiding this comment.
Following that model, this file really is the Spin CLI implementation, so we could move it to runtime_config/spin.rs. Other runtimes may also choose to use this resolver as well, but it is not a given
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
| StoreConfig { | ||
| type_: SpinKeyValueStore::RUNTIME_CONFIG_TYPE.to_string(), | ||
| config: toml::value::Table::try_from(default_config)?, | ||
| }, |
There was a problem hiding this comment.
Would probably benefit from a StoreConfig::new 🤷
There was a problem hiding this comment.
Good point. I'll do this in a follow up.
Currently many factors assume that their runtime configs are serialized as toml. This hurts reusability as this assumption might not be true for many runtimes.
This PR removes this assumption (for sqlite and kv) instead making the implementations generic on their runtime configurations. While this does add a generic to the code, the underlying factor code already was generic on runtime config - it just made the assumption that that generic config was serialized as toml. Now it knows nothing about the config other than it implements
DeserializeOwned.You can notice that the shape of how runtime config is serialized is moved to Spin CLI specific code while the core Factor impl does not care at all about this.
Note: eventually I would like to remove the assumption that runtime config is serialized at all, but that's a bigger change that requires more controversial decisions. This is still a step towards that direction while still being a good decision IMO even if we keep deserialization assumptions.