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

Misleading beahvior with boolean #746

Closed
nhorton opened this issue Jul 27, 2023 · 3 comments
Closed

Misleading beahvior with boolean #746

nhorton opened this issue Jul 27, 2023 · 3 comments

Comments

@nhorton
Copy link

nhorton commented Jul 27, 2023

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 Logging #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
Copy link
Collaborator

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

nhorton commented Aug 9, 2023

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

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.

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

2 participants