fix: return errors instead of panicking in config file substitution - #324
fix: return errors instead of panicking in config file substitution#324mvanhorn wants to merge 2 commits into
Conversation
Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the robustness of the configuration loader by replacing runtime panics with graceful error handling. When a file-based configuration substitution fails, the system now propagates the error up the call stack, allowing the application to handle missing files or incorrect paths without crashing. This change introduces a breaking API modification to the substitution functions, which has been addressed across the codebase. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. The code would panic, stop, and die, / When config files were not nearby. / Now errors flow with grace and care, / And leave the system standing there. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the configuration loader to return errors instead of panicking during file substitution, updating the associated tests to handle these errors. The review feedback suggests several optimizations and improvements: handling reflect.Interface types during reflection traversal, and reducing reflection overhead by skipping recursive processing for primitive types inside maps, slices, and arrays.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // If it's a pointer, resolve it | ||
| if v.Kind() == reflect.Ptr { | ||
| if v.IsNil() { | ||
| return | ||
| return nil | ||
| } | ||
| SubstituteConfigValues(v.Elem()) | ||
| return | ||
| return SubstituteConfigValues(v.Elem()) | ||
| } |
There was a problem hiding this comment.
Currently, SubstituteConfigValues only unwraps pointers (reflect.Ptr) at the top level. If a struct contains a field of type interface{} (e.g., interface{} holding a nested struct or map), its kind will be reflect.Interface. Since this is not handled at the top level, the function will return nil immediately, and any string values inside the interface will not be substituted.
We should handle reflect.Interface similarly to reflect.Ptr by unwrapping it using v.Elem().
| // If it's a pointer, resolve it | |
| if v.Kind() == reflect.Ptr { | |
| if v.IsNil() { | |
| return | |
| return nil | |
| } | |
| SubstituteConfigValues(v.Elem()) | |
| return | |
| return SubstituteConfigValues(v.Elem()) | |
| } | |
| // If it's a pointer or interface, resolve it | |
| if v.Kind() == reflect.Ptr || v.Kind() == reflect.Interface { | |
| if v.IsNil() { | |
| return nil | |
| } | |
| return SubstituteConfigValues(v.Elem()) | |
| } |
| // Only settable if map value is addressable, so we replace by setting | ||
| if val.Kind() == reflect.String { | ||
| newVal := reflect.ValueOf(substituteString(val.String())) | ||
| substituted, err := substituteString(val.String()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| newVal := reflect.ValueOf(substituted) | ||
| v.SetMapIndex(key, newVal) | ||
| } else { | ||
| // Recursively process nested maps/structs | ||
| copyVal := reflect.New(val.Type()).Elem() | ||
| copyVal.Set(val) | ||
| SubstituteConfigValues(copyVal) | ||
| if err := SubstituteConfigValues(copyVal); err != nil { | ||
| return err | ||
| } | ||
| v.SetMapIndex(key, copyVal) | ||
| } |
There was a problem hiding this comment.
In the else block of the map processing logic, any non-string value (including primitive types like int, bool, float64, etc.) is recursively processed. This causes unnecessary allocations via reflect.New and reflection overhead for every primitive value in a map.
We can optimize this by only recursively processing types that can actually contain nested structures or strings (i.e., pointers, structs, maps, slices, and arrays).
| // Only settable if map value is addressable, so we replace by setting | |
| if val.Kind() == reflect.String { | |
| newVal := reflect.ValueOf(substituteString(val.String())) | |
| substituted, err := substituteString(val.String()) | |
| if err != nil { | |
| return err | |
| } | |
| newVal := reflect.ValueOf(substituted) | |
| v.SetMapIndex(key, newVal) | |
| } else { | |
| // Recursively process nested maps/structs | |
| copyVal := reflect.New(val.Type()).Elem() | |
| copyVal.Set(val) | |
| SubstituteConfigValues(copyVal) | |
| if err := SubstituteConfigValues(copyVal); err != nil { | |
| return err | |
| } | |
| v.SetMapIndex(key, copyVal) | |
| } | |
| // Only settable if map value is addressable, so we replace by setting | |
| if val.Kind() == reflect.String { | |
| substituted, err := substituteString(val.String()) | |
| if err != nil { | |
| return err | |
| } | |
| newVal := reflect.ValueOf(substituted) | |
| v.SetMapIndex(key, newVal) | |
| } else if k := val.Kind(); k == reflect.Ptr || k == reflect.Struct || k == reflect.Map || k == reflect.Slice || k == reflect.Array { | |
| // Recursively process nested maps/structs/slices/pointers | |
| copyVal := reflect.New(val.Type()).Elem() | |
| copyVal.Set(val) | |
| if err := SubstituteConfigValues(copyVal); err != nil { | |
| return err | |
| } | |
| v.SetMapIndex(key, copyVal) | |
| } |
| // If it's a slice or array, process its elements | ||
| if v.Kind() == reflect.Slice || v.Kind() == reflect.Array { | ||
| for i := 0; i < v.Len(); i++ { | ||
| SubstituteConfigValues(v.Index(i)) | ||
| if err := SubstituteConfigValues(v.Index(i)); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return | ||
| return nil | ||
| } |
There was a problem hiding this comment.
When processing slices or arrays, the function recursively calls SubstituteConfigValues on every single element. For large slices of primitive types (e.g., []int, []bool), this results in significant unnecessary overhead.
We can optimize this by checking the element type of the slice/array first, and skipping the loop entirely if the elements cannot contain strings or nested structures.
// If it's a slice or array, process its elements
if v.Kind() == reflect.Slice || v.Kind() == reflect.Array {
elemKind := v.Type().Elem().Kind()
if elemKind == reflect.String || elemKind == reflect.Ptr || elemKind == reflect.Interface ||
elemKind == reflect.Struct || elemKind == reflect.Map || elemKind == reflect.Slice || elemKind == reflect.Array {
for i := 0; i < v.Len(); i++ {
if err := SubstituteConfigValues(v.Index(i)); err != nil {
return err
}
}
}
return nil
}The dev.yaml setup line in TestLoad exceeded the 120-char limit (lll). Split the os.WriteFile arguments across lines; no behavior change.
1 participant
Changes
📝 Description
What changed?
substituteStringinpkg/config/loader.gopreviously calledpanic()whenos.ReadFilefailed for afile|substitution path. It now returns a wrapped error (failed to read config file <path>: <cause>).SubstituteConfigValuesreturnserrorand propagates the first failure up through(*Config).Load, whose signature already returnserror. Theenv|substitution and plain-string handling are unchanged.Why is this change needed?
If a Kubernetes secret volume mount is missing or a configured file path is wrong, the operator crashes with no recovery path. Returning an error lets the caller handle the failure instead of taking down the process. Fixes #276.
Dependencies
🧪 Testing
Test Coverage
Added
pkg/config/loader_test.gocovering: reading a validfile|path, anenv|value, a plain string, a missingfile|path returning an error without panicking, andLoadsurfacing the file error. Updated the existingTestFileSubstitutionto the newSubstituteConfigValuesreturn signature.Ran
go build ./...(passes) andgo test -count=1 ./pkg/config/...(11 tests pass).Performance Impact
🚀 Deployment
Deploy Steps
Prerequisites
Post-Deployment Monitoring
Rollback Plan
Details:
The exported
SubstituteConfigValuessignature changes fromfunc(reflect.Value)tofunc(reflect.Value) error. All in-tree callers are updated. External callers, if any, need to handle the returned error.⚙️ Configuration Changes
✅ Developer Checklist