flipper icon indicating copy to clipboard operation
flipper copied to clipboard

Expressions

Open jnunemaker opened this issue 4 years ago • 16 comments
trafficstars

I'll try to communicate more soon. But the tldr is @orderedlist, @bkeepers and I are working on a new feature (for www.flippercloud.io and open source) we're calling "rules" (branch). This issue is just so we can keep track of the work together on the open source side.

ActiveRecord Migration

Those using the flipper-active_record adapter will want to migrate the database so it can store JSON expressions:

$ rails generate migration change_flipper_gates_value_to_text
  def up
    change_column :flipper_gates, :value, :text
  end

  def down
    change_column :flipper_gates, :value, :string
  end

TODO

  • [x] Support single Rule
  • [x] Support multiple rules through And rule
  • [x] Support multiple rules through Any rule
  • [x] Refactor from_hash and other case statements
  • [x] Get all specs and tests passing
  • [x] Add enable_* and disable_* methods to Flipper, Flipper::DSL and Flipper::Feature
  • [x] Add Rule equality like == and eql?
  • [x] Make real ruby classes for types in left, operator and right
  • [x] Add examples for Rule, Any, and And
  • [x] Make all types consistent in capitalization (currently Condition and property, string, etc.)
  • [x] Add rule shortcuts to make it easier to use them
  • [x] Add new shared adapter spec(s) to shared adapter tests
  • [x] Update feature synchronizer to include :json/rules
  • [x] Update api docs
  • [x] Update gate docs
  • [x] Error in AR/Sequel if value column is not text
  • [x] Add Flipper.add_rule and Flipper.remove_rule for easy adding and removing
  • [x] What about floats? Should we use number instead of integer as the type and support floats? Or add float as type?
  • [x] Document AR/Sequel migration to convert value from string to text

After releasing beta

  • [ ] Enforce types for operators. in requires array on right. percentage requires string on left and integer/number on right, etc. Make this easy to pass to javascript front end for @orderedlist.
  • [ ] Enforce string, number, null, boolean, etc. or array of former for flipper_properties somehow (maybe skip invalid and warn to avoid blowing up? or blow up in dev but skip in prod?)
  • [ ] Ensure that time based enablements are easy (enable when now epoch > int or now > date time)
  • [ ] Make it possible to enable/disable rules in flipper-ui
  • [ ] Make it possible to enable/disable rules in www.flippercloud.io
  • [ ] Update readme to include rules

Someday

  • [ ] Make it easier to work with date/time for scheduled releases (things like "enabled if Monday-Friday 9am-5pm)", just an and expression with day of week and time of day extraction that do gt, gte, lt, lte comparisions, etc.)
  • [ ] Make it possible to do repeating time based enablements (enable if M-F 9a-5pm, etc.)
  • [ ] Deprecate all existing gates in favor of rules with examples on how to migrate
  • [ ] Add feature type that passes actor and checks enabled? so you can do stuff like feature dependencies

jnunemaker avatar Sep 01 '21 12:09 jnunemaker

What does a "Rule" do?

marknuzz avatar Jun 29 '22 03:06 marknuzz

@marknuzz check out this file for examples. Sorry for the slow response. I was gone on a 6 week trip.

jnunemaker avatar Jul 25 '22 00:07 jnunemaker

@jnunemaker and co., this is beautiful. I was just thinking about how to implement features for different mobile app versions and it seems expressions / Rules will do the trick. Keeping an eye on this - thanks!

onesneakymofo avatar Aug 11 '22 14:08 onesneakymofo

@jnunemaker thanks for the incredible feature. I am looking to implement a percentage of time within a set of actors, ie when a feature flag is enabled for a User, but also have the percentage of time randomness for that User only. The Expressions would be a nice solution for this.

If you don't mind me asking, is there an estimate of when would this be released for open source?

marcogregorius avatar Sep 27 '23 01:09 marcogregorius

@marcogregorius Expressions are currently in the main branch and will be released as beta in 1.1. We don't have a timeline for that, but should be within the next month or so.

bkeepers avatar Sep 27 '23 12:09 bkeepers

If you also have rubocop configured to warn about Rails/ReversibleMigration: change_column is not reversible. one can use the following for the migration:

class ChangeFlipperGatesValueToText < ActiveRecord::Migration[7.0]
  def up
    change_column :flipper_gates, :value, :text
  end

  def down
    change_column :flipper_gates, :value, :string
  end
end

rnestler avatar Dec 11 '23 11:12 rnestler

Hi,

migration cannot be executed on mysql using trilogy

BLOB/TEXT column 'value' used in key specification without a key length (Trilogy::ProtocolError)

any ideas?

gs-deliverists-io avatar Dec 11 '23 12:12 gs-deliverists-io

Can you drop the full migration file and the output of the command so we can investigate? I've used trilogy like once and had a bunch of issues. Ended up going back to mysql2.

jnunemaker avatar Dec 11 '23 14:12 jnunemaker

@billkauf thanks for reporting. Working on a fix. https://github.com/flippercloud/flipper/pull/786

jnunemaker avatar Dec 11 '23 16:12 jnunemaker

@gs-deliverists-io

I'm seeing this as well, with the exact same migration as @rnestler has posted above.

BrandonHicks-msr avatar Dec 11 '23 16:12 BrandonHicks-msr

Can you drop the full migration file and the output of the command so we can investigate? I've used trilogy like once and had a bunch of issues. Ended up going back to mysql2.

@jnunemaker I'm seeing the same behavior.

Migration file:

class ChangeFlipperGatesValueToText < ActiveRecord::Migration[7.0]
  def up
    change_column(:flipper_gates, :value, :text)
  end

  def down
    change_column(:flipper_gates, :value, :string)
  end
end

output:

┃┃ == 20231211180842 ChangeFlipperGatesValueToText: migrating ====================
┃┃ -- change_column(:flipper_gates, :value, :text)
┃┃ rails aborted!
┃┃ StandardError: An error has occurred, all later migrations canceled: (StandardError)
┃┃
┃┃ Mysql2::Error: BLOB/TEXT column 'value' used in key specification without a key length
┃┃ db/migrate/20231211180842_change_flipper_gates_value_to_text.rb:4:in `up'
┃┃
┃┃ Caused by:
┃┃ ActiveRecord::StatementInvalid: Mysql2::Error: BLOB/TEXT column 'value' used in key specification without a key length
┃┃ (ActiveRecord::StatementInvalid)
┃┃ db/migrate/20231211180842_change_flipper_gates_value_to_text.rb:4:in `up'
┃┃
┃┃ Caused by:
┃┃ Mysql2::Error: BLOB/TEXT column 'value' used in key specification without a key length (Mysql2::Error)
┃┃ db/migrate/20231211180842_change_flipper_gates_value_to_text.rb:4:in `up'
┃┃ Tasks: TOP => db:migrate
┃┃ (See full trace by running task with --trace)

fer9305 avatar Dec 11 '23 18:12 fer9305

@fer9305 @BrandonHicks-msr @gs-deliverists-io

Does this fix the error for you?

- change_column(:flipper_gates, :value, :text)
+ change_column(:flipper_gates, :value, :text, size: 65535)

bkeepers avatar Dec 11 '23 18:12 bkeepers

@bkeepers

now it fails with a different error

== 20231211183249 ChangeFlipperGatesValueToText: migrating ====================
-- change_column(:flipper_gates, :value, :text, {:size=>65535})
rails aborted!
StandardError: An error has occurred, all later migrations canceled: (StandardError)

65535 is invalid :size value. Only :tiny, :medium, and :long are allowed.
db/migrate/20231211183249_change_flipper_gates_value_to_text.rb:4:in `up'

Caused by:
ArgumentError: 65535 is invalid :size value. Only :tiny, :medium, and :long are allowed. (ArgumentError)
db/migrate/20231211183249_change_flipper_gates_value_to_text.rb:4:in `up'
Tasks: TOP => db:migrate

however, if I pass :medium instead of 65535 it raises the original error

Caused by:
ActiveRecord::StatementInvalid: Mysql2::Error: BLOB/TEXT column 'value' used in key specification without a key length (ActiveRecord::StatementInvalid)

fer9305 avatar Dec 11 '23 18:12 fer9305

Ok, I'm able to duplicate locally now, so I'll work on a fix here soon.

bkeepers avatar Dec 11 '23 18:12 bkeepers

I can confirm I received a similar error as @fer9305.

Thank you @bkeepers for looking into this

BrandonHicks-msr avatar Dec 11 '23 19:12 BrandonHicks-msr

I opened another issue to talk specifically about this MySQL issue. Let's move conversation over there https://github.com/flippercloud/flipper/issues/789

bkeepers avatar Dec 11 '23 19:12 bkeepers