fix(core): preserve backward-compatible Loader.Watch interface#3313
fix(core): preserve backward-compatible Loader.Watch interface#3313pflynn-virtru wants to merge 3 commits intomainfrom
Conversation
The Loader.Watch method signature was changed to include a namespaces parameter, breaking downstream consumers that implement the interface. Revert Watch to the original signature and introduce a separate NamespaceAwareLoader interface for loaders that need namespace info. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe changes refactor the config loader architecture by extracting namespace-aware watching into a separate interface. The base Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
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📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
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 restores backward compatibility for the Loader.Watch interface by decoupling it from namespace-specific requirements. It introduces a new interface for loaders that explicitly need namespace context, ensuring that existing implementations remain functional while providing a clean path for future extensions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 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 counter productive. 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 interface was wide and deep, With parameters we could not keep. We split the path in two, To keep the old code true, And let the new logic leap. 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 loading system by introducing a NamespaceAwareLoader interface and simplifying the base Loader interface's Watch method. The Watch method in the Loader interface no longer requires namespace information, while loaders needing that context now implement NamespaceAwareLoader.WatchWithNamespaces. The core configuration logic was updated to use type assertions to handle both loader types, and all existing loader implementations and tests were updated accordingly. I have no feedback to provide.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
No in-tree loader implemented NamespaceAwareLoader, and the namespace info plumbed through WatchWithNamespaces was never consumed. Remove the interface, NamespaceInfo/ServiceInfo types, and the WatchWithNamespaces entry point; callers now use the simpler Loader.Watch path directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
This reverts commit 54b2b92.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
service/pkg/config/config_test.go (1)
171-195:⚠️ Potential issue | 🟡 MinorAdd a
Config.Watch(ctx)test for namespace-aware loaders receiving an empty namespace list.Current coverage validates
WatchWithNamespaceswith explicit namespaces, but it does not lock the documented behavior thatWatch(ctx)routes namespace-aware loaders with an empty slice.Proposed test addition
func TestConfig_Watch(t *testing.T) { ctx := t.Context() @@ t.Run("Loader receives NamespaceInfo", func(t *testing.T) { config := &Config{} loader := &MockNamespaceAwareLoader{} @@ require.NoError(t, err) assert.Equal(t, testNamespaces, receivedNamespaces) }) + + t.Run("NamespaceAwareLoader via Watch receives empty namespaces", func(t *testing.T) { + config := &Config{} + loader := &MockNamespaceAwareLoader{} + + var receivedNamespaces []NamespaceInfo + loader.watchWithNamespacesFn = func(_ context.Context, _ *Config, _ func(context.Context) error, namespaces []NamespaceInfo) error { + receivedNamespaces = namespaces + return nil + } + config.AddLoader(loader) + + err := config.Watch(ctx) + require.NoError(t, err) + assert.Empty(t, receivedNamespaces) + }) }As per coding guidelines "
**/*_test.go: ... add tests for new functionality".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/pkg/config/config_test.go` around lines 171 - 195, Add a test that verifies Config.Watch(ctx) routes namespace-aware loaders with an empty namespace slice: create a Config and a MockNamespaceAwareLoader, set its watchWithNamespacesFn to capture the namespaces argument into receivedNamespaces, call config.AddLoader(loader) and then err := config.Watch(ctx), assert no error and that receivedNamespaces is an empty slice (i.e., []NamespaceInfo{}). Reference Config.Watch, MockNamespaceAwareLoader, and the mock's watchWithNamespacesFn to locate where to hook the capture.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@service/pkg/config/config_test.go`:
- Around line 171-195: Add a test that verifies Config.Watch(ctx) routes
namespace-aware loaders with an empty namespace slice: create a Config and a
MockNamespaceAwareLoader, set its watchWithNamespacesFn to capture the
namespaces argument into receivedNamespaces, call config.AddLoader(loader) and
then err := config.Watch(ctx), assert no error and that receivedNamespaces is an
empty slice (i.e., []NamespaceInfo{}). Reference Config.Watch,
MockNamespaceAwareLoader, and the mock's watchWithNamespacesFn to locate where
to hook the capture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ffc8e11-f4f7-40ff-ba9d-956672896106
📒 Files selected for processing (7)
service/pkg/config/config.goservice/pkg/config/config_file_loader.goservice/pkg/config/config_test.goservice/pkg/config/default_settings_loader.goservice/pkg/config/environment_value_loader.goservice/pkg/config/legacy_loader.goservice/pkg/config/loader.go
ntrevino-virtru
left a comment
ntrevino-virtru
left a comment
There was a problem hiding this comment.
Should we consider a standard Go pattern with variadic arguments so we do not keep hitting this?
| } | ||
| if err := loader.Watch(ctx, c, onChangeCallback); err != nil { |
There was a problem hiding this comment.
I think this should be else if
2 participants
Summary
Loader.Watchto its original 3-arg signature(ctx, cfg, onChange)to maintain backward compatibility for downstream consumers that implementLoaderdirectlyNamespaceAwareLoaderinterface for loaders that want namespace/service information during watch, added viaWatchWithNamespaces(ctx, cfg, onChange, namespaces)Config.WatchWithNamespacestype-asserts each loader and routes toWatchWithNamespaceswhen the loader opts in, otherwise falls back to plainWatchGuidance for downstream implementers
If your custom loader does not need namespace/service info, implement
config.Loaderas before — no change required.If your custom loader wants namespace/service info (e.g. a remote config source scoped to enabled services), implement
config.NamespaceAwareLoader:Callers that need namespace-aware behavior must invoke
cfg.WatchWithNamespaces(ctx, namespaces)—cfg.Watch(ctx)passes an empty slice and will not deliver namespace info toNamespaceAwareLoaderimplementers.Test plan
go build ./service/...passesgo test ./service/pkg/config/...passes🤖 Generated with Claude Code
Summary by CodeRabbit