feat: hybrid config yaml and env - #22
Conversation
Summary by CodeRabbit
WalkthroughThe Makefile was expanded with new phony targets for auditing, formatting, linting, tidying, testing, and benchmarking, while the test-coverage target was removed. The go.mod dependency for Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test_Hybrid
participant Core as core.NewModule
participant Config as config.ForRoot
participant Module as Module
Test->>Core: NewModule(config.ForRoot(env.yaml, .env.example))
Core->>Config: ForRoot(env.yaml, .env.example)
Config->>Config: Parse YAML and env files into structs
Config->>Config: Merge structs into one config
Config-->>Core: Return merged config
Core-->>Module: Create module with config
Test->>Module: Get config reference
Module-->>Test: Return *HybridEnv
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
![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
🧹 Nitpick comments (2)
module.go (1)
36-57: Approve the hybrid configuration implementation with suggestions for improvement.The implementation correctly accumulates multiple configurations and merges them using
common.MergeStruct. This enables loading from multiple sources (YAML + env files) as intended.However, consider these improvements:
} else { var envArrs []E + // Pre-allocate slice capacity to avoid repeated allocations + envArrs = make([]E, 0, len(params)) for _, v := range params { if reflect.TypeOf(v).Kind() == reflect.String { val, err := New[E](any(v).(string)) if err != nil { + log.Printf("Failed to load config from %v: %v", v, err) continue } envArrs = append(envArrs, *val) } else if reflect.TypeOf(v).Kind() == reflect.Struct { opt := any(v).(Options[E]) err = godotenv.Load(opt.EnvPath) if err != nil { + log.Printf("Failed to load env file %s: %v", opt.EnvPath, err) continue } val := opt.Load() + if val == nil { + log.Printf("Load function returned nil for %s", opt.EnvPath) + continue + } envArrs = append(envArrs, *val) } } if len(envArrs) > 0 { value := common.MergeStruct(envArrs...) lastValue = &value }Improvements suggested:
- Performance: Pre-allocate slice capacity to avoid repeated allocations
- Debugging: Add detailed error logging to help troubleshoot configuration loading issues
- Safety: Add nil check for the
Load()function result to prevent potential panicsMakefile (1)
1-32: Excellent Makefile enhancements for development workflow.The new targets provide a comprehensive development toolkit with auditing, formatting, linting, testing, and benchmarking capabilities. All targets are properly declared as phony and use appropriate tools.
Consider adding the missing standard targets suggested by static analysis:
+.PHONY: all +all: tidy format lint audit test + +.PHONY: clean +clean: + go clean -cache -testcache -modcache + rm -rf coverage/ + .PHONY: auditBenefits of suggested additions:
alltarget: Provides a single command to run the complete development pipelinecleantarget: Helps reset the development environment by clearing caches and generated files- Standard compliance: Follows common Makefile conventions expected by developers
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
Makefile(1 hunks)go.mod(1 hunks)module.go(2 hunks)module_test.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
module_test.go (1)
module.go (2)
ForRoot(25-75)ENV(12-12)
module.go (1)
config.go (1)
New(9-22)
🪛 checkmake (0.2.2)
Makefile
[warning] 29-29: Missing required phony target "all"
(minphony)
[warning] 29-29: Missing required phony target "clean"
(minphony)
🔇 Additional comments (3)
go.mod (1)
8-8: Dependency update verified – no issues found
- Release notes for v2.1.2–v2.3.0 add only new features (including
MergeStructin v2.1.2) and bug fixes, with no breaking changes documented.- GitHub security advisories report no vulnerabilities affecting versions ≤ v2.3.0.
No further action required.
module.go (1)
8-8: LGTM! Import added for hybrid configuration support.The new import enables the
common.MergeStructfunctionality needed for merging multiple configuration sources.module_test.go (1)
4-4: LGTM! Import added for test output.The
fmtimport supports the test output in the new hybrid configuration test.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
module_test.go (1)
92-118: Excellent implementation of the hybrid configuration test!This test effectively addresses all the concerns from the previous review:
- ✅ Comprehensive assertions validate merged configuration values
- ✅ Clear comments indicate source of each value
- ✅ Proper use of
t.Logffor test output- ✅ Robust validation of the merged result
The test demonstrates proper hybrid configuration loading from multiple sources (YAML and env files).
Consider adding a small enhancement for robustness:
require.Equal(t, "development", cfg.NodeEnv) // From YAML file require.Equal(t, 3000, cfg.Port) // From YAML file require.True(t, cfg.Log) // From YAML file - require.NotEmpty(t, cfg.Special) // From env file (if SPECIAL is set) + // Check if SPECIAL env var is set, otherwise verify it's handled gracefully + if os.Getenv("SPECIAL") != "" { + require.NotEmpty(t, cfg.Special) // From env file + } else { + t.Logf("SPECIAL env var not set, got: %q", cfg.Special) + }This ensures the test handles cases where the SPECIAL environment variable might not be set in the test environment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
module_test.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: build (1.22.x)
module_test.go
[failure] 4-4:
"fmt" imported and not used
🪛 GitHub Actions: Go
module_test.go
[error] 4-4: Go test error: "fmt" imported and not used.
1 participant
No description provided.