Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements M1.4 to add a comprehensive configuration loader for the ratelord daemon, replacing hardcoded settings with environment variable and CLI flag support. The implementation provides sensible defaults, proper precedence (flags override env vars), and validation for key configuration parameters.
Changes:
- Added daemon configuration loader with support for DB path, policy path, listen address, poll interval, and web assets mode configuration via env vars and CLI flags
- Integrated configuration system into daemon startup with proper error handling and structured logging
- Updated deployment and API documentation to reflect new configuration options
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/ratelord-d/config.go | New configuration loader implementing env/flag precedence, validation, and path resolution for daemon settings |
| cmd/ratelord-d/main.go | Integrated configuration loader, replaced hardcoded values with config fields, improved variable naming in reload logic |
| pkg/api/server.go | Added SetAddr method to enable runtime configuration of HTTP listen address |
| DEPLOYMENT.md | Updated environment variable documentation and corrected CLI flag names in deployment examples |
| API_SPEC.md | Documented new configuration options for address and port overrides |
| TASKS.md | Marked M1.4 as complete |
| PROGRESS.md | Added configuration milestone completion entries |
| PHASE_LEDGER.md | Documented M1.4 completion with implementation details |
| NEXT_STEPS.md | Updated project status to reflect configuration implementation |
| LEARNING.md | Added retrospective notes on configuration implementation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pollIntervalParsed, err := time.ParseDuration(*flagPollInterval) | ||
| if err != nil { | ||
| return Config{}, fmt.Errorf("invalid poll interval: %w", err) | ||
| } |
There was a problem hiding this comment.
The poll interval should be validated to ensure it's positive. A negative or zero poll interval would cause time.NewTicker to panic when the Poller starts. Add validation to check that pollIntervalParsed is greater than zero before returning the config.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| package main | ||
|
|
||
| import ( | ||
| "errors" | ||
| "flag" | ||
| "fmt" | ||
| "io" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "time" | ||
| ) | ||
|
|
||
| const ( | ||
| defaultAddr = "127.0.0.1:8090" | ||
| defaultPollInterval = 10 * time.Second | ||
| defaultWebAssetsMode = "embedded" | ||
| ) | ||
|
|
||
| type Config struct { | ||
| DBPath string | ||
| PolicyPath string | ||
| Addr string | ||
| PollInterval time.Duration | ||
| WebAssetsMode string | ||
| WebDir string | ||
| } | ||
|
|
||
| func LoadConfig(args []string) (Config, error) { | ||
| cwd, err := os.Getwd() | ||
| if err != nil { | ||
| return Config{}, fmt.Errorf("failed to get cwd: %w", err) | ||
| } | ||
|
|
||
| defaultDBPath := filepath.Join(cwd, "ratelord.db") | ||
| defaultPolicyPath := filepath.Join(cwd, "policy.json") | ||
|
|
||
| dbPath := envOrDefault("RATELORD_DB_PATH", defaultDBPath) | ||
| policyPath := envOrDefaultWithFallback([]string{"RATELORD_POLICY_PATH", "RATELORD_CONFIG_PATH"}, defaultPolicyPath) | ||
| addr := addrFromEnv(defaultAddr) | ||
| pollInterval := defaultPollInterval | ||
| if pollIntervalEnv := os.Getenv("RATELORD_POLL_INTERVAL"); pollIntervalEnv != "" { | ||
| parsed, err := time.ParseDuration(pollIntervalEnv) | ||
| if err != nil { | ||
| return Config{}, fmt.Errorf("invalid RATELORD_POLL_INTERVAL: %w", err) | ||
| } | ||
| pollInterval = parsed | ||
| } | ||
| webAssetsMode := envOrDefault("RATELORD_WEB_ASSETS_MODE", defaultWebAssetsMode) | ||
| webDir := os.Getenv("RATELORD_WEB_DIR") | ||
|
|
||
| flagSet := flag.NewFlagSet("ratelord-d", flag.ContinueOnError) | ||
| flagSet.SetOutput(io.Discard) | ||
| flagDB := flagSet.String("db", dbPath, "path to SQLite database") | ||
| flagPolicy := flagSet.String("policy", policyPath, "path to policy JSON") | ||
| flagAddr := flagSet.String("addr", addr, "HTTP listen address") | ||
| flagPollInterval := flagSet.String("poll-interval", pollInterval.String(), "provider poll interval") | ||
| flagWebAssets := flagSet.String("web-assets", webAssetsMode, "web assets mode: embedded|fs|off") | ||
| flagWebDir := flagSet.String("web-dir", webDir, "web assets directory when web-assets=fs") | ||
|
|
||
| if err := flagSet.Parse(args); err != nil { | ||
| if errors.Is(err, flag.ErrHelp) { | ||
| flagSet.SetOutput(os.Stdout) | ||
| flagSet.PrintDefaults() | ||
| return Config{}, err | ||
| } | ||
| return Config{}, err | ||
| } | ||
|
|
||
| pollIntervalParsed, err := time.ParseDuration(*flagPollInterval) | ||
| if err != nil { | ||
| return Config{}, fmt.Errorf("invalid poll interval: %w", err) | ||
| } | ||
|
|
||
| resolvedDBPath := resolvePath(*flagDB, cwd) | ||
| resolvedPolicyPath := resolvePath(*flagPolicy, cwd) | ||
| mode := normalizeWebAssetsMode(*flagWebAssets) | ||
|
|
||
| config := Config{ | ||
| DBPath: resolvedDBPath, | ||
| PolicyPath: resolvedPolicyPath, | ||
| Addr: strings.TrimSpace(*flagAddr), | ||
| PollInterval: pollIntervalParsed, | ||
| WebAssetsMode: mode, | ||
| WebDir: strings.TrimSpace(*flagWebDir), | ||
| } | ||
|
|
||
| if config.Addr == "" { | ||
| return Config{}, errors.New("addr cannot be empty") | ||
| } | ||
|
|
||
| if config.WebAssetsMode == "fs" { | ||
| if config.WebDir == "" { | ||
| return Config{}, errors.New("web-assets=fs requires web-dir") | ||
| } | ||
| config.WebDir = resolvePath(config.WebDir, cwd) | ||
| } | ||
|
|
||
| if config.WebAssetsMode != "embedded" && config.WebAssetsMode != "fs" && config.WebAssetsMode != "off" { | ||
| return Config{}, fmt.Errorf("unsupported web-assets mode: %s", config.WebAssetsMode) | ||
| } | ||
|
|
||
| return config, nil | ||
| } | ||
|
|
||
| func envOrDefault(key, fallback string) string { | ||
| if value := os.Getenv(key); value != "" { | ||
| return value | ||
| } | ||
| return fallback | ||
| } | ||
|
|
||
| func envOrDefaultWithFallback(keys []string, fallback string) string { | ||
| for _, key := range keys { | ||
| if value := os.Getenv(key); value != "" { | ||
| return value | ||
| } | ||
| } | ||
| return fallback | ||
| } | ||
|
|
||
| func addrFromEnv(fallback string) string { | ||
| if value := os.Getenv("RATELORD_ADDR"); value != "" { | ||
| return value | ||
| } | ||
| if port := os.Getenv("RATELORD_PORT"); port != "" { | ||
| return fmt.Sprintf("127.0.0.1:%s", port) | ||
| } | ||
| return fallback | ||
| } | ||
|
|
||
| func resolvePath(path string, cwd string) string { | ||
| trimmed := strings.TrimSpace(path) | ||
| if trimmed == "" { | ||
| return trimmed | ||
| } | ||
| if filepath.IsAbs(trimmed) { | ||
| return trimmed | ||
| } | ||
| return filepath.Join(cwd, trimmed) | ||
| } | ||
|
|
||
| func normalizeWebAssetsMode(mode string) string { | ||
| switch strings.ToLower(strings.TrimSpace(mode)) { | ||
| case "", "embedded": | ||
| return "embedded" | ||
| case "fs", "dir", "directory": | ||
| return "fs" | ||
| case "off", "disabled", "none": | ||
| return "off" | ||
| default: | ||
| return strings.ToLower(strings.TrimSpace(mode)) | ||
| } | ||
| } |
There was a problem hiding this comment.
The new config.go file lacks test coverage. Given that other packages in the codebase (store, engine, provider, client) have comprehensive test coverage, this configuration loader should also have tests to verify env/flag precedence, validation logic, path resolution, and error cases. For example, tests should cover: invalid poll intervals, empty addresses, missing web-dir when mode is 'fs', invalid web-assets modes, and precedence of flags over environment variables.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
* Initial plan * test(config): add comprehensive test coverage for daemon config loader - Test default values and path resolution - Test env var parsing and precedence - Test flag overrides env vars - Test validation errors (invalid poll intervals, empty addr, missing web-dir) - Test web-assets mode validation and normalization - Test env var fallbacks (RATELORD_CONFIG_PATH, RATELORD_PORT) - Test helper functions (resolvePath, normalizeWebAssetsMode) Addresses PR feedback requesting test coverage for config.go Co-authored-by: rmax <26015+rmax@users.noreply.github.com> * fix(test): improve error handling in config tests - Add proper error checking for os.Getwd() and os.Chdir() - Fix variable declaration issues in error test cases - All tests still pass Co-authored-by: rmax <26015+rmax@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: rmax <26015+rmax@users.noreply.github.com>
Motivation
Description
cmd/ratelord-d/config.gowhich definesConfigandLoadConfigwith env/flag precedence, sensible defaults, parsing/validation, and support forRATELORD_DB_PATH,RATELORD_POLICY_PATH/RATELORD_CONFIG_PATH,RATELORD_ADDR/RATELORD_PORT,RATELORD_POLL_INTERVAL,RATELORD_WEB_ASSETS_MODE, andRATELORD_WEB_DIR.cmd/ratelord-d/main.goso the daemon usescfg.DBPathfor the store,cfg.PollIntervalfor the poller,cfg.PolicyPathfor initial load and SIGHUP reloads,cfg.Addrfor the API server, andcfg.WebAssetsMode/cfg.WebDirto select embedded/fs/off asset handling.SetAddrtopkg/api/server.goand updated server startup to honor the configured listen address, plus explicit handling for web asset modes (embedded/fs/off).DEPLOYMENT.md,API_SPEC.md,NEXT_STEPS.md,TASKS.md,PROGRESS.md,PHASE_LEDGER.md, andLEARNING.mdwere edited to document knobs and markM1.4complete.Testing
go test ./...which completed successfully for the testable packages (suite passed).go vet ./...which produced no issues.gofmtas part of the change pipeline.Codex Task