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

Add feature flag support with flipper #3408

Merged
merged 16 commits into from Mar 17, 2017
Merged

Conversation

nilbus
Copy link

@nilbus nilbus commented Mar 14, 2017

This patch introduces the flipper gem, which satisfies all the requirements listed in #3407. I chose this over alternatives, because it's self-contained (no third party involved), works with ActiveRecord (keeping dev setup consistent), and does exactly what we want.

This will be used to turn on and off experiment features and roll out to a random 50% experiment group described in exercism/discussions#123.

Todo:

  • Alias User id to flipper_id
  • Create a globally accessible Flipper instance, and use it in config.ru
  • Restrict the rack-mounted /flipper/ UI to the right people™
  • Documentation for using feature flags in test, development, and production

For authentication, I imagine I could hard-code @kytrinyx and myself in production, but is there a more appropriate existing group?

@kytrinyx
Copy link
Member

We don't have an existing group. I think it's fine to hard-code our IDs. The other option would to create a team that we can manage via the UI and authenticate against members of that team.

I don't think that's necessary, though.

@kytrinyx
Copy link
Member

Agreed, btw, flipper looks like a good choice.

I tested that nil is an acceptable flipper_id.

Can't use alias_method for User#username, because it's an ActiveRecord field, and
metaprogramming™.
- Refactor the Flipper::UI rack app builder into its own class.
- Introduce global $flipper for convenient reference anywhere.
  This is an appropriate use of a global.
  It is thread-safe, because it is assigned only once at startup.
Putting this in #setup with our without calling #super is wreaking all kinds of
failures. Some prefer this kind of duplication in tests anyway, since each is easier
to read. So here it is.
@nilbus
Copy link
Author

nilbus commented Mar 15, 2017

Okay, build fixed and ready for review!

Copy link
Member

@kotp kotp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small typo

=============

Feature flags allow specific features to be turned on and off at runtime for a
percentage of users or the enitire site. Feature flags enable:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'entire site'

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed!

conditionals, removing any old code the new feature replaced and all references to
your feature flag. Leaving enabled feature flags in the code increases complexity
and is unacceptable. The feature flag admin will delete the feature flag in the UI
after the PR is merged and deployed successfully.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's clarify in this section that this PR needs to be merged when the feature has been tested in production for the required/desired length of time.

Copy link
Author

@nilbus nilbus Mar 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this?

  1. Ensure everything is tested and working in production for a reasonable amount of time.
  2. Important: Create a new pull request that removes all your feature flipper's
    conditionals, removing any old code the new feature replaced and all references to
    your feature flag. Leaving enabled feature flags in the code increases complexity
    and is unacceptable. The feature flag admin will delete the feature flag in the UI
    after the PR is merged and deployed successfully.

Committed in 860e2ee

end

def deny_markup
'<html><body><img src="https://octodex.github.com/images/bouncercat.png"></body></html>'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love that we're using the bouncercat for this. Excellent choice.

User.stubs(:where).returns(authorized_users)
env = {'rack.session' => nil}
response = force_development_env { middleware_response(env) }
assert_allowed(response)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that in development you'll always be seeing the new feature?

Copy link
Author

@nilbus nilbus Mar 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Negative; it means that you can always visit /flipper and manage the features in development, whereas only we can do that in production.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe test_flipper_management_allowed_in_development would be more expressive here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. All methods renamed in a19c2d0.

@kytrinyx
Copy link
Member

This looks good, @nilbus. I have just the one minor suggestion.

@@ -3,6 +3,7 @@
class Guest
def id
end
alias_method :flipper_id, :id

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the flipper_id for User is the username, it kinda makes sense to maintain it here too.

def flipper_id
  username
end

This is a nitpick, feel free to just ignore this. 😄

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we are using this here to be sure we get nil for guests. It got me confused because we want to authorize using thegithub_id but we are using username instead. But yeah, they are the same. 😅

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a little confusing. I could as easily alias username and use the value "guest" instead of nil. If https://github.com/guest signs in, nothing will break ¯_(ツ)_/¯.


def authorized_github_ids
@authorized_github_ids ||=
User.where(username: authorized_usernames).map(&:github_id)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use pluck here if we just want attributes. User.where(username: authorized_usernames).pluck(:github_id)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes! Fixed in 24be5a3.


class AssignmentsApiTest < Minitest::Test
def test_unauthenticated_users_rejected
User.stubs(:where).returns(authorized_users)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case this is harmless, but we should avoid stubbing ActiveRecord classes. We are returning an Array here, but ActiveRecord returns an ActiveRecord_Relation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about this next version, where I'm stubbing the User ActiveRecord::Relation?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It fix one problem, but there is also the case where if you chain this relation with another method (where, order, etc..) you will get a new relation that is not stubbed. It is usually better to stub the API that we own (authorized_github_ids in this case) than stub an API of an object that we don't have any control over. :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a common problem I hit. I haven't typically thought to stub my own methods because of POODR's warning against this practice, but with a method this small, I absolutely agree that the effect is equivalent with a method this small, and it's much more resilient to change. Not to mention that it side-steps the annoying side effects I was having when stubbing this AR method with that led to the duplication. I appreciate you having me think about this! 🎉

Fixed in f41f595.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem @nilbus! Thank you for this PR/study! <3

@nilbus
Copy link
Author

nilbus commented Mar 17, 2017

Thanks for the reviews everyone.

@kytrinyx I believe this is ready to merge.

@kotp
Copy link
Member

kotp commented Mar 17, 2017

Has the issue been created yet to remove the flipper code at some point? Should this be created before this PR gets created, just to cover the debt points that are coming in?

@nilbus
Copy link
Author

nilbus commented Mar 17, 2017

@kotp Are you referring to future flipper code for individual features—conditionals that test on feature flags? If so, that could be a reasonable way to keep track of that debt, yes. It's the conditionals for individual features that introduce the real code debt.

Or instead do you mean the flipper gem and code introduced in this PR? Since feature flags can be useful beyond the scope of my experiments (e.g. for future experiments you may want to run, or for frequent integration of long-running branches to reduce conflict), I suggest that we keep Flipper around. It doesn't introduce any complexity on its own.

@kotp
Copy link
Member

kotp commented Mar 17, 2017

Important: Create a new pull request that removes all your feature flipper's
conditionals, removing any old code the new feature replaced and all references to
your feature flag. Leaving enabled feature flags in the code increases complexity
and is unacceptable. The feature flag admin will delete the feature flag in the UI
after the PR is merged and deployed successfully.

This issue, as reference in the changes.

@kytrinyx
Copy link
Member

I suggest that we keep Flipper around. It doesn't introduce any complexity on its own.

I agree that we should keep Flipper around.

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

Successfully merging this pull request may close these issues.

None yet

4 participants