search

flippercloud / flipper

🐬 Beautiful, performant feature flags for Ruby.
https://www.flippercloud.io/docs
MIT License
3.7k stars 413 forks source link

Misleading beahvior with `boolean` #746

Closed nhorton closed 1 year ago

nhorton commented 1 year ago

flipper.enable "foo" does a Boolean enable of a feature.

flipper.disable "foo" destroys every enablement record including actors and groups.

There are also no examples of how to actually disable the boolean version in the docs anywhere.

Suggestions:

  1. Add an example for explicitly enabling the Boolean version and explicitly disabling it in the docs
  2. Require a second argument in disable. It could be something that has the second optional for 6 months while you log warnings that the second will be required soon, and then make it fully required
  3. Consider doing #2 but for enable too. Lower pri, but I think it would be worthwhile.
  4. Add a disable_all method for the current behavior
jnunemaker commented 1 year ago

This is intentional. I think it would be more confusing to disable and go back to a state of partial enablement than the current behavior. If you want to go back in time an audit log with rollbacks is probably best.

I usually recommend enabling 100% of the time if you want to save your state. You can always turn that off and would revert to the partially enabled states.

nhorton commented 1 year ago

Hey John, Sorry - was on vacation.

I hear you but this still does not make sense. If you want the behavior you said, then enable "foo" should also destroy the actor and group gates. As it is, they are effectively cruft since they cannot ever be used again.

Regardless of the desired behavior, the API itself is inconsistent right now and it is really not in an obvious way. And at the very minimum, the documentation does not accurately describe the behavior.

jnunemaker commented 11 months ago

I hear you but this still does not make sense. If you want the behavior you said, then enable "foo" should also destroy the actor and group gates. As it is, they are effectively cruft since they cannot ever be used again.

Agreed. They should be cleared on enable too. I'm not remembering why they aren't. I'll make an issue.