-
Notifications
You must be signed in to change notification settings - Fork 2
Add CLI commands for config management #161
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
base: main
Are you sure you want to change the base?
Conversation
Add new 'sequoia config' subcommands to read and modify configuration values: - `config get <key>` - retrieves a config value by key (supports dot notation for nested values) - `config set <key> <value>` - sets a config value and writes to config.yaml - `config show` - displays the entire configuration Examples: sequoia config get queue_interval sequoia config set queue_interval 50 sequoia config get api_config.port sequoia config set api_config.port 8080
📝 WalkthroughWalkthroughAdds a new config CLI subcommand (cmd/config/config.go) providing Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)cmd/config/config.go (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (8)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cmd/config/config.gocmd/root.go
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/root.go (1)
cmd/config/config.go (1)
ConfigCmd(17-26)
cmd/config/config.go (3)
cmd/types/flags.go (1)
FlagHome(4-4)config/files.go (2)
Init(76-107)ConfigFileName(15-15)config/types.go (1)
Config(22-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build
- GitHub Check: cov
- GitHub Check: Test
- GitHub Check: lint
🔇 Additional comments (5)
cmd/root.go (1)
9-9: LGTM!The import alias
configcmdappropriately avoids conflict with the existingconfigpackage import, and the new subcommand is wired consistently with other subcommands likewallet.WalletCmd()anddatabase.DataCmd().Also applies to: 179-179
cmd/config/config.go (4)
16-26: LGTM!Clean implementation of the parent command following standard Cobra patterns.
28-53: LGTM!Proper error handling and reuse of existing
Export()method for serialization.
55-89: LGTM!Well-documented command with helpful examples and proper argument validation via
cobra.ExactArgs(1).
219-258: LGTM!Robust implementation that correctly handles pointer dereferencing, parses tag options (e.g.,
omitempty), and provides multiple lookup strategies (yaml tag, mapstructure tag, field name).
| // Write the updated config back to file | ||
| directory := os.ExpandEnv(home) | ||
| if err := writeConfigFile(directory, cfg); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Consider reusing the resolved directory from config.Init for consistency.
os.ExpandEnv(home) is called here, but config.Init also performs the same expansion internally. While currently harmless, if the expansion logic ever diverges, the write path might not match the read path. Consider either:
- Exposing the resolved directory from
config.Init, or - Extracting a shared helper function for directory resolution.
🤖 Prompt for AI Agents
In cmd/config/config.go around lines 121 to 125, the code calls
os.ExpandEnv(home) here but config.Init already resolves/expands the same home
path, which can lead to divergence; change this to reuse the resolved directory
from config.Init (or add a shared helper in the config package that performs the
expansion and use that in both Init and this write path) so the read and write
paths use the identical resolved directory; update call sites to use the new
exported resolved path or helper and ensure writeConfigFile is passed that
resolved directory.
- Change file permissions from 0644 to 0600 for security (config may contain sensitive values) - Fix potential integer overflow by using field.Type().Bits() instead of hardcoded 64 - Improve struct handling by serializing nested config sections as YAML for better readability
Add new 'sequoia config' subcommands to read and modify configuration values:
config get <key>- retrieves a config value by key (supports dot notation for nested values)config set <key> <value>- sets a config value and writes to config.yamlconfig show- displays the entire configurationExamples:
sequoia config get queue_interval sequoia config set queue_interval 50 sequoia config get api_config.port sequoia config set api_config.port 8080
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.