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

Disables Flipper memoization for test environment (Fixes Flipper middleware warning in Rails 7) #10501

Conversation

filipefurtad0
Copy link
Contributor

@filipefurtad0 filipefurtad0 commented Mar 1, 2023

On the rails 7 branch The build was displaying the error - Flipper::Middleware::Memoizer appears to be running twice. - This commit tries to fix that

The error mentions this approach https://github.com/jnunemaker/flipper/pull/523, but I could not really make flipper.preload work. I think one possible approach could be disabling memoization for the test environment.

Thinking out loud - If we wish to keep it we can as well enable it on our base spec helper by adding:

# in base_spec_helper.rb

RSpec.configure do |config|
  config.before(:each) do
    Flipper.instance = Flipper.new(Flipper::Adapters::Memory.new)
  end
end

What? Why?

  • prepares for Rails 7

What should we test?

Commit to be cherry picked into mkllnk:rails7 branch -> no testing needed here, just a

  • Green build in Rails 7 branch

Or

If we wish to merge it with master we should:

Test flipper:
Log in as a superadmin and verify that:

  • all features are listed
  • enabling/disabling features works as before

Release notes

Changelog Category: Technical changes

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

@filipefurtad0 filipefurtad0 marked this pull request as draft March 1, 2023 20:34
@filipefurtad0 filipefurtad0 changed the title Fixes Flipper middleware warning in Rails 7 [draft] Fixes Flipper middleware warning in Rails 7 Mar 1, 2023
@filipefurtad0 filipefurtad0 force-pushed the flipper_memoization_warning_rails7 branch from 8223eed to 2e1cb78 Compare March 6, 2023 13:55
@filipefurtad0 filipefurtad0 marked this pull request as ready for review March 6, 2023 14:01
@filipefurtad0 filipefurtad0 changed the title [draft] Fixes Flipper middleware warning in Rails 7 Fixes Flipper middleware warning in Rails 7 Mar 6, 2023
@filipefurtad0 filipefurtad0 changed the title Fixes Flipper middleware warning in Rails 7 Disables Flipper memoization for test environment (Fixes Flipper middleware warning in Rails 7) Mar 6, 2023
@mkllnk
Copy link
Member

mkllnk commented Mar 7, 2023

Cool, I didn't notice. I looked further into this and found that it's not the Rails upgrade causing this. It's just that the Rails upgrade also contains a Flipper upgrade and Flipper has some "breaking" changes:

We can actually upgrade flipper without Rails. I don't know why Dependabot isn't suggesting it but it may be because there are several gems that need to be updated together. I'll draft a PR.

@mkllnk
Copy link
Member

mkllnk commented Mar 7, 2023

I have an alternative solution in a new pull request. Are you happy to close this one?

@filipefurtad0
Copy link
Contributor Author

I have an alternative solution in a new pull request. Are you happy to close this one?

Sure @mkllnk , thank you for picking this up! 👍

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.

2 participants