UN-3291 [FIX] Migrate Flipt v1 to v2 and fix gRPC client bugs for feature flags#1808
Open
muhammad-ali-e wants to merge 2 commits intomainfrom
Open
UN-3291 [FIX] Migrate Flipt v1 to v2 and fix gRPC client bugs for feature flags#1808muhammad-ali-e wants to merge 2 commits intomainfrom
muhammad-ali-e wants to merge 2 commits intomainfrom
Conversation
…ture flags Migrate Docker dev essentials Flipt from v1.34.0 to v2.3.1 with file-based storage, and fix three gRPC client bugs introduced in PR #1665 that caused all feature flag evaluations to silently return False. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Summary by CodeRabbit
WalkthroughFlipt service image upgraded to v2.3.1 with env config removed, persistent volume added, and Traefik port label set. Flag evaluation now reads Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
muhammad-ali-e
requested review from
Deepak-Kesavan,
jagadeeswaran-zipstack,
johnyrahul,
pk-zipstack,
ritwik-g and
vishnuszipstack
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
unstract/flags/src/unstract/flags/feature_flag.py (2)
50-55: Unusednamespace_keyparameter.The
namespace_keyparameter is declared but never used in the function body. TheFliptClientreads the namespace from theUNSTRACT_FEATURE_FLAG_NAMESPACEenvironment variable internally.For consistency with
check_feature_flag_status(which also has this parameter), consider either:
- Removing it entirely if not needed for API compatibility
- Adding a deprecation warning like in
FliptClient.evaluate_variant()♻️ Option: Add deprecation warning for consistency
def check_feature_flag_variant( flag_key: str, namespace_key: str | None = None, entity_id: str = "unstract", context: dict[str, str] | None = None, ) -> dict: ... """ default_result = { "enabled": False, "match": False, "variant_key": "", "variant_attachment": "", "segment_keys": [], } + if namespace_key is not None: + import warnings + warnings.warn( + "namespace_key parameter is deprecated and will be ignored", + DeprecationWarning, + stacklevel=2, + ) try: client = FliptClient()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@unstract/flags/src/unstract/flags/feature_flag.py` around lines 50 - 55, Function check_feature_flag_variant declares namespace_key but never uses it; either remove the parameter for simplicity or mirror check_feature_flag_status by emitting a deprecation warning and ignoring the value. Update check_feature_flag_variant to: if you keep the parameter, import warnings and call warnings.warn(...) (match the message and deprecation style used in FliptClient.evaluate_variant), then proceed without using namespace_key; otherwise remove namespace_key from the signature and all callers. Reference check_feature_flag_variant and FliptClient.evaluate_variant (and check_feature_flag_status for consistency) when making the change.
45-45: Correct fix, butbool()wrapper is redundant.Since
FliptClient.evaluate_boolean()now returns abooldirectly (viaresult.enabled if hasattr(result, "enabled") else False), thebool()wrapper is unnecessary.♻️ Simplify by returning result directly
- return bool(result) + return result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@unstract/flags/src/unstract/flags/feature_flag.py` at line 45, The return uses an unnecessary bool() wrapper — since FliptClient.evaluate_boolean() already returns a bool, replace the redundant "return bool(result)" with "return result" in the method in feature_flag.py (locate the function that currently returns bool(result), e.g., the FeatureFlag or is_enabled wrapper that calls FliptClient.evaluate_boolean()) so the boolean is returned directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@unstract/flags/src/unstract/flags/feature_flag.py`:
- Around line 50-55: Function check_feature_flag_variant declares namespace_key
but never uses it; either remove the parameter for simplicity or mirror
check_feature_flag_status by emitting a deprecation warning and ignoring the
value. Update check_feature_flag_variant to: if you keep the parameter, import
warnings and call warnings.warn(...) (match the message and deprecation style
used in FliptClient.evaluate_variant), then proceed without using namespace_key;
otherwise remove namespace_key from the signature and all callers. Reference
check_feature_flag_variant and FliptClient.evaluate_variant (and
check_feature_flag_status for consistency) when making the change.
- Line 45: The return uses an unnecessary bool() wrapper — since
FliptClient.evaluate_boolean() already returns a bool, replace the redundant
"return bool(result)" with "return result" in the method in feature_flag.py
(locate the function that currently returns bool(result), e.g., the FeatureFlag
or is_enabled wrapper that calls FliptClient.evaluate_boolean()) so the boolean
is returned directly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
unstract/flags/src/unstract/flags/client/flipt.pyunstract/flags/src/unstract/flags/feature_flag.py
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
1 participant
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
FliptClient.evaluate_boolean()accessing non-existentresult.value(should beresult.enabled)check_feature_flag_status()calling.enabledon aboolreturn valuetimestamp_pb2in flipt_grpc package to prevent protobuf descriptor errors in workerscheck_feature_flag_variant()with full docstring for variant flag evaluationevaluate_variant()andlist_feature_flags()methods toFliptClientWhy
Falsetimestamp_pb2import that backend gets from Google Cloud librariesHow
flipt.py: Changedresult.value→result.enabledto match theBooleanEvaluationResponseprotobuf fieldflipt.py: Addedevaluate_variant()method that calls Flipt'sVariantEvaluationRequestgRPC endpointflipt.py: Addedlist_feature_flags()method that callsListFlagRequestgRPC endpoint to check flag existence/statusfeature_flag.py: Changedbool(result.enabled)→bool(result)sinceevaluate_boolean()already returns aboolfeature_flag.py: Addedcheck_feature_flag_variant()— checks flag is enabled vialist_feature_flags(), then evaluates variant and returns match/variant_key/variant_attachment/segment_keysflipt_grpc/__init__.py: Addedfrom google.protobuf import timestamp_pb2to pre-register the well-known typedocker-compose-dev-essentials.yaml: Updated image todocker.flipt.io/flipt/flipt:v2.3.1, replaced env-based DB with volume mountsample.essentials.env: RemovedFLIPT_DB_URL(no longer needed in v2)Can this PR break any existing features. If yes, please list possible items. If no, please explain why.
timestamp_pb2pre-registration is a no-op if already loadedcheck_feature_flag_variant()is additive; no existing callers are affectedDatabase Migrations
Env Config
FLIPT_DB_URLfromsample.essentials.env(Flipt v2 uses file-based storage)Related Issues or PRs
Dependencies Versions
Notes on Testing
False)check_feature_flag_variant()with a configured variant flag, segments, and rulesChecklist
I have read and understood the Contribution Guidelines.