feat: introduce common interface for feature gate#4401
Open
borosr wants to merge 9 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a FeatureGate interface and noop singleton default, then threads FeatureGate into billing worker construction and v3 API server configuration (defaults wired where callers don’t supply one). ChangesFeature Gate Infrastructure and Wiring
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
borosr
added
the
release-note/ignore
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/gatex/gatex.go`:
- Around line 26-43: The noopFeatureGate methods currently ignore the
caller-provided default and return hardcoded values; update EvaluateBool,
EvaluateInt, EvaluateFloat64, EvaluateString and EvaluateJSON to accept and
return the provided defaultValue parameter (i.e., name the second parameter like
defaultValue bool/int/float64/string/json.RawMessage) and return that
defaultValue with nil error instead of the hardcoded true/0/""/{}, so callers
receive their intended fallback when no real gate is configured.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0ed8e6f-7795-40f8-bca7-ec55eac3bf71
📒 Files selected for processing (5)
api/v3/server/server.goapp/common/openmeter_billingworker.goopenmeter/billing/worker/worker.goopenmeter/server/server.gopkg/gatex/gatex.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/gatex/gatex.go (1)
26-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNoopFeatureGate should return caller defaults, not hardcoded values.
Right now each method ignores the
defaultValueparameter and returns a constant (true,0,"",{}). That means callers can't control the fallback behavior when no real gate is configured. The interface signature promises thatdefaultValuewill be used, so the noop implementation should honor that.🔧 Return the provided defaults
-func (n NoopFeatureGate) EvaluateBool(string, bool) (bool, error) { - return true, nil +func (n NoopFeatureGate) EvaluateBool(_ string, defaultValue bool) (bool, error) { + return defaultValue, nil } -func (n NoopFeatureGate) EvaluateInt(string, int) (int, error) { - return 0, nil +func (n NoopFeatureGate) EvaluateInt(_ string, defaultValue int) (int, error) { + return defaultValue, nil } -func (n NoopFeatureGate) EvaluateFloat64(string, float64) (float64, error) { - return 0, nil +func (n NoopFeatureGate) EvaluateFloat64(_ string, defaultValue float64) (float64, error) { + return defaultValue, nil } -func (n NoopFeatureGate) EvaluateString(string, string) (string, error) { - return "", nil +func (n NoopFeatureGate) EvaluateString(_ string, defaultValue string) (string, error) { + return defaultValue, nil } -func (n NoopFeatureGate) EvaluateJSON(string, json.RawMessage) (json.RawMessage, error) { - return json.RawMessage(`{}`), nil +func (n NoopFeatureGate) EvaluateJSON(_ string, defaultValue json.RawMessage) (json.RawMessage, error) { + return slices.Clone(defaultValue), nil }Note: For
EvaluateJSON, cloning the slice prevents accidental mutation of the caller's buffer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gatex/gatex.go` around lines 26 - 44, The NoopFeatureGate methods currently ignore the provided defaultValue and return hardcoded constants; update EvaluateBool, EvaluateInt, EvaluateFloat64, and EvaluateString to return the passed-in defaultValue and nil error (preserving the method signatures), and change EvaluateJSON to return a cloned copy of the provided json.RawMessage defaultValue (to avoid sharing/mutation) along with nil error; adjust references to the parameters in the functions EvaluateBool, EvaluateInt, EvaluateFloat64, EvaluateString, and EvaluateJSON accordingly so callers receive their supplied defaults.
🧹 Nitpick comments (2)
pkg/gatex/gatex.go (1)
18-20: 💤 Low valueRedundant nil check in GetDefault().
Since
instanceis already initialized toNoopFeatureGate{}on line 15, it will never be nil. You can simplify this to justreturn instance.♻️ Simplify the singleton getter
func GetDefault() FeatureGate { - if instance == nil { - instance = NoopFeatureGate{} - } return instance }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gatex/gatex.go` around lines 18 - 20, The GetDefault() function contains a redundant nil check for the package-level variable instance which is already initialized to NoopFeatureGate{}; remove the conditional block that checks "if instance == nil { instance = NoopFeatureGate{} }" and simply return instance from GetDefault(); reference the symbols instance, GetDefault, and NoopFeatureGate when updating the function.cmd/billing-worker/wire.go (1)
58-59: 💤 Low valueConsider using wire.Value or a provider function for the zero-value struct.
wire.Structworks fine here, but it's a bit unusual for a struct with no fields. You could simplify towire.Value(gatex.NoopFeatureGate{})or—even better—align with the singleton pattern by usinggatex.GetDefaultas a provider. That way ifGetDefault()logic changes, Wire automatically picks it up.♻️ Alternative Wire patterns
Option 1: Use wire.Value
- wire.Struct(new(gatex.NoopFeatureGate)), + wire.Value(gatex.NoopFeatureGate{}), wire.Bind(new(gatex.FeatureGate), new(gatex.NoopFeatureGate)),Option 2: Use the singleton (recommended)
- wire.Struct(new(gatex.NoopFeatureGate)), - wire.Bind(new(gatex.FeatureGate), new(gatex.NoopFeatureGate)), + gatex.GetDefault,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/billing-worker/wire.go` around lines 58 - 59, The current binding uses wire.Struct(new(gatex.NoopFeatureGate)) for a zero-value struct; replace it with a clearer pattern—either use wire.Value(gatex.NoopFeatureGate{}) or (preferred) register the singleton provider gatex.GetDefault as a provider so Wire will use that implementation automatically; keep the existing wire.Bind(new(gatex.FeatureGate), ...) entry so the NoopFeatureGate still satisfies gatex.FeatureGate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@pkg/gatex/gatex.go`:
- Around line 26-44: The NoopFeatureGate methods currently ignore the provided
defaultValue and return hardcoded constants; update EvaluateBool, EvaluateInt,
EvaluateFloat64, and EvaluateString to return the passed-in defaultValue and nil
error (preserving the method signatures), and change EvaluateJSON to return a
cloned copy of the provided json.RawMessage defaultValue (to avoid
sharing/mutation) along with nil error; adjust references to the parameters in
the functions EvaluateBool, EvaluateInt, EvaluateFloat64, EvaluateString, and
EvaluateJSON accordingly so callers receive their supplied defaults.
---
Nitpick comments:
In `@cmd/billing-worker/wire.go`:
- Around line 58-59: The current binding uses
wire.Struct(new(gatex.NoopFeatureGate)) for a zero-value struct; replace it with
a clearer pattern—either use wire.Value(gatex.NoopFeatureGate{}) or (preferred)
register the singleton provider gatex.GetDefault as a provider so Wire will use
that implementation automatically; keep the existing
wire.Bind(new(gatex.FeatureGate), ...) entry so the NoopFeatureGate still
satisfies gatex.FeatureGate.
In `@pkg/gatex/gatex.go`:
- Around line 18-20: The GetDefault() function contains a redundant nil check
for the package-level variable instance which is already initialized to
NoopFeatureGate{}; remove the conditional block that checks "if instance == nil
{ instance = NoopFeatureGate{} }" and simply return instance from GetDefault();
reference the symbols instance, GetDefault, and NoopFeatureGate when updating
the function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b45179c-af01-4128-8e4d-ed33670e9ae8
📒 Files selected for processing (3)
cmd/billing-worker/wire.gocmd/billing-worker/wire_gen.gopkg/gatex/gatex.go
✅ Files skipped from review due to trivial changes (1)
- cmd/billing-worker/wire_gen.go
turip
previously approved these changes
May 20, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/server/server.go (1)
144-144: ⚡ Quick winAvoid hardcoding the no-op gate in the server constructor.
Line 144 always injects
gatex.GetDefault(), which blocks passing a real gate through this path when needed.💡 Suggested wiring tweak
type Config struct { RouterConfig router.Config RouterHooks RouterHooks PostAuthMiddlewares PostAuthMiddlewares ResponseValidation appconfig.ResponseValidationConfig + FeatureGate gatex.FeatureGate } func NewServer(config *Config) (*Server, error) { + featureGate := config.FeatureGate + if featureGate == nil { + featureGate = gatex.GetDefault() + } + v3API, err := v3server.NewServer(&v3server.Config{ ... - FeatureGate: gatex.GetDefault(), + FeatureGate: featureGate, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/server/server.go` at line 144, The constructor currently hardcodes FeatureGate to gatex.GetDefault(), preventing callers from injecting a real gate; update the server creation path that sets FeatureGate (the struct field named FeatureGate and the call to gatex.GetDefault()) so it accepts an injected gate parameter (e.g., add a gate parameter to the NewServer/New* constructor or a functional option) and use the provided gate when non-nil, falling back to gatex.GetDefault() only if no gate was passed; ensure references to FeatureGate in the constructor and any related config struct are updated to use the injected value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openmeter/server/server.go`:
- Line 144: The constructor currently hardcodes FeatureGate to
gatex.GetDefault(), preventing callers from injecting a real gate; update the
server creation path that sets FeatureGate (the struct field named FeatureGate
and the call to gatex.GetDefault()) so it accepts an injected gate parameter
(e.g., add a gate parameter to the NewServer/New* constructor or a functional
option) and use the provided gate when non-nil, falling back to
gatex.GetDefault() only if no gate was passed; ensure references to FeatureGate
in the constructor and any related config struct are updated to use the injected
value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b8f4048-7cd5-4231-86f8-81232d2cacc5
📒 Files selected for processing (7)
api/v3/server/server.goapp/common/openmeter_billingworker.gocmd/billing-worker/wire.gocmd/billing-worker/wire_gen.goopenmeter/billing/worker/worker.goopenmeter/server/server.gopkg/gatex/gatex.go
✅ Files skipped from review due to trivial changes (1)
- cmd/billing-worker/wire_gen.go
Contributor
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)
pkg/gatex/gatex.go (1)
41-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnnecessary nil check for struct value.
Since
instanceis initialized toNoopFeatureGate{}(a struct value, not a pointer), it can never benil. This check will always evaluate to false.🧹 Suggested simplification
func GetDefault() FeatureGate { - if instance == nil { - instance = NoopFeatureGate{} - } return instance }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gatex/gatex.go` around lines 41 - 43, The nil check on the package-level variable `instance` is redundant because `instance` is initialized as a non-pointer `NoopFeatureGate{}` value and can never be nil; remove the conditional block that checks `if instance == nil { instance = NoopFeatureGate{} }` and leave the existing initialization of `instance` intact so `instance` is always the default `NoopFeatureGate{}` value without the unreachable nil-check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/gatex/gatex.go`:
- Around line 41-43: The nil check on the package-level variable `instance` is
redundant because `instance` is initialized as a non-pointer `NoopFeatureGate{}`
value and can never be nil; remove the conditional block that checks `if
instance == nil { instance = NoopFeatureGate{} }` and leave the existing
initialization of `instance` intact so `instance` is always the default
`NoopFeatureGate{}` value without the unreachable nil-check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1a7557f8-7fb8-48d1-949c-a1f9b9835328
📒 Files selected for processing (1)
pkg/gatex/gatex.go
turip
reviewed
May 21, 2026
turip
reviewed
May 21, 2026
chrisgacsal
previously approved these changes
May 21, 2026
chrisgacsal
previously approved these changes
May 21, 2026
turip
approved these changes
May 21, 2026
chrisgacsal
approved these changes
May 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Introduce common interface to handle feature gate requests.
Why
We want to control specific cases using feature gate.
Summary by CodeRabbit