feat(feature_flags): add in feature_flags RE_* actions and update tests/docs #692
Conversation
pull-request-size
bot
added
the
size/L
boring-cyborg
bot
added
documentation
Codecov Report
@@ Coverage Diff @@
## develop #692 +/- ##
========================================
Coverage 99.97% 99.97%
========================================
Files 116 116
Lines 4855 4862 +7
Branches 267 267
========================================
+ Hits 4854 4861 +7
Partials 1 1
Continue to review full report at Codecov.
|
|
Hey @whardier that's a great idea! I’m returning from vacation next week and can help review, if that's okay. At a quick glance, I'd suggest receiving a re.compile object so customers to favour performance -- they can compile outside the handler for a full CPU power and we can use it on every invocation. Thank you!! |
|
Yup I kept things lambda oriented. I would refer to instead make a mapping
of compiled patterns.
On Thu, Sep 23, 2021 at 12:24 PM Heitor Lessa ***@***.***> wrote:
Hey @whardier <https://github.com/whardier> that's a great idea!
I’m returning from vacation next week and can help review, if that's okay.
At a quick glance, I'd suggest receiving a re.compile object so customers
to favour performance -- they can compile outside the handler for a full
CPU power and we can use it on every invocation.
Thank you!!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#692 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACKRFOI4STKUM6B2ZY7DMDUDOEHVANCNFSM5EUJBC4A>
.
--
<https://about.me/ShaneSpencer?promo=email_sig&utm_source=product&utm_medium=email_sig&utm_campaign=gmail_api&utm_content=thumb>
Shane Spencer
about.me/ShaneSpencer
<https://about.me/ShaneSpencer?promo=email_sig&utm_source=product&utm_medium=email_sig&utm_campaign=gmail_api&utm_content=thumb>
Sponsor @whardier on GitHub Sponsors <https://github.com/sponsors/whardier>
|
|
Alternative approach using user mappings: develop...whardier:feat/feature-flags-user-mapping-by-action |
This is awesome -- we're taking a look at it this week. Feel free to change this PR for a user-defined action that we can call it. If it is returns a falsy value or an exception we can say the rule didn't match. That should give more flexibility to define what they'd like with a simple contract. thoughts @risenberg-cyberark @michaelbrewer? |
|
I like the regex action. I think we'r missing tests - 6 actions require 12 tests - one for match, one for no match. Regarding the user actions - i'm the fence regarding this one, because they might override the default action by mistake. If we were to add such a support, the logic in the match by action should be - try to find the action in the default mapping, if it's None, check user mappings (not merge the dicts like i saw in the branch develop...whardier:feat/feature-flags-user-mapping-by-action) |
|
The logic can be improved — we can use a callable. I meant more of an idea
to allow user-defined function
…On Wed, 29 Sep 2021 at 15:06, Ran Isenberg ***@***.***> wrote:
I like the regex action. I think we'r missing tests - 6 actions require 12
tests - one for match, one for no match.
Regarding the user actions - i'm the fence regarding this one, because
they might override the default action by mistake. If we were to add such a
support, the logic in the match by action should be - try to find the
action in the default mapping, if it's None, check user mappings (not merge
the dicts like i saw in the branch
develop...whardier:feat/feature-flags-user-mapping-by-action
<develop...whardier:feat/feature-flags-user-mapping-by-action>
)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#692 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZPQBC4CXEZO7XYU4K5WGTUEMFMZANCNFSM5EUJBC4A>
.
|
|
I think it's a bit over-kill. After we add the regex and more 'not' actions, It will probably cover most of the use cases for most users. It would be really great if we could have some poll/feedback as to what actions are missing. |
|
Actually this would be instead of the regex action.
I’ll have a chat with the team tomorrow as we’ll tackle as many issues and
PRs we can tomorrow
…On Thu, 30 Sep 2021 at 08:29, Ran Isenberg ***@***.***> wrote:
I think it's a bit over-kill. After we add the regex and more 'not'
actions, It will probably cover most of the use cases for most users. It
would be really great if we could have some poll/feedback as to what
actions are missing.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#692 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZPQBCKTAGQI4K443FWWADUEP7U7ANCNFSM5EUJBC4A>
.
|
|
Strongly suggest renaming the rules so it is clear which is the pattern (the value or the key) and which is the string. I'd also suggest that it may be appropriate to have conditions that match the other way around to what is being proposed (i.e. swap what is the pattern and what is the string). |
|
I am on the fence on the approach as well as far as what would be best
suited for long-term maintainability and overall clarity.
Exceptions logged and falsy.
The default rules aren’t overridden and are last in the rules dict update.
|
|
If we go for RE instead to handle most cases we just need a precompiler
|
Requiring a precompiler does not sound like a good idea. |
|
It is fairly typical to use re.compile in situations like this. It would have to happen between after or during configuration load and use.. possibly part of validation |
|
Here is an example use case an RE would not cover -- compare if a key (e.g. current date in YYYY.MM.DD format) is greater than a given value (i.e. the general go-live date). The feature might also have anoter rule to see if the user is in a group of beta users and thus has access to the feature prior to the general go-live date. |
|
"The default rules aren’t overridden and are last in the rules dict update." - what if someone uses the same keys as the default rules? maybe it's ok? |
if we added greater than action that you could use unix epoch time, i know its less readable but it works |
As long as the key and value are formatted either as epoch time or as a string suggested the compares work. See: >>> def test(a,b):
... if a > b:
... print("Greater")
... else:
... print("Less than or equal")
...
>>> sa = "2021.10.01"
>>> na = 1633064400
>>> test(sa,"2021.11.01")
Less than or equal
>>> test(sa,"2021.09.01")
Greater
>>> test(na,1635742800)
Less than or equal
>>> test(na,1630472400)
Greater
>>> |
|
hey everyone - could we get the conflicts resolved as I'd like to have a second look at bringing RE actions instead of user defined? We'd also need to receive a compiled pattern ( |
|
@heitorlessa , if no one gets to it prior to this weekend I'll take a look at pulling down the branch and resolving conflicts. I'm slammed at work this week and that is the earliest I can get to it. |
|
Due to the lack of activity, I'm gonna close this for now - Once we merge #804, this will make it harder to maintain. Instead, let's focus on support Bring Your Own Action so customers can use anything they'd like to validate features. |
Issue #, if available:
#690
Description of changes:
Extend feature_toggles to enable regular expression actions... next step - lets just use embedded lua!
Checklist
Breaking change checklist
Nope
RFC issue #:
Nope
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
View rendered docs/utilities/feature_flags.md
View rendered docs/utilities/feature_flags.md