-
Notifications
You must be signed in to change notification settings - Fork 115
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 using Flipper #387
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Left some non-blocking comments.
README.md
Outdated
@@ -79,6 +80,9 @@ To illustrate, `bin/rails console` will run the console in the docker container | |||
- [ExceptionHunter](https://github.com/rootstrap/exception_hunter) for exception tracking | |||
- [Factory Bot](https://github.com/thoughtbot/factory_bot) for testing data | |||
- [Faker](https://github.com/stympy/faker) for generating test data | |||
- [flipper](https://github.com/jnunemaker/flipper) for feature flag support | |||
- [flipper-active_record](https://github.com/jnunemaker/flipper) for Flipper active_record support | |||
- [flipper-ui](https://github.com/jnunemaker/flipper) for UI-based feature flag management |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should have a /docs
directory to briefly explain how to access the visual Flipper interface or other relevant items about how / why to use Flipper. Same applies to new features.
app/models/user.rb
Outdated
@@ -35,6 +35,7 @@ class User < ApplicationRecord | |||
devise :database_authenticatable, :registerable, | |||
:recoverable, :trackable, :validatable | |||
include DeviseTokenAuth::Concerns::User | |||
include Flipper::Identifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for? Just asking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's to add the flipper_id
method that identifies the "actor".
Should we include it in ApplicationRecord
so that all of our records have the method defined? For example an app could have a model Product
and we might only want to enable certain feature for some products.. just thinking out loud, would like to hear others' thoughts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that could be an option, the implementation shouldn't impact or be tightly coupled with the business logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about including this in ApplicationRecord
. Feature flags are typically required for only a limited number of models, like User, Organization, Product.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check if this is true? Seems like Flipper already configures ActiveRecord::Base
to include Flipper::Identifier
.
If that's the case we can remove this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@santib yes, nice catch, it looks like their getting started guide is out of date
config/initializers/flipper.rb
Outdated
class CanAccessFlipperUI | ||
def self.matches?(request) | ||
current_user = request.env['warden'].user | ||
current_user.present? && current_user.is_a?(AdminUser) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be documented because we are imposing that the user should be of type AdminUser
.
config/routes.rb
Outdated
@@ -1,6 +1,9 @@ | |||
Rails.application.routes.draw do | |||
devise_for :admin_users, ActiveAdmin::Devise.config | |||
ActiveAdmin.routes(self) | |||
constraints CanAccessFlipperUI do | |||
mount Flipper::UI.app(Flipper) => '/flipper' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this enabled by default? Again just thinking out loud.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🌝 🏴
@mrodriguez90 @santib What is the purpose of including |
@blacksam07 I think anyone could benefit from using a tool like this. From my POV, it's an excellent tool and a compelling reason to select the rails_api_base to start any project, particularly for the time you could save by having this tool already set up. For people who don't require this tool, I think it's much simpler to remove the gem and a few lines of code, rather than investing the effort required to configure it for those who actually want it. The same idea applies to all the gems in the base. I think it's very unlikely to find a base that covers everything you need and in the way you need it. I think people use the base that most closely matches their needs and then change it as needed. |
@blacksam07 So first of all, I agree that we need to be thoughtful about what we add to a base app 👍 On the other hand, I don't see "feature flags" (or flipper) as a feature. Actually, for me it's more like a Platform tool, and that's why I think it's a good idea to have it in ALL of our projects. Because for me it shouldn't be considered an optional thing, instead I'd like to promote a way of working by encouraging continuous integration and delivery flows with the usage of Feature Flags. There are different kinds of feature flags, the ones I'm specifically referring to, are the "release toggles".
https://about.gitlab.com/handbook/product-development-flow/feature-flag-lifecycle |
@mrodriguez90 @santib thanks for your replies, I catch the both points, and I know that Also, this is a great job 👏 |
README.md
Outdated
- [flipper](https://github.com/jnunemaker/flipper) for feature flag support | ||
- [flipper-active_record](https://github.com/jnunemaker/flipper) for Flipper active_record support | ||
- [flipper-ui](https://github.com/jnunemaker/flipper) for UI-based feature flag management |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe in this place of the docs we can just say
- [Flipper](https://github.com/jnunemaker/flipper) for feature flag support
the activerecord and UI things can be implicit I guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can add a link in the admin panel, so that admins can easily navigate to the FFs page?
Oh I just realized there are configs for the UI just in case we want to remove the cloud recommendation or change the texts, adding a link back to the admin or something. Also, not sure if we need to add some mechanism to auto-create flags disabled by default. For example, if I, as a dev add a flag to the code, when I deploy the code I want the flag to automatically appear in the dashboard being disasbled. Right? Is that currently happening? |
@santib I'll add a new task for the UI changes. All FFs are disabled by default |
config.namespace :admin do |admin| | ||
admin.build_menu :default do |menu| | ||
menu.add label: 'Feature Flags', | ||
url: '/admin/feature-flags', html_options: { target: :blank } | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this adds the feature flag menu item to the active_admin page
@@ -24,6 +24,7 @@ | |||
RSpec.configure do |config| | |||
config.render_views = true | |||
config.include Devise::Test::ControllerHelpers, type: :controller | |||
config.include Devise::Test::IntegrationHelpers, type: :request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adds devise helpers like sign_in
to requests tests
@mrodriguez90 Just to clarify what I'm referring to, I think that it'd be good that FFs that are added in the codebase, appear in the Flipper UI automatically. e.g.
Is that currently happening? Or do we need to "register" the Feature Flag so that it appears automatically in the UI? |
@santib Oh I see, if a developer adds a feature flag in the code it won't show up in the UI automatically, someone has to register the flag first. This is how I imagine the dev experience:
Since these feature flags are stored in the database, I don't think there is an easy way to automatically register them from the codebase. |
@mrodriguez90 Maybe we can have them defined in a YML or a Ruby class, read it and create them. Maybe in a rake task that is automatically run on every release we can do something like namespace :feature_flags do
desc 'Initialize and turn off all the new feature flags so they are available in the UI'
task initialize: [:environment, :default_tenant] do
FeatureFlags::NAMES.each do |feature_flag|
next if Flipper.exist?(feature_flag)
Flipper.disable(feature_flag)
end
end
end or for example Gitlab also defines feature flags in more places than just calls in the codebase. I think that is a good idea not only for having them in the UI but also to keep track of all existing flags BTW. Maybe we can call leave it for another PR, but I think it'd be good to have a centralized place where we store the flags, and also use that for their initialization. That way we also prevent mistakes (e.g. typos) if a dev has to good to the UI and manually enter the flag name |
I like this, I'll add a new task to do it in a follow-up PR |
Purpose
Add a Feature Flag system to the rails_api_base using Flipper
Summary
AdminUser
with a valid session to access the Flipper UI.Context
Ruby community
Task
Notion task
Follow-Ups
This is what an
AdminUser
with a valid session will see