feat(gateway): add config set overrides#2350
Conversation
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
🌿 Preview your docs: https://nvidia-preview-pr-2350.docs.buildwithfern.com/openshell |
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
| } | ||
| Ok(component.to_string()) | ||
| }) | ||
| .collect::<std::result::Result<Vec<_>, _>>()?; |
There was a problem hiding this comment.
I think it makes sense to only support flat keys, i.e. without arrays (e.g. foo.bar[0].baz).
We don't have many things that use arrays, but now have interceptors and middlewares that are arrays. So editing these would be a manual config edit still.
|
|
||
| #[derive(Debug, Subcommand)] | ||
| enum ConfigCommand { | ||
| /// Update fields in the gateway TOML configuration. |
There was a problem hiding this comment.
Worth mentioning the config is updated in place? I think the name set is quite clear already, but maybe worth making sure?
I'm wondering if there is anything that could go wrong, e.g. someone overrides something that is hard to recover? Looking at other tools, it doesn't seem a concern they deal with.
|
|
||
| #[derive(Debug, Args)] | ||
| pub struct ConfigArgs { | ||
| #[command(subcommand)] |
There was a problem hiding this comment.
Do we need to add some properties here, e.g. the help_template, after_help, etc. to keep the help output consistent?
|
|
||
| #[derive(Clone, Debug)] | ||
| struct Assignment { | ||
| key: Vec<String>, |
There was a problem hiding this comment.
Probably a nit, but in the toml context, I would argue that key is the string representation root.table.table2.setting, whereas the path is the Vec representation (split by .).
| } | ||
| } | ||
|
|
||
| fn parse_assignment_value(raw: &str) -> Item { |
There was a problem hiding this comment.
Could we add focused tests for RHS parsing and use toml_edit’s value parser directly here?
Proposed parser shape:
fn parse_assignment_value(raw: &str) -> Result<Item, String> {
if let Ok(item) = raw.parse::<Item>() {
return Ok(item);
}
if raw.contains(['\n', '\r']) {
return Err("invalid assignment value: expected a single TOML value or an unquoted string".into());
}
Ok(value(raw))
}Proposed test cases:
openshell.gateway.log_level="""debug\ntrace"""is accepted as a multiline basic string.openshell.gateway.log_level='''debug\\n\ntrace'''is accepted as a multiline literal string.openshell.gateway.log_level=42\nother = trueis rejected as an invalid value fragment.openshell.gateway.log_level=42\notheris rejected as a multiline unquoted value.
That would pin the intended boundary between “single TOML value”, “single-line bare string fallback”, and invalid multiline input, while avoiding the current fake-document parse.
| .ok_or_else(|| miette::miette!("gateway config key '{key}' must be a TOML table")) | ||
| } | ||
|
|
||
| fn write_atomically(path: &Path, contents: &[u8]) -> Result<()> { |
There was a problem hiding this comment.
This follows the same basic tempfile + persist pattern we already use elsewhere, but we now have a couple of local atomic file writers. Could we either reuse/extract a small shared helper, or add tests here for the metadata we care about? In particular, the current implementation preserves mode bits but not uid/gid on Unix, which can matter for managed config files.
| format!("invalid assignment '{input}': expected a dotted KEY=VALUE argument") | ||
| })?; | ||
|
|
||
| let key = raw_key |
There was a problem hiding this comment.
Could we use toml_edit::Key::parse(raw_key) for the assignment path instead of splitting on . manually? The current parser only supports bare key components, and it will mis-split valid TOML quoted keys such as:
openshell.drivers."containerd.io".socket_path=/run/containerd/containerd.sock
With raw_key.split('.'), that becomes "containerd and io" as separate components, so there is no way to set driver tables or future config keys whose TOML key contains a dot. toml_edit::Key::parse already understands TOML dotted-key syntax and would keep this aligned with the value parsing side.
| Ok(()) | ||
| } | ||
|
|
||
| fn set(path: &Path, settings: &SetArgs) -> Result<()> { |
There was a problem hiding this comment.
nit: In the context of Go, I've often found that splitting something like this into function working on different types (Path -> io.Writer / io.Reader -> Config object) make things simpler to test without needing to go through the file system.
| Ok(()) | ||
| } | ||
|
|
||
| fn set(path: &Path, settings: &SetArgs) -> Result<()> { |
There was a problem hiding this comment.
Question: Can set be a method (possibly apply?) on the SetArgs type? At the very least, does it make sense to reorder the arguments to make it clear that path is the output?
| let mut document = if original.trim().is_empty() { | ||
| DocumentMut::new() | ||
| } else { | ||
| original | ||
| .parse::<DocumentMut>() | ||
| .into_diagnostic() | ||
| .wrap_err_with(|| format!("failed to parse gateway config '{}'", path.display()))? | ||
| }; | ||
|
|
||
| let openshell = ensure_table(document.as_table_mut(), "openshell")?; | ||
| if !openshell.contains_key("version") { | ||
| openshell.insert("version", value(i64::from(config_file::SCHEMA_VERSION))); | ||
| } |
There was a problem hiding this comment.
This seems like it should be common config handling (i.e. config.load()). How does OpenShell currently handle a config file that doesn't exist? Should this not generate a default config, or is this only done on parsing?
| .unwrap_or_else(|| value(raw)) | ||
| } | ||
|
|
||
| fn apply_assignment(document: &mut DocumentMut, assignment: &Assignment) -> Result<()> { |
| } | ||
|
|
||
| let rendered = document.to_string(); | ||
| config_file::parse(&rendered, path).map_err(|err| miette::miette!("{err}"))?; |
There was a problem hiding this comment.
Could we add coverage for setting a field inside a nested required table on a missing config file, and either support or document the expected behavior? For example:
openshell-gateway config set openshell.gateway.tls.cert_path=/tmp/cert.pem
starts from an empty document when the config file does not exist, then creates [openshell.gateway.tls] with only cert_path. Since TlsConfig also requires key_path, final schema validation should fail before writing. That may be the right behavior, but it would be good to pin it with a test and clarify that creating nested config objects from scratch may require setting all required sibling fields in the same invocation.
3 participants
Summary
Add a generic, atomic
openshell-gateway config setcommand and expose repeatable configuration overrides through the local Docker gateway task.Related Issue
N/A — extracted from the config-management work in #2137 following the engineering discussion.
Changes
KEY=VALUEassignments for gateway TOML updatesmise run gateway:docker -- --set KEY=VALUEpassthrough after generated config creationTesting
mise run pre-commitpassesChecklist