feat(config): add config command to view and set broker configuration - #33
Conversation
Add `config path`, `config list`, and `config set` subcommands so operators can locate, inspect, and modify the host-side configuration file. Previously the file had only a read path and was never created automatically, so operators had to author it by hand. Extend pkg/config with a Set write path that creates the file and its parent directory when absent, preserves other keys, clears a key on an empty value, and validates values before writing. Share validation and decoding between Load and Set, and keep validation errors path-free so Set does not misleadingly reference the file for a value typed on the CLI. Deduplicate the CLI writer resolution shared with the doctor command.
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a ChangesConfig schema and CLI
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/imds-broker/config.go (1)
86-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeys list can drift from
brokerconfig.Keys.The
Descriptionhardcodes the three key constants instead of deriving frombrokerconfig.Keys(used elsewhere, e.g. inSet's unknown-key error viastrings.Join(Keys, ", ")). If a key is added/removed fromKeys, this usage text will silently go stale.♻️ Suggested fix
- Description: "Valid keys: " + brokerconfig.KeyProfileFilter + ", " + - brokerconfig.KeyRegion + ", " + brokerconfig.KeyLogLevel + - ". An empty value clears the key.", + Description: "Valid keys: " + strings.Join(brokerconfig.Keys, ", ") + + ". An empty value clears the key.",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/imds-broker/config.go` around lines 86 - 88, The usage text in the config description is hardcoding individual brokerconfig key constants, which can drift from the canonical brokerconfig.Keys list used elsewhere. Update the Description construction in the config setup to derive the “Valid keys” text from brokerconfig.Keys (for example by joining the list) so it stays automatically in sync whenever keys change.pkg/config/config.go (1)
192-201: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider an atomic write (temp file + rename) to protect operator config.
os.WriteFiletruncates then writes in place, so an interrupted or failed write can leave a partially written or empty config file—losing the very keysSetworks to preserve. Writing to a temp file in the same directory andos.Rename-ing into place makes the update atomic and avoids clobbering existing content on failure.♻️ Suggested atomic write
if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { return nil, fmt.Errorf("create config dir: %w", err) } - if err := os.WriteFile(path, data, 0o600); err != nil { - return nil, fmt.Errorf("write config %q: %w", path, err) + tmp, err := os.CreateTemp(filepath.Dir(path), ".config-*.yaml.tmp") + if err != nil { + return nil, fmt.Errorf("create temp config: %w", err) + } + tmpName := tmp.Name() + defer os.Remove(tmpName) // no-op after a successful rename + if err := tmp.Chmod(0o600); err != nil { + tmp.Close() + return nil, fmt.Errorf("chmod temp config: %w", err) + } + if _, err := tmp.Write(data); err != nil { + tmp.Close() + return nil, fmt.Errorf("write temp config: %w", err) + } + if err := tmp.Close(); err != nil { + return nil, fmt.Errorf("close temp config: %w", err) + } + if err := os.Rename(tmpName, path); err != nil { + return nil, fmt.Errorf("write config %q: %w", path, err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/config.go` around lines 192 - 201, The config persistence path in Set currently uses os.WriteFile, which can leave a truncated or empty file if the write is interrupted. Update the write flow to use an atomic temp-file write in the same directory followed by os.Rename so the existing config is only replaced on success. Keep the existing error handling around yaml.Marshal and os.MkdirAll, and adjust the final write step in pkg/config/config.go to use a temp file plus rename sequence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/imds-broker/config.go`:
- Around line 86-88: The usage text in the config description is hardcoding
individual brokerconfig key constants, which can drift from the canonical
brokerconfig.Keys list used elsewhere. Update the Description construction in
the config setup to derive the “Valid keys” text from brokerconfig.Keys (for
example by joining the list) so it stays automatically in sync whenever keys
change.
In `@pkg/config/config.go`:
- Around line 192-201: The config persistence path in Set currently uses
os.WriteFile, which can leave a truncated or empty file if the write is
interrupted. Update the write flow to use an atomic temp-file write in the same
directory followed by os.Rename so the existing config is only replaced on
success. Keep the existing error handling around yaml.Marshal and os.MkdirAll,
and adjust the final write step in pkg/config/config.go to use a temp file plus
rename sequence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e6dac31-036d-4ddb-a45a-a1cb261b3278
📒 Files selected for processing (6)
cmd/imds-broker/config.gocmd/imds-broker/doctor.gocmd/imds-broker/main.gocmd/imds-broker/main_test.gopkg/config/config.gopkg/config/config_test.go
jamestelfer
changed the title
Build the `config set` valid-keys usage text from brokerconfig.Keys so it cannot drift from the canonical list. Replace the direct os.WriteFile in Set with a temp-file write plus rename so an interrupted write cannot truncate the operator's existing config.
1 participant
Purpose
Operators had no supported way to discover or change the central broker configuration. The config file was never created automatically and had only a read path, so the only options were to guess its location and hand-author YAML. That is error-prone: a typo in the profile-filter regex or an invalid log level was only surfaced later, at broker startup.
This adds a first-class
configcommand so an operator can locate the file, inspect the current values, and set or clear them safely. Values are validated before they are written, turning a class of latent startup failures into immediate, actionable feedback at the point of change.Context
config setwrites a host path and is an operator interface, not agent-reachable; the defence is the sandbox keeping the file and launch inputs out of the agent's reach, consistent with the project threat model.setround-trips through the schema, so hand-written YAML comments are not preserved. Accepted trade-off for a three-key config.