feat(config): support nested environment variables#27
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaced the project's custom reflection-based env parsing with github.com/caarlos0/env/v11, added an internal generic deep-merge function for structs, updated environment/YAML samples and go.mod, and adjusted tests to use the new parser and validate nested-struct merging and parse errors. Changes
Sequence Diagram(s)sequenceDiagram
participant ForRoot
participant YAML_Loader
participant EnvLoader as env.Parse
participant Merger as mergeStruct
participant App
ForRoot->>YAML_Loader: load YAML config (env.yaml)
YAML_Loader-->>ForRoot: YAML struct
ForRoot->>EnvLoader: parse .env/.env.example into struct
EnvLoader-->>ForRoot: env struct (or error)
ForRoot->>Merger: merge YAML struct + env struct
Merger-->>ForRoot: merged config
ForRoot->>App: provide merged config to feature/provider
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Support nested struct in configuration with hybrid loading from YAML and Env files.
4ffe9d2 to
15f6cc2
Compare
![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
🤖 Fix all issues with AI agents
In `@namespace.go`:
- Around line 29-34: The ForFeature function currently calls log.Fatalf on
env.ParseWithOptions failure (using env.ParseWithOptions(&value, opts)), which
force-exits instead of returning an error like NewEnv; change this to return the
parse error (or wrap it with context) to match NewEnv's error-returning behavior
and be consistent with how ForRoot handles errors, and remove the call to
log.Fatalf so callers can handle the error.
🧹 Nitpick comments (4)
.env.example (1)
19-23: Add trailing newline and consider consistent formatting.The static analysis correctly flags that line 23 is missing a trailing newline. Additionally, while this is an example file, consider maintaining consistent formatting with or without spaces around
=throughout.Suggested fix
MONGO_DBNAME = db DB_USER = root DB_PASS = root -DB_NAME = db +DB_NAME = db +module.go (2)
96-113: Pointer, slice, and map fields use shallow IsZero semantics.The current implementation treats pointer, slice, and map fields as primitives—they're overwritten only if non-zero. This means:
- A non-nil pointer in
sourcereplacestarget's pointer entirely (no deep merge of pointed-to structs)- Slices/maps are replaced wholesale, not merged element-by-element
If nested pointer-to-struct fields need deep merging, additional handling is required. If shallow replacement is intentional for these types, consider adding a comment to document this behavior.
84-94: Potential panic ifTis not a struct type.
mergeRecursivecallstarget.NumField()which panics if the value is not a struct. Since the generic constraint isT any, callers could theoretically pass non-struct types.Consider either:
- Adding a runtime check at the start of
mergeStruct- Documenting that
Tmust be a struct typeOptional defensive check
func mergeStruct[T any](input ...T) T { var result T resultVal := reflect.ValueOf(&result).Elem() + if resultVal.Kind() != reflect.Struct { + return result + } for _, item := range input { inputVal := reflect.ValueOf(item) mergeRecursive(resultVal, inputVal) } return result }env.go (1)
15-22: Consider simplifying this comment.The comment expresses uncertainty but the code clearly returns an error on
godotenv.Loadfailure. A simpler explanation would suffice:Suggested simplification
- // We don't check for error here because it's possible that the user - // provides environment variables directly without a .env file. - // However, to maintain backward compatibility with previous strict behavior (as enforced by tests), - // we will return error if godotenv fails, unless we decide to relax it. - // The previous implementation returned error. + // Return error if .env file cannot be loaded to maintain backward compatibility. + // Consider relaxing this if users need to provide env vars without a file. if err := godotenv.Load(path); err != nil { return nil, err }
| opts := env.Options{ | ||
| DefaultValueTagName: "default", | ||
| } | ||
| if err := env.ParseWithOptions(&value, opts); err != nil { | ||
| log.Fatalf("failed to parse env: %v", err) | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling: log.Fatalf vs returning error.
ForFeature calls log.Fatalf on parse failure, terminating the program immediately. However, NewEnv in env.go returns errors to the caller. This inconsistency could surprise users:
config.NewEnv[T]()→ returnserror, caller handles itconfig.ForFeature[T]()→ crashes the program on parse error
Consider returning an error or logging without terminating, similar to how ForRoot handles errors (lines 39-41 in module.go continue on error).
🤖 Prompt for AI Agents
In `@namespace.go` around lines 29 - 34, The ForFeature function currently calls
log.Fatalf on env.ParseWithOptions failure (using env.ParseWithOptions(&value,
opts)), which force-exits instead of returning an error like NewEnv; change this
to return the parse error (or wrap it with context) to match NewEnv's
error-returning behavior and be consistent with how ForRoot handles errors, and
remove the call to log.Fatalf so callers can handle the error.
1 participant
Support nested struct in configuration with hybrid loading from YAML and Env files.