proxy/config: create config package and migrate configuration #329
Conversation
The configuration is become more complex as llama-swap adds more advanced features. This commit moves config to its own package so it can be developed independently of the proxy package. Additionally, enforcing a public API for a configuration will allow downstream usage to be more decoupled.
WalkthroughRefactors configuration into a dedicated Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Main as llama-swap (main)
participant CFG as config.LoadConfig
participant PM as Proxy (srv)
participant Watch as fsnotify (watcher)
Note over Main,CFG: startup
Main->>CFG: LoadConfig(path)
CFG-->>Main: conf
Main->>PM: srv.Handler = proxy.New(conf)
Note right of PM: proxy handler running
Note over Watch: config file watcher
Watch->>Main: config change event
Main->>CFG: LoadConfig(path) -- reload
CFG-->>Main: conf'
Main->>PM: srv.Handler = proxy.New(conf') -- swap handler
Note right of PM: handler replaced (hot-swap)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
proxy/proxymanager_test.go (1)
25-68: Fix the short var declarations that shadow theconfigpackageStatements like
config := config.AddDefaultGroupToConfig(...)attempt to declare a local variable namedconfigwhile simultaneously referring to the importedconfigpackage. In Go, the newly declared identifier’s scope covers the entire statement, so the right-hand side no longer sees the package and compilation fails (e.g. “config.AddDefaultGroupToConfig undefined”). The same issue appears throughout this file, including other short declarations (config := ...,config, err := ...,config := config.Config{...}). Please rename the local variable (e.g.cfg) in all of these spots.A representative fix for this block:
- config := config.AddDefaultGroupToConfig(config.Config{ + cfg := config.AddDefaultGroupToConfig(config.Config{ HealthCheckTimeout: 15, Models: map[string]config.ModelConfig{ "model1": getTestSimpleResponderConfig("model1"), "model2": getTestSimpleResponderConfig("model2"), }, LogLevel: "error", }) - proxy := New(config) + proxy := New(cfg)Apply the same pattern everywhere the tests currently shadow the package name so the suite compiles again.
🧹 Nitpick comments (1)
llama-swap.go (1)
42-95: Avoid double-loading configuration on startupLine 42 already loads the config into
conf, but whenreloadProxyManager()runs for the first time (Line 98) theelsebranch executes and immediately callsconfig.LoadConfigagain becausesrv.Handleris stillnil. Consider threading the preloadedconfinto that initial handler creation so we only hit the filesystem once during startup. This keeps boot time leaner, especially when configs live on slower volumes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
Makefile(2 hunks)config/config.go(1 hunks)config/config_posix_test.go(1 hunks)config/config_test.go(1 hunks)llama-swap.go(4 hunks)proxy/helpers_test.go(2 hunks)proxy/metrics_monitor.go(2 hunks)proxy/process.go(4 hunks)proxy/process_test.go(6 hunks)proxy/processgroup.go(2 hunks)proxy/processgroup_test.go(4 hunks)proxy/proxymanager.go(3 hunks)proxy/proxymanager_test.go(22 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
config/config_posix_test.go (2)
proxy/config_posix_test.go (2)
TestConfig_DefaultValuesPosix(49-89)TestConfig_LoadPosix(91-242)proxy/config_test.go (6)
TestConfig_MacroInCommentStrippedBeforeExpansion(383-442)name(331-335)TestConfig_AutomaticPortAssignments(128-200)TestConfig_MacroReplacement(202-232)name(235-239)TestConfig_MacroModelId(444-483)
proxy/processgroup.go (2)
config/config.go (1)
Config(149-169)proxy/logMonitor.go (1)
LogMonitor(23-35)
proxy/proxymanager.go (1)
config/config.go (1)
Config(149-169)
proxy/metrics_monitor.go (1)
config/config.go (1)
Config(149-169)
proxy/process_test.go (2)
config/config.go (1)
ModelConfig(20-40)proxy/process.go (1)
NewProcess(78-103)
proxy/helpers_test.go (1)
config/config.go (1)
ModelConfig(20-40)
proxy/process.go (1)
config/config.go (2)
ModelConfig(20-40)SanitizeCommand(414-447)
proxy/processgroup_test.go (2)
config/config.go (5)
AddDefaultGroupToConfig(370-412)Config(149-169)ModelConfig(20-40)GroupConfig(116-121)DEFAULT_GROUP_ID(18-18)proxy/processgroup.go (1)
NewProcessGroup(29-54)
proxy/proxymanager_test.go (2)
config/config.go (7)
AddDefaultGroupToConfig(370-412)Config(149-169)ModelConfig(20-40)GroupConfig(116-121)DEFAULT_GROUP_ID(18-18)LoadConfigFromReader(198-367)ModelFilters(77-79)proxy/proxymanager.go (1)
New(48-131)
llama-swap.go (2)
config/config.go (1)
LoadConfig(189-196)proxy/proxymanager.go (1)
New(48-131)
…geek#329) * proxy/config: create config package and migrate configuration The configuration is become more complex as llama-swap adds more advanced features. This commit moves config to its own package so it can be developed independently of the proxy package. Additionally, enforcing a public API for a configuration will allow downstream usage to be more decoupled.
The configuration is become more complex as llama-swap adds more advanced features. This commit moves config to its own package so it can be developed independently of the proxy package.
Additionally, enforcing a public API for a configuration will allow downstream usage to be more decoupled.
Summary by CodeRabbit