docs: add disabled flag behaviour#1919
Conversation
✅ Deploy Preview for polite-licorice-3db33c ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
dosubot
bot
added
the
size:L
![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 introduces an Architecture Decision Record (ADR) to change flagd's handling of disabled flags. Instead of returning an error, disabled flags will now return a successful evaluation with a reason of DISABLED and the defaultVariant value, aligning with the OpenFeature specification. Feedback on the ADR highlights several technical implementation details that need to be addressed: the logic in evaluateVariant should be simplified, and the resolve[T] function must be updated to handle DISABLED reasons for empty variants to avoid type mismatch errors. Additionally, the proposal needs to account for gRPC v1 compatibility in bulk evaluations and include specific implementation steps in the timeline for the gRPC and OFREP service layers.
| if flag.State == Disabled { | ||
| if flag.DefaultVariant == "" { | ||
| return "", flag.Variants, model.DisabledReason, metadata, nil | ||
| } | ||
| return flag.DefaultVariant, flag.Variants, model.DisabledReason, metadata, nil | ||
| } |
There was a problem hiding this comment.
The proposed logic for evaluateVariant can be simplified as flag.DefaultVariant already contains the empty string when no default variant is defined. This aligns with the architectural goal of simplifying implementations when complexity arises from misconfiguration prevention. Additionally, there is a technical issue with this proposal: the resolve[T] function in core/pkg/evaluator/json.go (lines 310-330) only handles model.FallbackReason as a special case for empty variants. For any other reason, including model.DisabledReason, it attempts a lookup in the variants map (line 324), which will result in a TYPE_MISMATCH error if the variant is empty. To support the field-omission pattern for disabled flags, resolve[T] must be updated to treat model.DisabledReason similarly to model.FallbackReason when the variant is empty.
| if flag.State == Disabled { | |
| if flag.DefaultVariant == "" { | |
| return "", flag.Variants, model.DisabledReason, metadata, nil | |
| } | |
| return flag.DefaultVariant, flag.Variants, model.DisabledReason, metadata, nil | |
| } | |
| if flag.State == Disabled { | |
| return flag.DefaultVariant, flag.Variants, model.DisabledReason, metadata, nil | |
| } |
References
- When a feature designed to prevent user misconfiguration introduces significant implementation complexity, re-evaluate if a simpler architectural approach can solve the problem more elegantly.
| // After: remove the skip — disabled flags flow through normal evaluation | ||
| // and will be returned with reason=DISABLED | ||
| ``` |
There was a problem hiding this comment.
Removing the skip in ResolveAllValues is insufficient for gRPC v1 compatibility. Given our support window covers the latest and two previous minor releases, ensuring compatibility for active versions is essential. When defaultVariant is null, the type switch on defaultValue (line 191 in core/pkg/evaluator/json.go) hits the default case and continues if ProtoVersionKey is set (which it is for gRPC v1). The logic needs to be adjusted to ensure disabled flags are processed regardless of the defaultVariant type to meet the requirement of including them in bulk evaluations.
References
- The support window for flagd components covers the latest minor release and the two previous minor releases, for a total of three supported minor versions.
| 1. Update `evaluateVariant` in `core/pkg/evaluator/json.go` to return `reason=DISABLED` with `defaultVariant` instead of an error | ||
| 2. Remove the disabled-flag skip in `ResolveAllValues` | ||
| 3. Remove `FlagDisabledErrorCode` handling from `errFormat`, `errFormatV2`, and `EvaluationErrorResponseFrom` | ||
| 4. Update flagd-testbed with test cases for disabled flag evaluation across all surfaces | ||
| 5. Update OpenFeature providers to recognize `DISABLED` as a non-error, non-cacheable reason | ||
| 6. Update provider documentation and migration guides |
There was a problem hiding this comment.
The timeline is missing several critical implementation steps required to achieve the proposed behavior:
- Update resolve[T]: Modify core/pkg/evaluator/json.go to handle model.DisabledReason for empty variants to avoid TYPE_MISMATCH errors.
- Update gRPC v1 Service Layer: Update flag_evaluator_v1.go to handle nil values in ResolveAll so flags deferring to code defaults are included in the response map.
- Update OFREP Success Mapping: Update core/pkg/service/ofrep/models.go to ensure the reason remains DISABLED (not DEFAULT) when a disabled flag defers to code defaults.
suthar26
force-pushed
the
docs/adr-disabled-flag-evaluation
branch
2 times, most recently
from
acff81c to
e7c0c39
Compare
Signed-off-by: Parth Suthar <parth.suthar@dynatrace.com>
suthar26
force-pushed
the
docs/adr-disabled-flag-evaluation
branch
from
e7c0c39 to
bfa06df
Compare
|
1 participant
This PR
Related Issues
Fixes #1918
Notes
Follow-up Tasks
How to test