Skip to content

refactor(feature-flags): add intersection tests; structure refinement #3775

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

Merged

Conversation

heitorlessa
Copy link
Contributor

@heitorlessa heitorlessa commented Feb 15, 2024

Issue number: #3531

Summary

Adds remaining changes before releasing new intersection rule actions ANY_IN_VALUE, ALL_IN_VALUE, NONE_IN_VALUE.

Changes

Please provide a summary of what's being changed

  • Test new operators
    • ANY_IN_VALUE
    • ALL_IN_VALUE
    • NONE_IN_VALUE
  • Use context_value and condition_value fn arguments
  • Static type function arguments
  • Docstring functions
  • Split context vs schema value validation
    • Move condition value validation under Schema Validator
    • Raise ValueError for non-list context value
  • Test invalid schema values
    • ANY_IN_VALUE
    • ALL_IN_VALUE
    • NONE_IN_VALUE
  • Add Exception Handler

Unrelated

  • Load rule action mapping once (we're defining lambda actions on every evaluation call)
  • Refactor if/elif/elif with a simple dispatcher (getattr)
    • Validator noop
    • Validator getattr
    • Validator days of the week
    • Validator modulo range
    • Validator time value
    • Validator datetime value
    • Get rid of redundant validations in time/datetime value
    • Decouple exact exception message in test but specifics
    • Condition keys
  • Add docstrings and return to dispatchers
  • Add docstring on end-to-end flow

User experience

Please share what the user experience looks like before and after this change

ALL_IN_VALUE

context=["Gerald"]

{
    "my_feature": {
        "default": false,
        "rules": {
            "tenant_id is in allowed list": {
                "when_match": true,
                "conditions": [
                    {
                        "action": "ALL_IN_VALUE",
                        "key": "tenant_id",
                        "value": [
                            "Łukasz",
                            "Gerald",
                            "Leandro",
                            "Heitor"
                        ]
                    }
                ]
            }
        }
    }
}

ANY_IN_VALUE

context=["Gerald"]

{
    "my_feature": {
        "default": false,
        "rules": {
            "tenant_id is in allowed list": {
                "when_match": true,
                "conditions": [
                    {
                        "action": "ANY_IN_VALUE",
                        "key": "tenant_id",
                        "value": [
                            "Łukasz",
                            "Gerald",
                            "Leandro",
                            "Heitor"
                        ]
                    }
                ]
            }
        }
    }
}

NONE_IN_VALUE

context=["Simon"]

{
    "my_feature": {
        "default": false,
        "rules": {
            "tenant_id is in allowed list": {
                "when_match": true,
                "conditions": [
                    {
                        "action": "NONE_IN_VALUE",
                        "key": "tenant_id",
                        "value": [
                            "Łukasz",
                            "Gerald",
                            "Leandro",
                            "Heitor"
                        ]
                    }
                ]
            }
        }
    }
}

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

RFC issue number:

Checklist:

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

Acknowledgment

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

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@heitorlessa heitorlessa requested a review from a team February 15, 2024 13:10
@boring-cyborg boring-cyborg bot added the tests label Feb 15, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 15, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2024

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (e14e768) 96.38% compared to head (9d149f1) 96.39%.
Report is 45 commits behind head on develop.

Files Patch % Lines
...owertools/utilities/feature_flags/feature_flags.py 88.00% 2 Missing and 1 partial ⚠️
...ambda_powertools/utilities/feature_flags/schema.py 97.72% 1 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3775      +/-   ##
===========================================
+ Coverage    96.38%   96.39%   +0.01%     
===========================================
  Files          214      214              
  Lines        10030    10102      +72     
  Branches      1846     1869      +23     
===========================================
+ Hits          9667     9738      +71     
- Misses         259      260       +1     
  Partials       104      104              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@heitorlessa heitorlessa marked this pull request as draft February 15, 2024 13:18
@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 16, 2024
@heitorlessa heitorlessa marked this pull request as ready for review February 19, 2024 08:57
@heitorlessa
Copy link
Contributor Author

Leaving these for another two PRs

  • Perf improvement
    • Validate same schema once
    • Identify perf bottleneck w/ feature flags (slowest test in our functional suite)
  • Revamp docs
    • Revamp user guide (add schema examples, add diagrams, remove excessive banners, etc.)
    • Revamp Schema Validation errors (provide actionable examples)

Copy link
Contributor Author

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had a peer review with @rubenfonseca and will make the following changes before:

  • Add debug on noop method
  • Use cachedproperty for TimeValues

@heitorlessa
Copy link
Contributor Author

heitorlessa commented Feb 19, 2024

Leaving to a separate PR

  • Validate schema once (hash)
  • Revamp docs (add validation_exception_handler, add schema examples, add diagram, remove excessive banners etc)

rubenfonseca
rubenfonseca previously approved these changes Feb 19, 2024
Copy link
Contributor

@rubenfonseca rubenfonseca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just left a NP

Co-authored-by: Ruben Fonseca <[email protected]>
Signed-off-by: Heitor Lessa <[email protected]>
Copy link

Quality Gate Passed Quality Gate passed

Issues
3 New issues

Measures
0 Security Hotspots
No data about Coverage
2.2% Duplication on New Code

See analysis details on SonarCloud

@heitorlessa heitorlessa merged commit b266979 into aws-powertools:develop Feb 19, 2024
@heitorlessa heitorlessa deleted the chore/feat-flags-adjustments branch February 19, 2024 13:19
@heitorlessa heitorlessa added the internal Maintenance changes label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Maintenance changes size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants