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

Configure Strict adapter, introduce AdapterBuilder #763

Merged
merged 14 commits into from
Dec 5, 2023
Merged

Conversation

bkeepers
Copy link
Collaborator

@bkeepers bkeepers commented Sep 20, 2023

#760 adds a Strict adapter to warn when features don't exist. This configures it by default and sets it to :warn for now. But we should change it to :raise in test/development in 2.0.

Rails.application.configure do
  config.flipper.strict = false # or :warn or :raise
end

This also adds…

AdapterBuilder

There has never really been a good way to configure the Russian doll structure of adapters. Wrapping an adapter has required re-configuring the entire stack.

This introduces a #use method that can be used to configure adapters that wrap the primary storage adapter.

Flipper.configure do |config|
  config.use Flipper::Adapters::ActiveRecordCacheStore, Rails.cache

  # This is already configured by Flipper::Engine based on FLIPPER_STRICT env var or Rails.env
  # config.use Flipper::Adapters::Strict

  # Already configured by `flipper-active_record` gem
  # config.adapter { Flipper::Adapters::ActiveRecord.new }
end

TODO:

  • Add AdapterBuilder to make it easier to wrap/compose adapters
  • Configure Strict by default
  • Configure in Cloud

* strict:
  Remove kwarg for Strict handler, use arg or block
Base automatically changed from strict to main September 21, 2023 11:05
* origin/main: (88 commits)
  Lets see the body
  Temporarily put a bottom bun on the debug
  Show funding button
  Add free tier to UI
  Update readme
  Update readme
  Update readme
  Update gemspec so summary is up to date
  Rescue and log in post_to_pool too
  Minor shuffling of specs
  Minor reordering
  Make submitter a one time use thing
  Remove unused forwardable
  Move several cloud config bits to telemetry
  Simplifying cloud configuration options
  Minor formatting
  Update changelog
  Make reset private
  Remove unnecessary return
  Remove unused constants
  ...
* origin/main:
  protect against nil response
@bkeepers bkeepers marked this pull request as ready for review December 1, 2023 22:06
@bkeepers bkeepers requested a review from jnunemaker December 1, 2023 22:06
@bkeepers
Copy link
Collaborator Author

bkeepers commented Dec 1, 2023

I tested this in an app that uses flipper cloud and here's the console:

$ rails console
>> Flipper.enabled?(:nopenope)
Could not find feature "nopenope". Call `Flipper.add("nopenope")` to create it.
=> false


$ FLIPPER_STRICT=true rails console
>> Flipper.enabled?(:nopenope)
/Users/bkeepers/projects/flipper/lib/flipper/adapters/strict.rb:16:in `block in <class:Strict>': Could not find feature "nopenope". Call `Flipper.add("nopenope")` to create it. (Flipper::Adapters::Strict::NotFound)

Default is warn, setting FLIPPER_STRICT to true or raise will raise errors, false will do nothing.

Copy link
Collaborator

@jnunemaker jnunemaker left a comment

Choose a reason for hiding this comment

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

This looks great.

My only concern is performance. As long as we doc heavily (add to optimizations) that people will want to use strict = false in production or we default to strict = false in production, it should be ok.

I don't think we want to look up all the features for every enabled? check. People with memoize and/or preloading off would definitely be hurt.

lib/flipper/adapter_builder.rb Show resolved Hide resolved
@bkeepers
Copy link
Collaborator Author

bkeepers commented Dec 5, 2023

My only concern is performance. As long as we doc heavily (add to optimizations) that people will want to use strict = false in production or we default to strict = false in production, it should be ok.

Good call. 443742b changes the default for production to false. I'll update the optimization docs too.

@jnunemaker
Copy link
Collaborator

:shipit:

@bkeepers bkeepers merged commit eff8dae into main Dec 5, 2023
64 checks passed
@bkeepers bkeepers deleted the adapter_builder branch December 5, 2023 15:15
@SethHorsley
Copy link

What is the expected way to handle this in tests?

Should I add all the features in a fixture or in an initializer file for flipper?

It mentions this in the docs:

Adding feature flags to your app is as simple as adding conditionals
...

<% if Flipper.enabled? :new_feature, current_user %>
 <%= link_to "New Feature", new_feature_path %>
<% end %>

https://www.flippercloud.io/docs/guides/launch#add-feature-flags-to-your-app

It makes it seem like just adding the conditionals like Flipper.enabled? :new_feature is all you need but in tests this warns:

Could not find feature "deep_search". Call `Flipper.add("deep_search")` to create it.

@kwent
Copy link

kwent commented Jan 19, 2024

We need more guidance for tests, it's impossible for us to upgrade flipper without rewriting some specs.

Rails.application.configure do
  config.flipper.strict = false
end

Is not working in specs context.

@SethHorsley
Copy link

SethHorsley commented Jan 20, 2024

this is followed up here @kwent
#807

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.

4 participants