feat!: DISABLED is a successful evaluation (still defaults)#1968
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
✅ Deploy Preview for polite-licorice-3db33c canceled.
|
6be120a to
f2d6817
Compare
There was a problem hiding this comment.
Pull request overview
This PR changes disabled flag evaluation from an error path to a successful evaluation with reason=DISABLED, aligning evaluator, gRPC, and OFREP behavior with the linked ADR and issue.
Changes:
- Removes
FLAG_DISABLEDas a model error code and updates evaluator logic to returnDISABLEDwithout value/variant. - Includes disabled flags in bulk evaluation responses and adds reason-only handling for gRPC v2.
- Updates OFREP success/error mapping and adjusts evaluator/service tests for the new behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
core/pkg/evaluator/json.go |
Converts disabled evaluation to successful DISABLED results and includes disabled flags in bulk evaluation. |
core/pkg/evaluator/json_test.go |
Updates evaluator expectations for disabled flags. |
core/pkg/model/error.go |
Removes the disabled error code and readable message. |
core/pkg/service/ofrep/models.go |
Returns disabled evaluations as OFREP success responses. |
core/pkg/service/ofrep/models_test.go |
Removes old disabled-error status expectation. |
flagd/pkg/service/flag-evaluation/flag_evaluator.go |
Adds disabled handling to legacy bulk responses and updates error formatting. |
flagd/pkg/service/flag-evaluation/flag_evaluator_v1.go |
Adds disabled handling to v1 bulk responses. |
flagd/pkg/service/flag-evaluation/flag_evaluator_v1_test.go |
Removes old disabled error-code test case. |
flagd/pkg/service/flag-evaluation/flag_evaluator_test.go |
Removes old disabled error-code/readable-message test cases. |
flagd/pkg/service/flag-evaluation/flag_evaluator_v2.go |
Refactors v2 response setting and adds reason-only disabled handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return evalErrFormatted | ||
| } | ||
|
|
||
| func applyResolveV2Response[T constraints]( |
There was a problem hiding this comment.
I generally favour the "rule of three" for duplication: with only two cases of the pattern, inline duplication wins on readability, especially with these method extractions.
099468e to
102f56f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of disabled flags across the evaluator, OFREP service, and flag evaluation services. Instead of returning a FLAG_DISABLED error, disabled flags now return a DisabledReason reason with omitted or zero values. Feedback on these changes includes a recommendation to populate Metadata for disabled flags in the ResolveAll response to prevent missing contextual information, and a correction to the expected JSON in BenchmarkResolveObjectValue to prevent benchmark failures.
Signed-off-by: Parth Suthar <parth.suthar@dynatrace.com>
Signed-off-by: Parth Suthar <parth.suthar@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
102f56f to
e79a26f
Compare
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
…chmark expectation Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
v1 (and the legacy/Old service) use proto3 scalar fields that can't omit Value on the wire. Single-flag already emits zero values alongside reason=DISABLED; making bulk specially-omit Value via the oneof was inconsistent with the single-flag behavior and added complexity. Just let bulk include disabled flags with zero values + reason=DISABLED, matching single-flag. v2 and OFREP retain proper omission (they have the means to omit). Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
|
@suthar26 v1 RPC is deprecated and basically unused, and it's not really compatible with this change I've reverted those. |
Leave v1/legacy paths fully untouched; v2/core/OFREP no longer reference this constant but it's harmless to keep around. Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of disabled feature flags. Instead of treating disabled flags as errors with a FlagDisabledErrorCode, they are now treated as successful evaluations that return a specific DisabledReason with nil/empty values and no error. This change allows disabled flags to be included in bulk evaluations and properly handled in OFREP and flagd v2 services. Corresponding unit tests and benchmarks have been updated to reflect this new behavior. I have no additional feedback to provide as there are no review comments to assess.
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
beeme1mr
left a comment
There was a problem hiding this comment.
Please address the relevant code quality warnings. The rest of the change looks good.
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
a65d435 to
bcf90a6
Compare
|
Fixed them, just the patch duplication warning remains, which I haven't found a way to relax (and I disagree with in this case). |
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
🤖 I have created a release *beep* *boop* --- <details><summary>flagd: 0.16.0</summary> ## [0.16.0](flagd/v0.15.7...flagd/v0.16.0) (2026-06-01) ### ⚠ BREAKING CHANGES * DISABLED is a successful evaluation (still defaults) ([#1968](#1968)) * Disabled flags now resolve successfully with `reason=DISABLED` instead of returning a `FLAG_DISABLED` error. See the [ADR](https://github.com/open-feature/flagd/blob/main/docs/architecture-decisions/disabled-flag-evaluation.md). * Breaking, but barely: resolved values are unchanged (SDKs still surface the caller-provided default). * The break is only observable for consumers that inspect `reason` / `errorCode`, call flagd directly over gRPC / OFREP, or import `core/pkg/model`. ### ✨ New Features * DISABLED is a successful evaluation (still defaults) ([#1968](#1968)) ([3c1d00b](3c1d00b)) * Disabled flags now resolve successfully with `reason=DISABLED` instead of returning a `FLAG_DISABLED` error. See the [ADR](https://github.com/open-feature/flagd/blob/main/docs/architecture-decisions/disabled-flag-evaluation.md). * Breaking, but barely: resolved values are unchanged (SDKs still surface the caller-provided default). * The break is only observable for consumers that inspect `reason` / `errorCode`, call flagd directly over gRPC / OFREP, or import `core/pkg/model`. </details> <details><summary>core: 0.16.0</summary> ## [0.16.0](core/v0.15.8...core/v0.16.0) (2026-06-01) ### ⚠ BREAKING CHANGES * DISABLED is a successful evaluation (still defaults) ([#1968](#1968)) * Disabled flags now resolve successfully with `reason=DISABLED` instead of returning a `FLAG_DISABLED` error. See the [ADR](https://github.com/open-feature/flagd/blob/main/docs/architecture-decisions/disabled-flag-evaluation.md). * Breaking, but barely: resolved values are unchanged (SDKs still surface the caller-provided default). * The break is only observable for consumers that inspect `reason` / `errorCode`, call flagd directly over gRPC / OFREP, or import `core/pkg/model`. ### ✨ New Features * DISABLED is a successful evaluation (still defaults) ([#1968](#1968)) ([3c1d00b](3c1d00b)) * Disabled flags now resolve successfully with `reason=DISABLED` instead of returning a `FLAG_DISABLED` error. See the [ADR](https://github.com/open-feature/flagd/blob/main/docs/architecture-decisions/disabled-flag-evaluation.md). * Breaking, but barely: resolved values are unchanged (SDKs still surface the caller-provided default). * The break is only observable for consumers that inspect `reason` / `errorCode`, call flagd directly over gRPC / OFREP, or import `core/pkg/model`. </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com> Signed-off-by: Todd Baert <todd.baert@dynatrace.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Todd Baert <todd.baert@dynatrace.com>


reason=DISABLEDinstead of returning aFLAG_DISABLEDerror. See the ADR.reason/errorCode, call flagd directly over gRPC / OFREP, or importcore/pkg/model.Related Issues
Fixes #1966
Part of #1965