Skip to content

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

Closed
wants to merge 7 commits into from

Conversation

gwlester
Copy link
Contributor

@gwlester gwlester commented Sep 30, 2021

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 #:

  • Migration process documented
  • Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 30, 2021
@gwlester gwlester changed the title bug(feature_flags): Fix for but 714 -- only evaluting first rule fix(feature_flags): Fix for but 714 -- only evaluting first rule Sep 30, 2021
@heitorlessa
Copy link
Contributor

cc @risenberg-cyberark I believe that's the expected behaviour in the original schema, no?

from the docs:

You can have multiple rules with different names. The rule engine will return the first result when_match of the matching rule configuration, or default value when none of the rules apply.

@heitorlessa
Copy link
Contributor

I'd take that back - Another part of the docs also says:

For multiple conditions, we will evaluate the list of conditions as a logical AND, so all conditions needs to match to return when_match value.

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.

@heitorlessa heitorlessa added the need-more-information Pending information to continue label Oct 1, 2021
@ran-isenberg
Copy link
Contributor

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.
Yep, it's the same silly indentation issue.
The docs seem ok to me.
Anyways, thx for the fix. It's correct.

@ran-isenberg
Copy link
Contributor

I think the pr should also fix a test for multiple rules where the second rule matches, not the first.

@heitorlessa
Copy link
Contributor

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. Yep, it's the same silly indentation issue. The docs seem ok to me. Anyways, thx for the fix. It's correct.

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

@gwlester
Copy link
Contributor Author

gwlester commented Oct 1, 2021

I remembered i solved a similar bug in the conditions about 1.5 months ago with the same indentation in the conditions checks: _evaluate_conditions. Yep, it's the same silly indentation issue. The docs seem ok to me. Anyways, thx for the fix. It's correct.

I remembered that but wasn't sure if it was fixed by pushing indentation 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.
multi-conditions are and. multi-rules are or

Personally, I think this is critical.

@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2021

Codecov Report

Merging #715 (85faa67) into develop (68e2c8e) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             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     
Impacted Files Coverage Δ
...owertools/utilities/feature_flags/feature_flags.py 100.00% <100.00%> (ø)
aws_lambda_powertools/logging/logger.py 98.43% <0.00%> (-1.57%) ⬇️
aws_lambda_powertools/metrics/metrics.py 100.00% <0.00%> (ø)
aws_lambda_powertools/shared/constants.py 100.00% <0.00%> (ø)
aws_lambda_powertools/utilities/batch/sqs.py 100.00% <0.00%> (ø)
aws_lambda_powertools/shared/jmespath_utils.py 100.00% <0.00%> (ø)
aws_lambda_powertools/utilities/parameters/ssm.py 100.00% <0.00%> (ø)
..._lambda_powertools/utilities/feature_flags/base.py 100.00% <0.00%> (ø)
..._lambda_powertools/utilities/parameters/secrets.py 100.00% <0.00%> (ø)
...lambda_powertools/utilities/parameters/dynamodb.py 100.00% <0.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68e2c8e...85faa67. Read the comment docs.

@ran-isenberg
Copy link
Contributor

I remembered i solved a similar bug in the conditions about 1.5 months ago with the same indentation in the conditions checks: _evaluate_conditions. Yep, it's the same silly indentation issue. The docs seem ok to me. Anyways, thx for the fix. It's correct.

I remembered that but wasn't sure if it was fixed by pushing indentation 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. multi-conditions are and. multi-rules are or

Personally, I think this is critical.

I agree

@ran-isenberg
Copy link
Contributor

I'm on it

@gwlester
Copy link
Contributor Author

gwlester commented Oct 1, 2021

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?

@gwlester
Copy link
Contributor Author

gwlester commented Oct 1, 2021

@risenberg-cyberark :

I think the pr should also fix a test for multiple rules where the second rule matches, not the first.

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.

@pull-request-size pull-request-size bot removed the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 1, 2021
@boring-cyborg boring-cyborg bot added documentation Improvements or additions to documentation tests labels Oct 1, 2021
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 1, 2021
@ran-isenberg
Copy link
Contributor

ran-isenberg commented Oct 1, 2021

it was working because the match value of the rule was the same as the default value of the feature.
When i change expected_value_second_check to be True and run the test. it fails, because it doesnt match on the second rule.
After your fix, the test works with expected_value_second_check = True or False, because it returns the correct value.
I'm creating a PR.
see #724

@heitorlessa see my PR 724

@ran-isenberg
Copy link
Contributor

@gwlester I think you pushed the commits to the wrong branch

@gwlester
Copy link
Contributor Author

gwlester commented Oct 1, 2021

@gwlester I think you pushed the commits to the wrong branch
No, pulled the changes into here when attempting to resolve conflicts -- so we close this PR and just go with yours since it fixes the test as well as the code, right?

@ran-isenberg
Copy link
Contributor

@gwlester I think you pushed the commits to the wrong branch
No, pulled the changes into here when attempting to resolve conflicts -- so we close this PR and just go with yours since it fixes the test as well as the code, right?

indeed.
I meant that you added the other new actions here.

@gwlester gwlester closed this Oct 1, 2021
@gwlester gwlester deleted the feature-714 branch January 31, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation need-more-information Pending information to continue size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants