-
-
Notifications
You must be signed in to change notification settings - Fork 418
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
Proposed feature: Check if enabled only for some actors #481
Conversation
lib/flipper/dsl.rb
Outdated
@@ -34,6 +34,14 @@ def enabled?(name, *args) | |||
feature(name).enabled?(*args) | |||
end | |||
|
|||
def enabled_for_some?(name, actor) | |||
if enabled?(name) | |||
raise Flipper::Error, "The feature #{name.inspect} is enabled for all!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably doesn't make sense to raise an error. I did so on a whim here, because I think it would be desirable to provide some logging in the case that this condition happens. I think a better solution might be an optional block or argument that can be leveraged by the caller to respond as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and converted this to a block in b962c0b.
lib/flipper/dsl.rb
Outdated
@@ -34,6 +34,15 @@ def enabled?(name, *args) | |||
feature(name).enabled?(*args) | |||
end | |||
|
|||
def enabled_for_some?(name, actor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My colleague made a good point, that this is mostly about "enabled for actors" or actor-only feature flags that aren't enabled by percentage or globally. That's a much better way to put it. There may be a better way to model this deeper in the abstractions, but my hot take would be name this method strict_enabled?
.
@@ -34,6 +34,15 @@ def enabled?(name, *args) | |||
feature(name).enabled?(*args) | |||
end | |||
|
|||
def strict_enabled?(name, actor) | |||
if enabled?(name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still leaves open the possibility of enabled by percentage which could also be problematic 🤔
Could you show a couple examples of how this is used in the code? I can see and understand the code/specs, but I kind of want to see it in real life so I better understand the use case and if there is a different way we could maybe solve it using what already exists. I think a few real life code examples would help me understand. Thanks! |
Yes, of course! Here is an example. Give some small testing-in-production abstraction like: module TestingInProduction
def self.simulate_decline?(user)
Flipper.strict_enabled?(:simulate_decline, user, on_reject: ->(feature){
Rails.logger.warn <<~MSG
The feature #{feature.inspect} is enabled too liberally to be used safely for testing-in-production.
MSG
})
end
end Note: In this example, I went with an And later on in application code: class PurchaseController < ApplicationController
def create
# Note: We only want this to return `true` when the user is an enabled actor. Not when the feature is enabled
# by percentage or globally as that would affect other users in an unpredictable or unintended way.
if TestingInProduction.simulate_decline?(user)
render json: { error: "declined" }, status: 400
else
# ... (do things to actually complete the purchase)
render json: { success: true }
end
end
end I'm going to noodle more on another use case. We've repeated a pattern similar to this a few times, but it's all in the name of testing-in-production, so I don't have another use case handy in mind right now. |
In discussion with a colleague, another interesting perspective came up. Potentially this feature could be limited to flipper-ui in that the primary risk that we're addressing is the potential for mistakenly toggling a feature on in a more "global" sense, so perhaps there would be room to add some level of restriction to the UI that is configurable. For example, disable the "Fully Enable" button for particular features. 🤔 Clearing: There's still a part of the UI-only idea that makes me uneasy. Given the "in production" aspect of my use case, I would worry that there is still some way to get into a pickle which could result in production issues. For that reason, I like the idea of a more "provably safe" method. |
Interesting. Seems like what you really want is a way to remove certain gates from a feature and disallow using them. I actually definitely want to support that. I haven't built it because it will involve adapter changes -- either some sort of KV or config storage per feature or switching to v2 #163 adapters that are more KV based and dumber, but with the ability to store more. |
@iamvery Did you end up working around this or is it still hanging open? Just curious. I'm probably going to close it due to age, but I'm very happy to keep working through this (and will do a better job of responding promptly, sorry about that). |
Oh, and here is the issue about customizing which gates are used per feature. |
I'm not actively working on this or thinking about it, so thank you for closing it out. For the time being on our side we're not actively making changes. If something changes, I'll come back to this. Thanks for responding! And no problem at all on the timing 🙏 |
Context
I realize that this may not be a feature you want to support, but it came up in a use case that we have for Flipper, so I wanted to at least open the conversation here in case there is an opportunity to pay it forward. I will also say that the PR as it is being opened is more a conversation starter than anything, so please know that I'm not deeply committed to the implementation. I'm happy to iterate on the solution.
Problem
We recently started using Flipper as a way of empowering some testing-in-production features in our infrastructure. In particular, we wanted a way to flag certain domain object flowing through our system to behave in a certain way. For example:
To do this, we use Flipper, but as a safety mechanism we added an additional check to make certain that the features aren't enabled globally, because we wouldn't want to decline every purchase for all users by accident 😎
Solution
Our solution involved a small wrapper around Flipper, so we thought I might be something to add to the library. It essentially amounts to checking for a feature which is globally enabled and faulting on that condition.
Please let me know your thoughts! I would love to understand your perspective on this problem and whether it is something you could like to include in Flipper.