-
Notifications
You must be signed in to change notification settings - Fork 421
fix(feature_flags): Fix for but 714 -- only evaluting first rule #715
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
cc @risenberg-cyberark I believe that's the expected behaviour in the original schema, no? from the docs:
|
I'd take that back - Another part of the docs also says:
I'd defer to @risenberg-cyberark who did the original schema to confirm before proceeding here, as we'll need remove ambiguity from docs too thereafter. |
I remembered i solved a similar bug in the conditions about 1.5 months ago with the same indendation in the conditions checks: _evaluate_conditions. |
I think the pr should also fix a test for multiple rules where the second rule matches, not the first. |
I remembered that but wasn't sure if was fixed by pushing identation or pulling it. Either way, if you have the bandwidth for fixing the test and merge conflict by Monday, I can include it in Tue/Wed release as it's an important one |
The code I pushed allows multi-rules to work as well as multi-conditions. Personally, I think this is critical. |
Codecov Report
@@ Coverage Diff @@
## develop #715 +/- ##
===========================================
- Coverage 99.97% 99.93% -0.05%
===========================================
Files 116 116
Lines 4866 4901 +35
Branches 267 271 +4
===========================================
+ Hits 4865 4898 +33
- Misses 0 1 +1
- Partials 1 2 +1
Continue to review full report at Codecov.
|
I agree |
I'm on it |
Ok, a dumb question -- the coverage report says this decreased the coverage by 0.04%-- but the only file touched (...owertools/utilities/feature_flags/feature_flags.py) is at 100% coverage! What gives? |
@risenberg-cyberark :
Unless I'm misreading it, that is what test_flags_conditions_rule_match_multiple_actions_multiple_rules_multiple_conditions is supposed to do with: # match second rule
toggle = feature_flags.evaluate(name="my_feature", context={"tenant_id": "4446", "username": "az"}, default=False) I have no idea how this test was passing before. |
it was working because the match value of the rule was the same as the default value of the feature. @heitorlessa see my PR 724 |
@gwlester I think you pushed the commits to the wrong branch |
|
indeed. |
Issue #, if available: #714
Description of changes:
Corrected indentation in evaluate rules so that it will only return the default after all rules are evaluated.
Checklist
Breaking change checklist
RFC issue #:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.