Refactor all default config values into config.go #162
Conversation
move all default values for global, model and group configurations so it is all in one place.
|
Warning Rate limit exceeded@mostlygeek has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 9 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughThis change introduces explicit default value handling for configuration structs, including platform-specific defaults during YAML unmarshaling. It centralizes and simplifies default assignments, enforces stricter validation for certain fields, and adds consistency checks for macro usage in model configurations. Associated tests are updated and expanded to verify these behaviors across platforms. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConfigLoader
participant ModelConfig
User->>ConfigLoader: LoadConfigFromReader(reader)
ConfigLoader->>ConfigLoader: Set global defaults (HealthCheckTimeout, StartPort, LogLevel)
ConfigLoader->>ModelConfig: UnmarshalYAML (with explicit defaults)
ModelConfig-->>ConfigLoader: Model config with defaults
ConfigLoader->>ConfigLoader: Validate fields (HealthCheckTimeout, StartPort)
ConfigLoader->>ConfigLoader: Check macro consistency (Cmd/Proxy with ${PORT})
ConfigLoader-->>User: Loaded & validated Config struct
Possibly related PRs
Suggested labels
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
proxy/config.go(3 hunks)proxy/config_posix_test.go(2 hunks)proxy/config_test.go(2 hunks)proxy/config_windows_test.go(2 hunks)proxy/process.go(0 hunks)
💤 Files with no reviewable changes (1)
- proxy/process.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests
🔇 Additional comments (4)
proxy/config.go (2)
34-60: LGTM! Clean implementation of explicit defaults.The UnmarshalYAML method properly sets explicit defaults for all ModelConfig fields, including the platform-specific
CmdStopfor Windows. The use of a type alias to avoid recursion is the correct pattern.
142-160: Good refactoring of default value handling.Setting defaults before unmarshaling and simplifying the validation logic makes the code cleaner and more maintainable. The minimum constraints (15s for health check timeout, >=1 for start port) are reasonable.
proxy/config_test.go (1)
80-113: Test updates correctly reflect the new default values.The expected values now properly include the explicit defaults set during configuration loading, including
LogLevel: "info"and empty slices forEnvandAliasesfields.proxy/config_posix_test.go (1)
45-84: Well-structured test for POSIX default values.The test comprehensively validates all default configuration values on POSIX systems, including global defaults, model defaults, and the automatic
${PORT}replacement. The expectation of emptyCmdStopis correct for non-Windows platforms.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
proxy/config.go (2)
34-60: Provide a POSIX fallback forCmdStop& revisit the unconditional proxy default
defaults.CmdStopis only initialised for Windows. On POSIX the field remains the empty string, which forces every model author to specify a stop command explicitly or rely onprocess.Killlater on. For symmetry with Windows (and to keep boilerplate low) consider a sane default such askill -TERM ${PID}.Likewise, setting
Proxyto"http://localhost:${PORT}"unconditionally means every model inherits a proxy even when it does not expose an HTTP interface. This may surprise existing configurations that deliberately omitted the field. A safer pattern is to leaveProxyempty by default and selectively inject the placeholder only when${PORT}appears inCmd(the behaviour you previously had).- Proxy: "http://localhost:${PORT}", + Proxy: "",and after unmarshalling, if
Cmdcontains${PORT}andProxyis still empty, populate it with the localhost template.
142-147: Minor: expose default values via constants
120,5800, and"info"appear as magic literals. Defining them as typed constants (e.g.const DefaultHealthCheckTimeout = 120) improves discoverability, centralises future changes, and keeps the intent explicit throughout the codebase.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
proxy/config.go(3 hunks)proxy/config_posix_test.go(2 hunks)proxy/config_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- proxy/config_test.go
- proxy/config_posix_test.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests
🔇 Additional comments (2)
proxy/config.go (2)
208-211: Improved error message looks goodThe new wording clearly communicates the
${PORT}mismatch betweencmdandproxy. Nice catch.
153-160:⚠️ Potential issue
startPortrejects the perfectly valid value1The validation enforces
startPort > 1(< 1is rejected), so1is treated as invalid even though it is a non-privileged port on all platforms. If the intention is to forbid zero, change the check to:- if config.StartPort < 1 { + if config.StartPort == 0 {Otherwise update the error message to clarify that values below 2 are disallowed.
Likely an incorrect or invalid review comment.
PR #162 refactored the default configuration code. This introduced a subtle bug where `env` became `[]string{}` instead of the default of `nil`. In golang, `exec.Cmd.Env == nil` means to use the "current process's environment". By setting it to `[]string{}` as a default the Process's environment was emptied out which caused an array of strange and difficult to troubleshoot behaviour. See issues #168 and #169 This commit changes the behaviour to append model configured environment variables to the default list rather than replace them.
Append custom env vars instead of replace in Process (#168, #169) PR #162 refactored the default configuration code. This introduced a subtle bug where `env` became `[]string{}` instead of the default of `nil`. In golang, `exec.Cmd.Env == nil` means to use the "current process's environment". By setting it to `[]string{}` as a default the Process's environment was emptied out which caused an array of strange and difficult to troubleshoot behaviour. See issues #168 and #169 This commit changes the behaviour to append model configured environment variables to the default list rather than replace them.
move all default values for global, model and group configurations so it is all in one place. No functional changes for users but cleaned and made testing more rigid.
Summary by CodeRabbit
New Features
Bug Fixes
${PORT}, the command must also include${PORT}.Tests
Refactor