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 #557

Open
19 of 29 tasks
jnunemaker opened this issue Sep 1, 2021 · 16 comments
Open
19 of 29 tasks

Expressions #557

jnunemaker opened this issue Sep 1, 2021 · 16 comments
Assignees

Comments

@jnunemaker
Copy link
Collaborator

jnunemaker commented Sep 1, 2021

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

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

What does a "Rule" do?

@jnunemaker
Copy link
Collaborator Author

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

@onesneakymofo
Copy link
Contributor

onesneakymofo commented Aug 11, 2022

@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!

@jnunemaker jnunemaker changed the title Rules Rules/Expressions Aug 15, 2022
@bkeepers bkeepers mentioned this issue Jan 3, 2023
@bkeepers bkeepers changed the title Rules/Expressions Expressions Jul 17, 2023
@marcogregorius
Copy link

@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?

@bkeepers
Copy link
Collaborator

@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.

@rnestler
Copy link

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

@gs-deliverists-io
Copy link

gs-deliverists-io commented Dec 11, 2023

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?

@jnunemaker
Copy link
Collaborator Author

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
Copy link
Collaborator Author

@billkauf thanks for reporting. Working on a fix. #786

@BrandonHicks-msr
Copy link

@gs-deliverists-io

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

@fer9305
Copy link

fer9305 commented Dec 11, 2023

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)

@bkeepers
Copy link
Collaborator

@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)

@fer9305
Copy link

fer9305 commented Dec 11, 2023

@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)

@bkeepers
Copy link
Collaborator

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

@BrandonHicks-msr
Copy link

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

Thank you @bkeepers for looking into this

@bkeepers
Copy link
Collaborator

I opened another issue to talk specifically about this MySQL issue. Let's move conversation over there #789

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

No branches or pull requests

9 participants