aws-powertools / powertools-lambda-python

A developer toolkit to implement Serverless best practices and increase developer velocity.
https://docs.powertools.aws.dev/lambda/python/latest/
MIT No Attribution
2.84k stars 389 forks source link

List intersection action for conditions in feature flags #3531

Closed Rogalek closed 7 months ago

Rogalek commented 9 months ago

Use case

I was thinking if there is a way to write action for condition in feature flags that can do list Intersection. https://www.geeksforgeeks.org/python-intersection-two-lists/

Solution/User Experience

So user could provide key: [A, B] and inside AWS Appconfig json there will be stored value: [B, C] And if at least one elements match for both list that this will match criteria.

Alternative solutions

No response

Acknowledgment

boring-cyborg[bot] commented 9 months ago

Thanks for opening your first issue here! We'll come back to you as soon as we can. In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link

rubenfonseca commented 9 months ago

Interesting, the existing KEY_IN_VALUE does not cover a scenario where both items are lists. Is this something you would be able to contribute with a pull request? I would love to help you merge it.

Rogalek commented 9 months ago

Yes, sure I can try to create PR after Christmas and New Year

gwlester commented 8 months ago

May want a couple of intersections:

leandrodamascena commented 8 months ago

Hi @Rogalek and @gwlester! Thanks for sharing this idea to add more features to our Feature Flags utility. For me, it makes a lot of sense to expand this support. After all, we open up more opportunities to use this utility, such as checking if, given an array, ANY of those elements are present, something that we don't support nowadays because we don't check individual values passed in the context.

Just a small comment:

  • ALL_IN_VALUE

We already support it if you use EQUALS.

Do you still have plans to send a PR with these changes @Rogalek? Please let me know and we can work together to get it merged.

gwlester commented 8 months ago

@leandrodamascena You wrote: ALL_IN_VALUE We already support it if you use EQUALS.

They are not the same -- All_IN_VALUE would mean that all in the list are in the value, but does not also mean that all in the value are in the list. EQUAL means that all in the list are in the value and also that all in the value are in the list.

gwlester commented 8 months ago

I think I may be taking a shot at implementing this over the next couple of weeks -- does anyone have any issues with that?

@heitorlessa , @leandrodamascena

gwlester commented 8 months ago

I have the code at https://github.com/gwlester/aws-lambda-powertools-python/tree/feature-3531 -- but not sure what to do about the tests. With the way they are organized now, I don't see a good way to add ones for these three actions (there is another ticket to refactor them).

Can I get some feedback about the tests?

leandrodamascena commented 8 months ago

@leandrodamascena You wrote: ALL_IN_VALUE We already support it if you use EQUALS.

They are not the same -- All_IN_VALUE would mean that all in the list are in the value, but does not also mean that all in the value are in the list. EQUAL means that all in the list are in the value and also that all in the value are in the list.

Hey @gwlester! Yep, you're right, I agree!

I have the code at gwlester/aws-lambda-powertools-python@feature-3531 -- but not sure what to do about the tests.

Do you want to open a PR without testing and we can work together to add these tests? We can do some code reviews until we reach the ideal code for merge. It makes sense?

Thanks

gwlester commented 8 months ago

@leandrodamascena pull request created: https://github.com/aws-powertools/powertools-lambda-python/pull/3692

github-actions[bot] commented 7 months ago

⚠️COMMENT VISIBILITY WARNING⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.

heitorlessa commented 7 months ago

Adding a sample schema for reference as I didn't find them earlier.


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"
                        ]
                    }
                ]
            }
        }
    }
}
heitorlessa commented 7 months ago

@Rogalek @gwlester I've noticed this is our first built-in condition that an input may not be a list and a silent error (False flag) may not be ideal -- instead of failing flat causing further disruption to other flags, I'm gonna add an exception handler that you could use to trap ANY exception.

I'm refactoring the last bits oft the implementation, and will work on the exception handler. For that reason, we'll release early next week only - mon/tue.

heitorlessa commented 7 months ago

Closing as it's coming in this week's release. PR merged w/ refactorings.

github-actions[bot] commented 7 months ago

⚠️COMMENT VISIBILITY WARNING⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.

github-actions[bot] commented 7 months ago

This is now released under 2.34.0 version!