refactor(config): extract BindSourcesToStructs into pkg/config/binder - #3270
refactor(config): extract BindSourcesToStructs into pkg/config/binder#3270dschmidt wants to merge 2 commits into
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
🟢 Coverage 66.67% diff coverage · 0.00% coverage variation
Metric Results Coverage variation ✅ 0.00% coverage variation (-1.00%) Diff coverage ✅ 66.67% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (ab0b3c5) 88027 20575 23.37% Head commit (eba5e87) 88030 (+3) 20575 (+0) 23.37% (0.00%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#3270) 27 18 66.67% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Load the yaml config file via opencloud/pkg/config.BindSourcesToStructs (env vars still via opencloud/pkg/config/envdecode), so immichframe depends only on regular released opencloud (pinned to the v7.4.0 commit) and not on the adapter kit. This pulls in opencloud's service configs; opencloud-eu/opencloud#3270 extracts BindSourcesToStructs into a leaf package to drop those again.
1af19e3 to
2966708
Compare
Load the yaml config file via opencloud/pkg/config.BindSourcesToStructs (env vars still via opencloud/pkg/config/envdecode), so immichframe depends only on regular released opencloud (pinned to the v7.4.0 commit) and not on the adapter kit. This pulls in opencloud's service configs; opencloud-eu/opencloud#3270 extracts BindSourcesToStructs into a leaf package to drop those again.
There was a problem hiding this comment.
🟡 Changes recommended
It introduces an API-breaking re-export shape (function → assignable exported var) and contains a binder implementation issue (ignored YAML parse error + indirect binding target) plus a potentially flaky env-dependent test.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors configuration binding by moving BindSourcesToStructs into a new leaf package (pkg/config/binder) so external/third-party services can reuse OpenCloud’s YAML/env binding without importing the aggregate pkg/config dependency tree.
Changes:
- Introduces
pkg/config/binderwithBindSourcesToStructsand a test-friendlyBindSourcesToStructsFS. - Keeps backward compatibility by exposing
BindSourcesToStructsfrompkg/config. - Moves/adjusts unit tests accordingly (including new package-level binder tests).
File summaries
| File | Description |
|---|---|
| pkg/config/helpers.go | Re-exports BindSourcesToStructs from the new leaf binder package. |
| pkg/config/helpers_test.go | Updates tests to call binder FS-binding helper directly. |
| pkg/config/binder/binder.go | New leaf implementation for YAML/env binding into config structs. |
| pkg/config/binder/binder_test.go | New unit tests for binder package behavior. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| _ = cnf.LoadSources("yaml", yamlContent) | ||
|
|
||
| err = cnf.BindStruct("", &dst) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Deliberate: both are verbatim pre-extraction behavior and this is a pure move. Handling the error would make unparseable yaml fail the startup instead of binding nothing; marked the ignore as deliberate in code. The other three findings are addressed.
There was a problem hiding this comment.
🔵 Needs a closer look
pkg/config/binder.BindSourcesToStructsFS passes &dst to BindStruct, which provides a *interface{} rather than the caller’s destination and can yield incorrect or confusing binding behavior.
Review details
Suppressed comments (1)
pkg/config/binder/binder.go:49
dstis already the destination provided by callers (typically a pointer to a struct). Passing&dsthere handsBindStructa*interface{}instead, which is inconsistent with other usages in the repo (e.g.services/proxy/pkg/middleware/security.go:54) and can lead to confusing behavior if a caller accidentally passes a non-pointer value.
err = cnf.BindStruct("", &dst)
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
New leaf package (only gookit + pkg/config/defaults) so callers can bind a yaml config file without importing the aggregate service config. pkg/config keeps a backward-compatible re-export.
4372c34 to
eba5e87
Compare
Move
BindSourcesToStructsinto a new leaf packagepkg/config/binder(depends only on gookit +pkg/config/defaults), so code that only needs to bind a yaml config file can import it without pulling in the aggregate service config inpkg/config.pkg/configkeeps a backward-compatible re-export, so existing callers are unchanged.Why
This lets third-party services, like my opencloud-music, adopt the exact same config pattern as OpenCloud itself (config structs + yaml/env binding) without pulling in the whole OpenCloud dependency tree.