Skip to content
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

Expressions #692

Merged
merged 197 commits into from
Jul 17, 2023
Merged

Expressions #692

merged 197 commits into from
Jul 17, 2023

Conversation

bkeepers
Copy link
Collaborator

@bkeepers bkeepers commented Jan 3, 2023

Described by @jnunemaker in #557, Expressions are a new syntax for defining rules in Flipper and will enable much more sophisticated feature flipping. We're going to try to get this merged into Flipper as an experimental new feature while we put the finishing touches on it.

@bkeepers bkeepers mentioned this pull request Mar 30, 2023
3 tasks
@jhubert
Copy link

jhubert commented Apr 7, 2023

So excited for this! Game changer 🎉

@swishstache
Copy link

When an expression match requires a method call (such as "user.admin?") should we expect to just add additional values to "flipper_properties"?

  def flipper_properties
    super.merge!("is_admin" => admin?)
  end

@bkeepers
Copy link
Collaborator Author

@swishstache Yep, that's what we were thinking. Does that work for you?

@swishstache
Copy link

@swishstache Yep, that's what we were thinking. Does that work for you?

It does! 🎉👍

@swishstache
Copy link

When using multiple actors, how might we achieve a match for a combination of them? For example, enabling a feature for "basic plan" user actors but, only for those that belong to a specific organization actor.

Example Code (based on the code from "examples/expressions.rb")

org_foo = Org.new(1, {
  "type" => "Org",
  "id" => 1,
  "now" => NOW,
})

org_bar = Org.new(33, {
  "type" => "Org",
  "id" => 33,
  "now" => NOW,
})

user_ted = User.new(1, {
  "type" => "User",
  "id" => 1,
  "plan" => "basic",
  "age" => 39,
  "team_user" => true,
  "now" => NOW,
})

user_lasso = User.new(32, {
  "type" => "User",
  "id" => 32,
  "plan" => "basic",
  "age" => 39,
  "team_user" => true,
  "now" => NOW,
})

# Basic plan users under Foo org only
is_foo_org = Flipper.property(:type).eq("Org"), Flipper.property(:flipper_id).eq("Org;1")
is_basic_plan_user = Flipper.property(:type).eq("User"), Flipper.property(:plan).eq("basic")

Flipper.enable :neat_feature, Flipper.all(is_foo_org, is_basic_plan_user)

# A basic plan user under Foo organization
assert Flipper.enabled?(:neat_feature, [org_foo, user_ted]) # => Expected true but was false. Please correct.

# A basic plan user under Bar organization
refute Flipper.enabled?(:neat_feature, [org_bar, user_lasso]) # => false

@jnunemaker
Copy link
Collaborator

@swishstache I'd collapse the actors into the properties. For expressions, the goal would be to pass all the context in one actor and then let the expression sort it out. For example, instead of passing org and user, I'd pass the user with the current org in the properties.

Forgive any typos or slight incorrectness in the pseudo-code below...

class OrgUser
  include Flipper::Actor

  def initialize(org, user)
    @user = user
    @org = org
  end

  def flipper_id
    "User;#{@user.id};Org;#{@org.id}" # or it could be @user.flipper_id or whatever makes sense
  end

  def flipper_properties
    @user.flipper_properties.merge("org_id" => @org.id)
  end
end

org_foo = Org.new(1, {
  "type" => "Org",
  "id" => 1,
  "now" => NOW,
})

org_bar = Org.new(33, {
  "type" => "Org",
  "id" => 33,
  "now" => NOW,
})

user_ted = User.new(1, {
  "type" => "User",
  "id" => 1,
  "plan" => "basic",
  "age" => 39,
  "team_user" => true,
  "now" => NOW,
  "org_id" => 1,
})

user_lasso = User.new(32, {
  "type" => "User",
  "id" => 32,
  "plan" => "basic",
  "age" => 39,
  "team_user" => true,
  "now" => NOW,
  "org_id" => 33,
})

# Basic plan users under Foo org only
is_foo_org = Flipper.property(:org_id).eq(1)
is_basic_plan_user = Flipper.all(Flipper.property(:type).eq("User"), Flipper.property(:plan).eq("basic"))

Flipper.enable :neat_feature, Flipper.all(is_foo_org, is_basic_plan_user)

# A basic plan user under Foo organization
assert Flipper.enabled?(:neat_feature, OrgUser.new(org_foo, user_ted)) # => true

# A basic plan user under Bar organization
refute Flipper.enabled?(:neat_feature, OrgUser.new(org_bar, user_lasso)) # => false

@bkeepers has been knee deep in this so he may have better ideas, but I was picturing something like that.

The only other thing I can think of is to merge all properties into one big hash but then you have to deal with collisions of property keys which seems painful.

* origin/main:
  Update changelog
  Improve performance of `color_name` function in log subscriber
  Update Changelog.md
  remove remote source from CSP.
  Bundle bootstrap, jquery and poppler.
  Add mirroring example
  Use new method of making logs bold
  Add a key_prefix option to the Redis adapter
@bkeepers bkeepers marked this pull request as ready for review July 17, 2023 13:37
@bkeepers
Copy link
Collaborator Author

I'm going to work on getting this released this week as "alpha", so I'm going to go ahead and merge this and I'll track any further work in #557.

@bkeepers bkeepers merged commit 61d189c into main Jul 17, 2023
@bkeepers bkeepers deleted the learn-the-rules branch July 17, 2023 14:55
bkeepers added a commit that referenced this pull request Aug 1, 2023
…d `percentage_of_time` methods on `Flipper` and `Flipper::DSL`

Backported from #692
bkeepers added a commit that referenced this pull request Aug 1, 2023
…d `percentage_of_time` methods on `Flipper` and `Flipper::DSL`

Backported from #692
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants