Allow an extra parameter to be passed to group blocks

[aroben]

Methods that took a group name now take an optional block parameter, which is passed through to the group's block. This can be used, e.g., to implement dynamic groups based on a database. Here's how it looks:

Flipper.register(:team) do |actor, team_id|
  Team.find(team_id).members.include?(actor)
end

$flipper[:my_feature].enable $flipper.group(:team, Team.by_name("admins").id.to_s)

The block parameter must be convertible to a String via #to_str.

Fixes #50

/cc @rsanheim @jnunemaker

[rsanheim]

Why did you need these changes?

[aroben]

aroben@95ca7f4 made Flipper.register return a Flipper::Group instead of a Flipper::Types::Group. This code was then passing that Flipper::Group into Feature#enable.

We could handle this case, but it seemed like it would just encourage mistakes to be so lax about the types. I think in most cases client code shouldn't be dealing with Flipper::Group objects at all; they're mostly an implementation detail.

[rsanheim]

This seems like a good solution to me. It also seems non-invasive to me, and I can't see any areas were existing users of flipper would be hurt by it unless they are depending on the specific class of Flipper::Types::Group.

[rsanheim]

Hrm, I guess this could break old clients, if they happen to use this separator in group names. That would be pretty surprising, though.

[aroben]

Yup, it would certainly be surprising. And the clients will notice it right away, as soon as Flipper.register is called.

[aroben]

I guess I should write some docs too.

[rsanheim]

This would definitely require a 0.0.7.beta bump.

I also noticed that there is a CHANGELOG that noone has touched since 0.0.6...we should either kill that or automate it somehow so we don't forget it.

[jnunemaker]

I just don't think I like the complexity this adds. I feel like the code below, which uses the stock flipper features we have now, is more explicit and simple.

Flipper.register(:admins_team) do |actor|
  Team.by_name("admins").members.include?(actor)
end

flipper[:my_feature].enable flipper.group(:admins_team)

I keep looking at the changes and I just don't get the use case driving the block changes and the dynamic group issue/pr. I'm open to change, but this feels like trading explicit simple code for a bit magical code. Thoughts?

[aroben]

@jnunemaker The hope is to better support the way that we at GitHub ship new features.

We have a bunch of teams at GitHub and we like to turn new features on for them before turning them on more widely. For example, someone working on a new organization-related feature might turn the feature on for just the @github/orgs team. Or someone working on a new pull request feature might turn it on for just the @github/pull-requests team. Or someone working on a new billing feature might turn it on for just the @github/billing team.

As you said, we could just define groups for each of these teams:

%w[orgs pull-requests billing].each do |team|
  Flipper.register("#{team}_team") do |actor|
    Team.by_name(team).members.include?(actor)
  end
end

But requiring a code change each time we decide there's a new team that it would be useful to scope a feature to seems heavyweight. The goal of this PR was to make it so these teams could be added dynamically without making a code change/deploy. In our web UI for modifying Flipper features, we'd just have a "enable for team" field that let you type in any GitHub team name.

There is definitely added complexity for supporting this functionality. The hope was to make it super easy for someone to turn on a feature for a team without having to make a commit/deploy.

[jnunemaker]

I've always considered using teams at GitHub as a backwards way of doing features. The first backwards thing is that it leads to an explosion of teams which is its own issue. The second backwards part is that most people then use that team for mentions and more. This means when you join the team to try the new feature, you also get a bunch of notifications. Because I've never wanted to receive the notifications, I often don't join the new teams and try out new features. I think this style of enabling features at GitHub exists simply because we didn't have flipper in the past.

Now that we have flipper, I would lean toward using teams on github.com for mentions and notifications as we do (to get work done) and use flipper for joining/leaving the use of a new feature.

Let's pretend I was going to move notifications to a new cluster. Instead of making a new team in the github organization like notifications-new-cluster and using those people as guinea pigs, I'd make a feature that was named notifications_new_cluster and I'd tell people to add themselves as an actor or I would add them myself using our flipper interface.

This solves the issue of needing to commit/deploy as the person can simply add themselves in the UI as an actor that is enabled for the feature. It also moves all feature related stuff into flipper completely which means no chance of N+1 database calls for feature checks. The Team.by_name(...).members check requires a database call and would not (without work) be memoized per request as flipper actor checks are.

Any thoughts on that? I realize this is different from how we do things now, but I think it is a good thing, an improvement. Teams are for getting work done and communication and features are for toggling.

[aroben]

The second backwards part is that most people then use that team for mentions and more.

This is definitely a problem (unless you create two teams, one for the people working on the feature and the other for people testing the feature, but then you have an even bigger explosion of teams).

Now that we have flipper, I would lean toward using teams on github.com for mentions and notifications as we do (to get work done) and use flipper for joining/leaving the use of a new feature.

That seems like a reasonable approach for now. If we find there are groups of people that we typically want to enable features for (e.g., all engineers working on github.com) we can create a group for them.

Thanks for taking the time to think about this and write up your thoughts. I agree we should try to lean into Flipper a little harder before trying to shoehorn our old workflow into Flipper. Maybe eventually we'll decide that team-based shipping is a good idea, but let's see how far we get without it.

[jnunemaker]

we can create a group for them

Sounds good.

Thanks for taking the time to think about this and write up your thoughts.

No problem. Thanks for proposing a working solution. Makes it easier to think through.

Maybe eventually we'll decide that team-based shipping is a good idea, but let's see how far we get without it.

Definitely. If we get to a point where we are feeling pain, we can come back to the drawing board.