Feature: config refactor 3#61
Conversation
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughThis PR standardizes the default SQLite path to Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment
participant FS as config.yaml (File)
participant Loader as Config Loader
participant Reflect as applyEnvOverrides
participant App as Application (uses Config)
Env->>Loader: process start (Load())
Loader->>FS: read YAML into cfg
Loader->>Reflect: applyEnvOverrides(cfg)
Reflect->>Env: read env vars per `env:` tags
Reflect-->>Loader: populated cfg
Loader->>App: return final cfg
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/config.go (1)
392-395: 🧹 Nitpick | 🔵 TrivialDead code: Ollama special-case check is unreachable.
Lines 388–390 already
continuewhen bothapiKeyandbaseURLare empty. The identical condition on line 393 (apiKey == "" && baseURL == "") can never be true at that point.♻️ Remove dead branch
// Skip if no env vars set for this provider if apiKey == "" && baseURL == "" { continue } - // Ollama special case: no API key required, enabled via base URL - if kp.providerType == "ollama" && apiKey == "" && baseURL == "" { - continue - } - existing, exists := cfg.Providers[kp.name]
🤖 Fix all issues with AI agents
In `@config/config.go`:
- Around line 350-359: The int parsing branch in Load() currently swallows
strconv.Atoi errors (case reflect.Int) causing silent failures; update that
branch to surface or log parse errors instead of silently skipping: when
strconv.Atoi(envVal) returns an error, either append/return an error from Load()
(so callers see invalid env like POSTGRES_MAX_CONNS=abc) or emit a warning
including the field identifier (use the struct field/name or tag) and the bad
value before leaving the default; keep the successful path that calls
fieldVal.SetInt(int64(n)) unchanged.
In `@docs/advanced/configuration.mdx`:
- Line 22: Replace the placeholder "TODO: the link to the quick start should be
provided here!" in docs/advanced/configuration.mdx with a real quick-start link
or remove the line; specifically locate the TODO string and either insert an MDX
link pointing to the project's Quick Start page (e.g., the canonical docs
quick-start route) or delete the placeholder so it won't appear in published
docs.
In `@internal/storage/sqlite.go`:
- Around line 20-22: Introduce a shared constant storage.DefaultSQLitePath in
the storage package and replace all hardcoded "data/gomodel.db" occurrences by
referencing that constant: update the default assignment that sets cfg.Path (in
the function that initializes SQLite storage in sqlite.go), the storage
initialization in storage.go, and the factory/config initializers that currently
set defaults (those that check/assign cfg.Path or equivalent in
config/config.go, internal/usage/factory.go, and internal/auditlog/factory.go)
to use storage.DefaultSQLitePath instead of the literal string; ensure the
constant is exported (DefaultSQLitePath) and imported/used where needed so all
default-path fallbacks reference the single source of truth.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…h constant Introduce a shared DefaultSQLitePath constant in the storage package and use it across all default-path fallbacks so the default SQLite database path is defined in a single place. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/usage/factory.go (1)
114-142: 🧹 Nitpick | 🔵 Trivial
buildStorageConfigis duplicated betweenusage/factory.goandauditlog/factory.go.This function is nearly identical to
auditlog/factory.go:buildStorageConfig(lines 83–111). Both copy config fields and apply the same defaults. Consider extracting a shared helper in thestoragepackage (or reusingstorage.DefaultConfig()with field overlay) to keep them in sync.
🤖 Fix all issues with AI agents
In `@config/config.go`:
- Around line 329-370: The reflection walker applyEnvOverridesValue currently
switches on field.Type.Kind() handling String, Bool, and Int but silently
ignores other kinds when an `env` tag is present; add a default branch in that
switch to return a clear error (or at minimum log and return an error) naming
the field and unsupported kind so callers fail fast when unsupported typed
fields (e.g., int64, float64, slices, maps) carry an `env` tag; update the error
message to include field.Name, envKey and field.Type.Kind() to make the failure
actionable and ensure any caller of applyEnvOverridesValue surfaces the problem.
- Around line 14-16: The config package now imports internal/storage only to use
the DefaultSQLitePath constant, creating a fragile upward dependency; move the
DefaultSQLitePath constant out of internal/storage into the config package (or a
new shared defaults package) and update storage to reference
config.DefaultSQLitePath (or defaults.DefaultSQLitePath) instead; locate the
constant definition in internal/storage (search for DefaultSQLitePath) and move
it, then remove the storage import from config and adjust any references to use
the new symbol in config (or defaults) to keep the dependency arrow pointing
inward.
|
|
||
| "gomodel/internal/storage" | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Check if internal/storage imports config
rg -n '"gomodel/config"' --type=go -g 'internal/storage/**'Repository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Also check the overall dependency structure
rg -n '"gomodel/internal/storage"' --type=go -g 'config/**'Repository: ENTERPILOT/GOModel
Length of output: 109
🏁 Script executed:
# Find where DefaultSQLitePath is defined and used
rg -n 'DefaultSQLitePath' --type=goRepository: ENTERPILOT/GOModel
Length of output: 586
Architectural note: config now depends on internal/storage.
This introduces a dependency from a top-level package (config) into an internal package (internal/storage) solely for the DefaultSQLitePath constant. While there's no import cycle today, if storage ever needs to import config, one will appear. Consider moving the constant to the config package (or a shared defaults package) and having storage reference it instead—keeping the dependency arrow pointing inward.
🤖 Prompt for AI Agents
In `@config/config.go` around lines 14 - 16, The config package now imports
internal/storage only to use the DefaultSQLitePath constant, creating a fragile
upward dependency; move the DefaultSQLitePath constant out of internal/storage
into the config package (or a new shared defaults package) and update storage to
reference config.DefaultSQLitePath (or defaults.DefaultSQLitePath) instead;
locate the constant definition in internal/storage (search for
DefaultSQLitePath) and move it, then remove the storage import from config and
adjust any references to use the new symbol in config (or defaults) to keep the
dependency arrow pointing inward.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores