From 776b9767235b0d0bfc40d50735b60bab0392ddb6 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 17 Dec 2023 17:01:05 -0500 Subject: [PATCH 01/40] Demonstrate failure case --- spec/flipper/adapters/poll_spec.rb | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 spec/flipper/adapters/poll_spec.rb diff --git a/spec/flipper/adapters/poll_spec.rb b/spec/flipper/adapters/poll_spec.rb new file mode 100644 index 000000000..ee7da01e5 --- /dev/null +++ b/spec/flipper/adapters/poll_spec.rb @@ -0,0 +1,24 @@ +require 'flipper/adapters/poll' + +RSpec.describe Flipper::Adapters::Poll do + let(:remote_adapter) { + adapter = Flipper::Adapters::Memory.new(threadsafe: true) + flipper = Flipper.new(adapter) + flipper.enable(:search) + flipper.enable(:analytics) + adapter + } + let(:local_adapter) { Flipper::Adapters::Memory.new(threadsafe: true) } + let(:poller) { + Flipper::Poller.get("for_spec", { + start_automatically: false, + remote_adapter: remote_adapter, + }) + } + + it "syncs in main thread if local adapter is empty" do + instance = described_class.new(poller, local_adapter) + instance.features # call something to force sync + expect(local_adapter.features).to eq(remote_adapter.features) + end +end From 9cd0164f446a0ee3b561a31a40ecd974567e731c Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 17 Dec 2023 17:01:46 -0500 Subject: [PATCH 02/40] Fix cold start for adapters that have no state --- lib/flipper/adapters/poll.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/flipper/adapters/poll.rb b/lib/flipper/adapters/poll.rb index ac8e694db..2b7f321ee 100644 --- a/lib/flipper/adapters/poll.rb +++ b/lib/flipper/adapters/poll.rb @@ -18,6 +18,14 @@ def initialize(poller, adapter) @adapter = adapter @poller = poller @last_synced_at = 0 + + # If the adapter is empty, we need to sync before starting the poller. + # Yes, this will block the main thread, but that's better than thinking + # nothing is enabled. + if adapter.features.empty? + @poller.sync + end + @poller.start end From e9f188fef66a4e7f782cf2d00b1a509e2af9afd6 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 17 Dec 2023 17:05:01 -0500 Subject: [PATCH 03/40] Ensure that sync is not in main thread for non-empty adapters --- spec/flipper/adapters/poll_spec.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/spec/flipper/adapters/poll_spec.rb b/spec/flipper/adapters/poll_spec.rb index ee7da01e5..2fe08fe75 100644 --- a/spec/flipper/adapters/poll_spec.rb +++ b/spec/flipper/adapters/poll_spec.rb @@ -21,4 +21,21 @@ instance.features # call something to force sync expect(local_adapter.features).to eq(remote_adapter.features) end + + it "does not sync in main thread if local adapter is not empty" do + # make local not empty by importing remote + flipper = Flipper.new(local_adapter) + flipper.import(remote_adapter) + + # make a fake poller to verify calls + poller = double("Poller", last_synced_at: Concurrent::AtomicFixnum.new(0)) + expect(poller).to receive(:start).twice + expect(poller).not_to receive(:sync) + + # create new instance and call something to force sync + instance = described_class.new(poller, local_adapter) + instance.features # call something to force sync + + expect(local_adapter.features).to eq(remote_adapter.features) + end end From a1d2937a9e3d612c5d3286066d377dae35a3c1e9 Mon Sep 17 00:00:00 2001 From: Brandon Keepers Date: Thu, 11 Jan 2024 11:15:50 -0500 Subject: [PATCH 04/40] Generate a default initializer --- lib/generators/flipper/setup_generator.rb | 5 +++ .../flipper/templates/initializer.rb | 35 +++++++++++++++++++ .../flipper/setup_generator_test.rb | 5 +++ 3 files changed, 45 insertions(+) create mode 100644 lib/generators/flipper/templates/initializer.rb diff --git a/lib/generators/flipper/setup_generator.rb b/lib/generators/flipper/setup_generator.rb index c393c46d3..b6cb864fc 100644 --- a/lib/generators/flipper/setup_generator.rb +++ b/lib/generators/flipper/setup_generator.rb @@ -4,10 +4,15 @@ module Flipper module Generators class SetupGenerator < ::Rails::Generators::Base desc 'Peform any necessary steps to install Flipper' + source_paths << File.expand_path('templates', __dir__) class_option :token, type: :string, default: nil, aliases: '-t', desc: "Your personal environment token for Flipper Cloud" + def generate_initializer + template 'initializer.rb', 'config/initializers/flipper.rb' + end + def generate_active_record invoke 'flipper:active_record' if defined?(Flipper::Adapters::ActiveRecord) end diff --git a/lib/generators/flipper/templates/initializer.rb b/lib/generators/flipper/templates/initializer.rb new file mode 100644 index 000000000..bfcccf6ed --- /dev/null +++ b/lib/generators/flipper/templates/initializer.rb @@ -0,0 +1,35 @@ +Rails.application.configure do + ## Memoization ensures that only one adapter call is made per feature per request. + ## For more info, see https://www.flippercloud.io/docs/optimization#memoization + # config.flipper.memoize = true + + ## Flipper preloads all features before each request, which is recommended if: + ## * you have a limited number of features (< 100?) + ## * most of your requests depend on most of your features + ## * you have limited gate data combined across all features (< 1k enabled gates, like individual actors, across all features) + ## + ## For more info, see https://www.flippercloud.io/docs/optimization#preloading + # config.flipper.preload = [:only, :these, :common, :features] + + ## Warn or raise an error if an unknown feature is checked + ## Can be set to `:warn`, `:raise`, or `false` + # config.flipper.strict = Rails.env.development? && :warn + + ## Show Flipper checks in logs + # config.flipper.log = true + + ## Reconfigure Flipper to use the Memory adaper and disable Cloud in tests + # config.flipper.test_help = true + + ## The path that Flipper Cloud will use to sync features + # config.flipper.cloud_path = "_flipper" + + ## The instrumenter that Flipper will use. Defaults to ActiveSupport::Notifications. + # config.flipper.instrumenter = ActiveSupport::Notifications +end + +Flipper.configure do |config| + ## Configure other adapters that you want to use here: + ## See http://flippercloud.io/docs/adapters + # config.use Flipper::Adapters::ActiveSupportCacheStore, Rails.cache, expires_in: 5.minutes +end diff --git a/test_rails/generators/flipper/setup_generator_test.rb b/test_rails/generators/flipper/setup_generator_test.rb index 33a9cbd5b..1009d5278 100644 --- a/test_rails/generators/flipper/setup_generator_test.rb +++ b/test_rails/generators/flipper/setup_generator_test.rb @@ -17,6 +17,11 @@ class SetupGeneratorTest < Rails::Generators::TestCase end end + test "generates an initializer" do + run_generator + assert_file 'config/initializers/flipper.rb', /Flipper\.configure/ + end + test "does not invoke flipper:active_record generator if ActiveRecord adapter not defined" do # Ensure adapter not defined Flipper::Adapters.send(:remove_const, :ActiveRecord) rescue nil From aea25b0f927b580c0fb271ef50945669c2cbf3e4 Mon Sep 17 00:00:00 2001 From: Brandon Keepers Date: Thu, 11 Jan 2024 11:32:40 -0500 Subject: [PATCH 05/40] Document groups --- lib/generators/flipper/templates/initializer.rb | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/generators/flipper/templates/initializer.rb b/lib/generators/flipper/templates/initializer.rb index bfcccf6ed..007c1a849 100644 --- a/lib/generators/flipper/templates/initializer.rb +++ b/lib/generators/flipper/templates/initializer.rb @@ -9,7 +9,7 @@ ## * you have limited gate data combined across all features (< 1k enabled gates, like individual actors, across all features) ## ## For more info, see https://www.flippercloud.io/docs/optimization#preloading - # config.flipper.preload = [:only, :these, :common, :features] + # config.flipper.preload = true ## Warn or raise an error if an unknown feature is checked ## Can be set to `:warn`, `:raise`, or `false` @@ -33,3 +33,13 @@ ## See http://flippercloud.io/docs/adapters # config.use Flipper::Adapters::ActiveSupportCacheStore, Rails.cache, expires_in: 5.minutes end + +## Register a group that can be used for enabling features. +## +## Flipper.enable_group :my_feature, :admins +## +## See https://www.flippercloud.io/docs/features#enablement-group +# +# Flipper.register(:admins) do |actor| +# actor.respond_to?(:admin?) && actor.admin? +# end From 686a1174149971d59e3f2aa4fa474e640752e790 Mon Sep 17 00:00:00 2001 From: Stephen Crosby Date: Fri, 27 Sep 2024 19:14:43 -0700 Subject: [PATCH 06/40] Use reject(&:nil?) in enabled? check Rejecting nil has the advantage of allowing use of null object patterns using objects that implement nil? but are not strictly nil. This also works well with the 'actor cannot be nil' guard clause which uses `nil?` to check nilness. --- lib/flipper/feature.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/flipper/feature.rb b/lib/flipper/feature.rb index f446f921e..e734e32f9 100644 --- a/lib/flipper/feature.rb +++ b/lib/flipper/feature.rb @@ -100,7 +100,7 @@ def clear # # Returns true if enabled, false if not. def enabled?(*actors) - actors = actors.flatten.compact.map { |actor| Types::Actor.wrap(actor) } + actors = actors.flatten.reject(&:nil?).map { |actor| Types::Actor.wrap(actor) } actors = nil if actors.empty? # thing is left for backwards compatibility From e29a800b9bbf36c63c7b8cf425790fad7ae685c5 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Thu, 7 Nov 2024 12:53:50 -0500 Subject: [PATCH 07/40] Add comment about null object pattern --- lib/flipper/feature.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/flipper/feature.rb b/lib/flipper/feature.rb index e734e32f9..2a63f4311 100644 --- a/lib/flipper/feature.rb +++ b/lib/flipper/feature.rb @@ -100,6 +100,8 @@ def clear # # Returns true if enabled, false if not. def enabled?(*actors) + # Allows null object pattern. See PR for more. + # https://github.com/flippercloud/flipper/pull/887 actors = actors.flatten.reject(&:nil?).map { |actor| Types::Actor.wrap(actor) } actors = nil if actors.empty? From d91f5f357fb1061aeb567df6b1ce0d57f928be06 Mon Sep 17 00:00:00 2001 From: Stephen Crosby Date: Thu, 7 Nov 2024 13:53:09 -0800 Subject: [PATCH 08/40] Add test for actor implementing .nil? --- spec/flipper/feature_spec.rb | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/spec/flipper/feature_spec.rb b/spec/flipper/feature_spec.rb index d583af0e9..84c7fb75e 100644 --- a/spec/flipper/feature_spec.rb +++ b/spec/flipper/feature_spec.rb @@ -76,6 +76,26 @@ expect(subject.enabled?(actors)).to be(false) end end + + context "for an object that implements .nil? == true" do + let(:actor) { Flipper::Actor.new("User;1") } + + before do + def actor.nil? + true + end + end + + it 'returns true if feature is enabled' do + subject.enable + expect(subject.enabled?(actor)).to be(true) + end + + it 'returns false if feature is disabled' do + subject.disable + expect(subject.enabled?(actor)).to be(false) + end + end end describe '#to_s' do From 99edda229e6bac6b61d0b263198680de4a0b6459 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 20 Nov 2024 15:08:59 -0500 Subject: [PATCH 09/40] Add pstore for ruby 3.3.x --- Gemfile | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Gemfile b/Gemfile index 11e61e111..2a8dcbca0 100644 --- a/Gemfile +++ b/Gemfile @@ -12,10 +12,11 @@ gem 'statsd-ruby', '~> 1.2.1' gem 'rspec', '~> 3.0' gem 'rack-test' gem 'rackup', '= 1.0.0' -gem 'sqlite3', "~> #{ENV['SQLITE3_VERSION'] || '1.4.1'}" -gem 'rails', "~> #{ENV['RAILS_VERSION'] || '7.1'}" +gem 'sqlite3', "~> #{ENV['SQLITE3_VERSION'] || '2.1.0'}" +gem 'rails', "~> #{ENV['RAILS_VERSION'] || '8.0'}" gem 'minitest', '~> 5.18' gem 'minitest-documentation' +gem 'pstore' gem 'webmock' gem 'ice_age' gem 'redis-namespace' From 80ce8384dcd4b19376e2a334a965152f81243a64 Mon Sep 17 00:00:00 2001 From: Andrew Nesbitt Date: Sat, 30 Nov 2024 20:55:50 +0000 Subject: [PATCH 10/40] Add funding_uri to metadata.rb --- lib/flipper/metadata.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/flipper/metadata.rb b/lib/flipper/metadata.rb index ff0b1363c..fbe676e9e 100644 --- a/lib/flipper/metadata.rb +++ b/lib/flipper/metadata.rb @@ -7,5 +7,6 @@ module Flipper "source_code_uri" => "https://github.com/flippercloud/flipper", "bug_tracker_uri" => "https://github.com/flippercloud/flipper/issues", "changelog_uri" => "https://github.com/flippercloud/flipper/releases/tag/v#{Flipper::VERSION}", + "funding_uri" => "https://github.com/sponsors/flippercloud", }.freeze end From 0328e8ebaffa0b12d1afc4eadea1c13ded96bd2c Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 6 Jan 2025 23:45:48 +0000 Subject: [PATCH 11/40] Bump supercharge/mongodb-github-action from 1.11.0 to 1.12.0 Bumps [supercharge/mongodb-github-action](https://github.com/supercharge/mongodb-github-action) from 1.11.0 to 1.12.0. - [Release notes](https://github.com/supercharge/mongodb-github-action/releases) - [Changelog](https://github.com/supercharge/mongodb-github-action/blob/main/CHANGELOG.md) - [Commits](https://github.com/supercharge/mongodb-github-action/compare/1.11.0...1.12.0) --- updated-dependencies: - dependency-name: supercharge/mongodb-github-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] --- .github/workflows/ci.yml | 2 +- .github/workflows/examples.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 69035fa58..5415968e3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -84,7 +84,7 @@ jobs: - name: Setup memcached uses: KeisukeYamashita/memcached-actions@v1 - name: Start MongoDB - uses: supercharge/mongodb-github-action@1.11.0 + uses: supercharge/mongodb-github-action@1.12.0 with: mongodb-version: 4.0 - name: Check out repository code diff --git a/.github/workflows/examples.yml b/.github/workflows/examples.yml index ca8eb75a3..82319441a 100644 --- a/.github/workflows/examples.yml +++ b/.github/workflows/examples.yml @@ -66,7 +66,7 @@ jobs: - name: Setup memcached uses: KeisukeYamashita/memcached-actions@v1 - name: Start MongoDB - uses: supercharge/mongodb-github-action@1.11.0 + uses: supercharge/mongodb-github-action@1.12.0 with: mongodb-version: 4.0 - name: Check out repository code From 1141a27d49f690b1c0e02461a67d6724e874fac0 Mon Sep 17 00:00:00 2001 From: ur5us Date: Fri, 17 Jan 2025 16:28:47 +1300 Subject: [PATCH 12/40] Relax sanitize version constraint s. https://github.com/rgrove/sanitize/blob/main/CHANGELOG.md#700-2024-12-29 --- flipper-ui.gemspec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flipper-ui.gemspec b/flipper-ui.gemspec index 1124ccb41..b820a326d 100644 --- a/flipper-ui.gemspec +++ b/flipper-ui.gemspec @@ -25,5 +25,5 @@ Gem::Specification.new do |gem| gem.add_dependency 'rack-session', '>= 1.0.2', '< 3.0.0' gem.add_dependency 'flipper', "~> #{Flipper::VERSION}" gem.add_dependency 'erubi', '>= 1.0.0', '< 2.0.0' - gem.add_dependency 'sanitize', '< 7' + gem.add_dependency 'sanitize', '< 8' end From 7de251eee84572cb8f1a4e7ca4435d353a5d526b Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Tue, 4 Feb 2025 14:21:11 -0500 Subject: [PATCH 13/40] Add cloud config instrument and instrument telemetry errors/retries --- lib/flipper/cloud/configuration.rb | 4 ++++ lib/flipper/cloud/telemetry/submitter.rb | 2 ++ 2 files changed, 6 insertions(+) diff --git a/lib/flipper/cloud/configuration.rb b/lib/flipper/cloud/configuration.rb index d6493e62a..930ba7742 100644 --- a/lib/flipper/cloud/configuration.rb +++ b/lib/flipper/cloud/configuration.rb @@ -135,6 +135,10 @@ def log(message, level: :debug) logger.send(level, "name=flipper_cloud #{message}") end + def instrument(name, payload = {}, &block) + instrumenter.instrument(name, payload, &block) + end + private def app_adapter diff --git a/lib/flipper/cloud/telemetry/submitter.rb b/lib/flipper/cloud/telemetry/submitter.rb index 2863fd046..6022e77da 100644 --- a/lib/flipper/cloud/telemetry/submitter.rb +++ b/lib/flipper/cloud/telemetry/submitter.rb @@ -51,6 +51,7 @@ def to_body(drained) Typecast.to_gzip(json) rescue => exception + @cloud_configuration.instrument "telemetry_error.#{Flipper::InstrumentationNamespace}", exception: exception, request_id: request_id @cloud_configuration.log "action=to_body request_id=#{request_id} error=#{exception.inspect}", level: :error end @@ -63,6 +64,7 @@ def retry_with_backoff(attempts, &block) result, should_retry = yield return [result, nil] unless should_retry rescue => error + @cloud_configuration.instrument "telemetry_retry.#{Flipper::InstrumentationNamespace}", attempts_remaining: attempts_remaining, exception: error @cloud_configuration.log "action=post_to_cloud attempts_remaining=#{attempts_remaining} error=#{error.inspect}", level: :error should_retry = true caught_exception = error From 0a0c82491278c3dd257d19a279308fc27f210c6d Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Tue, 4 Feb 2025 14:57:48 -0500 Subject: [PATCH 14/40] Increase default backoff to be more friendly The old version was attacking us. This version is much more hey you are doing your best and i apreciate that so i'll chill over here while you have a moment. --- examples/cloud/backoff_policy.rb | 2 +- lib/flipper/cloud/telemetry.rb | 2 +- lib/flipper/cloud/telemetry/backoff_policy.rb | 9 ++++++--- lib/flipper/cloud/telemetry/submitter.rb | 2 +- spec/flipper/cloud/telemetry/backoff_policy_spec.rb | 6 +++--- spec/flipper/cloud/telemetry/submitter_spec.rb | 8 ++++---- spec/flipper/cloud/telemetry_spec.rb | 4 ++-- 7 files changed, 18 insertions(+), 15 deletions(-) diff --git a/examples/cloud/backoff_policy.rb b/examples/cloud/backoff_policy.rb index 5f4fb62bb..5319ea1f9 100644 --- a/examples/cloud/backoff_policy.rb +++ b/examples/cloud/backoff_policy.rb @@ -5,7 +5,7 @@ intervals = [] policy = Flipper::Cloud::Telemetry::BackoffPolicy.new -10.times do |n| +5.times do |n| intervals << policy.next_interval end diff --git a/lib/flipper/cloud/telemetry.rb b/lib/flipper/cloud/telemetry.rb index 05d232a52..e5fdfd252 100644 --- a/lib/flipper/cloud/telemetry.rb +++ b/lib/flipper/cloud/telemetry.rb @@ -76,7 +76,7 @@ def start @metric_storage = MetricStorage.new @pool = Concurrent::FixedThreadPool.new(2, { - max_queue: 5, + max_queue: 20, # ~ 20 minutes of data at 1 minute intervals fallback_policy: :discard, name: "flipper-telemetry-post-to-cloud-pool".freeze, }) diff --git a/lib/flipper/cloud/telemetry/backoff_policy.rb b/lib/flipper/cloud/telemetry/backoff_policy.rb index 68349bcce..97e6ee2ab 100644 --- a/lib/flipper/cloud/telemetry/backoff_policy.rb +++ b/lib/flipper/cloud/telemetry/backoff_policy.rb @@ -3,10 +3,10 @@ module Cloud class Telemetry class BackoffPolicy # Private: The default minimum timeout between intervals in milliseconds. - MIN_TIMEOUT_MS = 1_000 + MIN_TIMEOUT_MS = 30_000 # Private: The default maximum timeout between intervals in milliseconds. - MAX_TIMEOUT_MS = 30_000 + MAX_TIMEOUT_MS = 120_000 # Private: The value to multiply the current interval with for each # retry attempt. @@ -67,7 +67,10 @@ def next_interval @attempts += 1 - [interval, @max_timeout_ms].min + # cap the interval to the max timeout + result = [interval, @max_timeout_ms].min + # jitter even when maxed out + result == @max_timeout_ms ? add_jitter(result, 0.05) : result end def reset diff --git a/lib/flipper/cloud/telemetry/submitter.rb b/lib/flipper/cloud/telemetry/submitter.rb index 6022e77da..e5678365f 100644 --- a/lib/flipper/cloud/telemetry/submitter.rb +++ b/lib/flipper/cloud/telemetry/submitter.rb @@ -34,7 +34,7 @@ def call(drained) return if drained.empty? body = to_body(drained) return if body.nil? || body.empty? - retry_with_backoff(10) { submit(body) } + retry_with_backoff(5) { submit(body) } end private diff --git a/spec/flipper/cloud/telemetry/backoff_policy_spec.rb b/spec/flipper/cloud/telemetry/backoff_policy_spec.rb index 03a9d1e82..0b9d6ff2c 100644 --- a/spec/flipper/cloud/telemetry/backoff_policy_spec.rb +++ b/spec/flipper/cloud/telemetry/backoff_policy_spec.rb @@ -4,8 +4,8 @@ context "#initialize" do it "with no options" do policy = described_class.new - expect(policy.min_timeout_ms).to eq(1_000) - expect(policy.max_timeout_ms).to eq(30_000) + expect(policy.min_timeout_ms).to eq(30_000) + expect(policy.max_timeout_ms).to eq(120_000) expect(policy.multiplier).to eq(1.5) expect(policy.randomization_factor).to eq(0.5) end @@ -87,7 +87,7 @@ randomization_factor: 0.5, }) 10.times { policy.next_interval } - expect(policy.next_interval).to eq(10_000) + expect(policy.next_interval).to be_within(10_000*0.1).of(10_000) end end diff --git a/spec/flipper/cloud/telemetry/submitter_spec.rb b/spec/flipper/cloud/telemetry/submitter_spec.rb index afceb39ec..b20c0bc56 100644 --- a/spec/flipper/cloud/telemetry/submitter_spec.rb +++ b/spec/flipper/cloud/telemetry/submitter_spec.rb @@ -71,15 +71,15 @@ to_return(status: 429, body: "{}"). to_return(status: 200, body: "{}") instance = described_class.new(cloud_configuration) - expect(instance.backoff_policy.min_timeout_ms).to eq(1_000) - expect(instance.backoff_policy.max_timeout_ms).to eq(30_000) + expect(instance.backoff_policy.min_timeout_ms).to eq(30_000) + expect(instance.backoff_policy.max_timeout_ms).to eq(120_000) end it "tries 10 times by default" do stub_request(:post, "https://www.flippercloud.io/adapter/telemetry"). to_return(status: 500, body: "{}") subject.call(enabled_metrics) - expect(subject.backoff_policy.retries).to eq(9) # 9 retries + 1 initial attempt + expect(subject.backoff_policy.retries).to eq(4) # 4 retries + 1 initial attempt end [ @@ -105,7 +105,7 @@ stub_request(:post, "https://www.flippercloud.io/adapter/telemetry"). to_raise(error_class) subject.call(enabled_metrics) - expect(subject.backoff_policy.retries).to eq(9) + expect(subject.backoff_policy.retries).to eq(4) end end diff --git a/spec/flipper/cloud/telemetry_spec.rb b/spec/flipper/cloud/telemetry_spec.rb index 9a9299969..8c60d5dac 100644 --- a/spec/flipper/cloud/telemetry_spec.rb +++ b/spec/flipper/cloud/telemetry_spec.rb @@ -122,7 +122,7 @@ # Check the conig interval and the timer interval. expect(telemetry.interval).to eq(120) expect(telemetry.timer.execution_interval).to eq(120) - expect(stub).to have_been_requested.times(10) + expect(stub).to have_been_requested.times(5) end it "doesn't try to update telemetry interval from error if not response error" do @@ -152,7 +152,7 @@ expect(telemetry.interval).to eq(60) expect(telemetry.timer.execution_interval).to eq(60) - expect(stub).to have_been_requested.times(10) + expect(stub).to have_been_requested.times(5) end describe '#record' do From 0dc4071a38045086914f3b96f41160a2d6662b0a Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Tue, 4 Feb 2025 15:53:05 -0500 Subject: [PATCH 15/40] Lower to 1 thread pool Reduces how many threads are trying to talk to us. --- lib/flipper/cloud/telemetry.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/flipper/cloud/telemetry.rb b/lib/flipper/cloud/telemetry.rb index e5fdfd252..0680f7487 100644 --- a/lib/flipper/cloud/telemetry.rb +++ b/lib/flipper/cloud/telemetry.rb @@ -75,7 +75,7 @@ def start @metric_storage = MetricStorage.new - @pool = Concurrent::FixedThreadPool.new(2, { + @pool = Concurrent::FixedThreadPool.new(1, { max_queue: 20, # ~ 20 minutes of data at 1 minute intervals fallback_policy: :discard, name: "flipper-telemetry-post-to-cloud-pool".freeze, From 8e971f14e27f013ae0ec4c0c3bed939600436375 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Tue, 4 Feb 2025 16:03:15 -0500 Subject: [PATCH 16/40] Lock concurrent-ruby Hitting issue with logger: ``` uninitialized constant ActiveSupport::LoggerThreadSafeLevel::Logger (NameError) ``` Stack overflow says this is the way. --- Gemfile | 1 + 1 file changed, 1 insertion(+) diff --git a/Gemfile b/Gemfile index 2a8dcbca0..1a5c0b39e 100644 --- a/Gemfile +++ b/Gemfile @@ -6,6 +6,7 @@ Dir['flipper-*.gemspec'].each do |gemspec| gemspec(name: "flipper-#{plugin}", development_group: plugin) end +gem 'concurrent-ruby', '1.3.4' gem 'debug' gem 'rake' gem 'statsd-ruby', '~> 1.2.1' From 1ccf098cda84f13090855fdb175b1cb5c50ee9aa Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Tue, 4 Feb 2025 16:41:49 -0500 Subject: [PATCH 17/40] Get specs passing Now they all try to do a sync so let's make the get_all more flexible and stub the rest. --- lib/flipper/adapters/http.rb | 4 ++-- spec/flipper/cloud_spec.rb | 13 +++++++++---- spec/flipper/engine_spec.rb | 1 + 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/lib/flipper/adapters/http.rb b/lib/flipper/adapters/http.rb index cf190b310..ec82b96c4 100644 --- a/lib/flipper/adapters/http.rb +++ b/lib/flipper/adapters/http.rb @@ -59,8 +59,8 @@ def get_all response = @client.get("/features?exclude_gate_names=true") raise Error, response unless response.is_a?(Net::HTTPOK) - parsed_response = Typecast.from_json(response.body) - parsed_features = parsed_response.fetch('features') + parsed_response = response.body.empty? ? {} : Typecast.from_json(response.body) + parsed_features = parsed_response['features'] || [] gates_by_key = parsed_features.each_with_object({}) do |parsed_feature, hash| hash[parsed_feature['key']] = parsed_feature['gates'] hash diff --git a/spec/flipper/cloud_spec.rb b/spec/flipper/cloud_spec.rb index b2e1685c6..ef436105e 100644 --- a/spec/flipper/cloud_spec.rb +++ b/spec/flipper/cloud_spec.rb @@ -43,6 +43,7 @@ context 'initialize with token and options' do it 'sets correct url' do + stub_request(:any, %r{fakeflipper.com}).to_return(status: 200) instance = described_class.new(token: 'asdf', url: 'https://www.fakeflipper.com/sadpanda') # pardon the nesting... memoized = instance.adapter @@ -78,27 +79,31 @@ end it 'can set debug_output' do + instance = Flipper::Adapters::Http::Client.new(token: 'asdf', url: 'https://www.flippercloud.io/adapter') expect(Flipper::Adapters::Http::Client).to receive(:new) - .with(hash_including(debug_output: STDOUT)).at_least(:once) + .with(hash_including(debug_output: STDOUT)).at_least(:once).and_return(instance) described_class.new(token: 'asdf', debug_output: STDOUT) end it 'can set read_timeout' do + instance = Flipper::Adapters::Http::Client.new(token: 'asdf', url: 'https://www.flippercloud.io/adapter') expect(Flipper::Adapters::Http::Client).to receive(:new) - .with(hash_including(read_timeout: 1)).at_least(:once) + .with(hash_including(read_timeout: 1)).at_least(:once).and_return(instance) described_class.new(token: 'asdf', read_timeout: 1) end it 'can set open_timeout' do + instance = Flipper::Adapters::Http::Client.new(token: 'asdf', url: 'https://www.flippercloud.io/adapter') expect(Flipper::Adapters::Http::Client).to receive(:new) - .with(hash_including(open_timeout: 1)).at_least(:once) + .with(hash_including(open_timeout: 1)).at_least(:once).and_return(instance) described_class.new(token: 'asdf', open_timeout: 1) end if RUBY_VERSION >= '2.6.0' it 'can set write_timeout' do + instance = Flipper::Adapters::Http::Client.new(token: 'asdf', url: 'https://www.flippercloud.io/adapter') expect(Flipper::Adapters::Http::Client).to receive(:new) - .with(hash_including(open_timeout: 1)).at_least(:once) + .with(hash_including(open_timeout: 1)).at_least(:once).and_return(instance) described_class.new(token: 'asdf', open_timeout: 1) end end diff --git a/spec/flipper/engine_spec.rb b/spec/flipper/engine_spec.rb index efad364b5..36b4a6967 100644 --- a/spec/flipper/engine_spec.rb +++ b/spec/flipper/engine_spec.rb @@ -12,6 +12,7 @@ end before do + stub_request(:get, /flippercloud\.io/).to_return(status: 200, body: "{}") Rails.application = nil ActiveSupport::Dependencies.autoload_paths = ActiveSupport::Dependencies.autoload_paths.dup ActiveSupport::Dependencies.autoload_once_paths = ActiveSupport::Dependencies.autoload_once_paths.dup From 67b03ca74082f050a43468be59143ef14da21be7 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Tue, 4 Feb 2025 16:47:10 -0500 Subject: [PATCH 18/40] Add note about possible empty data --- lib/flipper/adapters/poll.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/flipper/adapters/poll.rb b/lib/flipper/adapters/poll.rb index 2b7f321ee..5c9269f50 100644 --- a/lib/flipper/adapters/poll.rb +++ b/lib/flipper/adapters/poll.rb @@ -23,7 +23,14 @@ def initialize(poller, adapter) # Yes, this will block the main thread, but that's better than thinking # nothing is enabled. if adapter.features.empty? - @poller.sync + begin + @poller.sync + rescue => error + # TODO: Warn here that it's possible that no data has been synced + # and flags are being evaluated without flag data being present + # until a sync completes. We rescue to avoid flipper being down + # causing your processes to crash. + end end @poller.start From 74cc1cda153945372dc0f922a8ece6d0c0f9e5ee Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Tue, 18 Feb 2025 17:28:47 -0500 Subject: [PATCH 19/40] Add redis connection pool adapter --- Gemfile | 1 + lib/flipper/adapters/redis_connection_pool.rb | 239 ++++++++++++++++++ .../adapters/redis_connection_pool_spec.rb | 60 +++++ 3 files changed, 300 insertions(+) create mode 100644 lib/flipper/adapters/redis_connection_pool.rb create mode 100644 spec/flipper/adapters/redis_connection_pool_spec.rb diff --git a/Gemfile b/Gemfile index 1a5c0b39e..b4551fa64 100644 --- a/Gemfile +++ b/Gemfile @@ -7,6 +7,7 @@ Dir['flipper-*.gemspec'].each do |gemspec| end gem 'concurrent-ruby', '1.3.4' +gem 'connection_pool' gem 'debug' gem 'rake' gem 'statsd-ruby', '~> 1.2.1' diff --git a/lib/flipper/adapters/redis_connection_pool.rb b/lib/flipper/adapters/redis_connection_pool.rb new file mode 100644 index 000000000..28e761141 --- /dev/null +++ b/lib/flipper/adapters/redis_connection_pool.rb @@ -0,0 +1,239 @@ +require 'set' +require 'redis' +require 'flipper' +require 'connection_pool' + +module Flipper + module Adapters + class RedisConnectionPool + include ::Flipper::Adapter + + def self.default_pool + return @default_pool if defined?(@default_pool) + + size = ENV.fetch('FLIPPER_REDIS_POOL_SIZE', 5).to_i + timeout = ENV.fetch('FLIPPER_REDIS_POOL_TIMEOUT', 5).to_i + @default_pool = ConnectionPool.new(size: size, timeout: timeout) do + Redis.new(url: ENV["FLIPPER_REDIS_URL"] || ENV["REDIS_URL"]) + end + end + + attr_reader :key_prefix + + def features_key + "#{key_prefix}flipper_features" + end + + def key_for(feature_name) + "#{key_prefix}#{feature_name}" + end + + # Public: Initializes a Redis flipper adapter. + # + # pool - The Redis connection pool to use. + # key_prefix - an optional prefix with which to namespace + # flipper's Redis keys + def initialize(pool, key_prefix: nil) + @pool = pool + @key_prefix = key_prefix + @sadd_returns_boolean = with_connection do |conn| + conn.class.respond_to?(:sadd_returns_boolean) && conn.class.sadd_returns_boolean + end + end + + # Public: The set of known features. + def features + read_feature_keys + end + + # Public: Adds a feature to the set of known features. + def add(feature) + if redis_sadd_returns_boolean? + with_connection { |conn| conn.sadd? features_key, feature.key } + else + with_connection { |conn| conn.sadd features_key, feature.key } + end + true + end + + # Public: Removes a feature from the set of known features. + def remove(feature) + if redis_sadd_returns_boolean? + with_connection { |conn| conn.srem? features_key, feature.key } + else + with_connection { |conn| conn.srem features_key, feature.key } + end + with_connection { |conn| conn.del key_for(feature.key) } + true + end + + # Public: Clears the gate values for a feature. + def clear(feature) + with_connection { |conn| conn.del key_for(feature.key) } + true + end + + # Public: Gets the values for all gates for a given feature. + # + # Returns a Hash of Flipper::Gate#key => value. + def get(feature) + doc = with_connection { |conn| doc_for(conn, feature) } + result_for_feature(feature, doc) + end + + def get_multi(features) + read_many_features(features) + end + + def get_all + features = read_feature_keys.map { |key| Flipper::Feature.new(key, self) } + read_many_features(features) + end + + # Public: Enables a gate for a given thing. + # + # feature - The Flipper::Feature for the gate. + # gate - The Flipper::Gate to enable. + # thing - The Flipper::Type being enabled for the gate. + # + # Returns true. + def enable(feature, gate, thing) + feature_key = key_for(feature.key) + case gate.data_type + when :boolean + clear(feature) + with_connection do |conn| + conn.hset feature_key, gate.key, thing.value.to_s + end + when :integer + with_connection do |conn| + conn.hset feature_key, gate.key, thing.value.to_s + end + when :set + with_connection do |conn| + conn.hset feature_key, to_field(gate, thing), 1 + end + when :json + with_connection do |conn| + conn.hset feature_key, gate.key, Typecast.to_json(thing.value) + end + else + unsupported_data_type gate.data_type + end + + true + end + + # Public: Disables a gate for a given thing. + # + # feature - The Flipper::Feature for the gate. + # gate - The Flipper::Gate to disable. + # thing - The Flipper::Type being disabled for the gate. + # + # Returns true. + def disable(feature, gate, thing) + feature_key = key_for(feature.key) + case gate.data_type + when :boolean + with_connection { |conn| conn.del feature_key } + when :integer + with_connection { |conn| conn.hset feature_key, gate.key, thing.value.to_s } + when :set + with_connection { |conn| conn.hdel feature_key, to_field(gate, thing) } + when :json + with_connection { |conn| conn.hdel feature_key, gate.key } + else + unsupported_data_type gate.data_type + end + + true + end + + private + + def with_connection(&block) + @pool.with(&block) + end + + def redis_sadd_returns_boolean? + @sadd_returns_boolean + end + + def read_many_features(features) + docs = docs_for(features) + result = {} + features.zip(docs) do |feature, doc| + result[feature.key] = result_for_feature(feature, doc) + end + result + end + + def read_feature_keys + with_connection { |conn| conn.smembers(features_key).to_set } + end + + def doc_for(connection, feature) + connection.hgetall(key_for(feature.key)) + end + + def docs_for(features) + with_connection do |conn| + conn.pipelined do |pipeline| + features.each do |feature| + doc_for(pipeline, feature) + end + end + end + end + + def result_for_feature(feature, doc) + result = {} + fields = doc.keys + + feature.gates.each do |gate| + result[gate.key] = + case gate.data_type + when :boolean, :integer + doc[gate.key.to_s] + when :set + fields_to_gate_value fields, gate + when :json + value = doc[gate.key.to_s] + Typecast.from_json(value) + else + unsupported_data_type gate.data_type + end + end + + result + end + + # Private: Converts gate and thing to hash key. + def to_field(gate, thing) + "#{gate.key}/#{thing.value}" + end + + # Private: Returns a set of values given an array of fields and a gate. + # + # Returns a Set of the values enabled for the gate. + def fields_to_gate_value(fields, gate) + regex = %r{^#{Regexp.escape(gate.key.to_s)}/} + keys = fields.grep(regex) + values = keys.map { |key| key.split('/', 2).last } + values.to_set + end + + # Private + def unsupported_data_type(data_type) + raise "#{data_type} is not supported by this adapter" + end + end + end +end + +Flipper.configure do |config| + config.adapter do + default_pool = Flipper::Adapters::RedisConnectionPool.default_pool + Flipper::Adapters::RedisConnectionPool.new(default_pool) + end +end diff --git a/spec/flipper/adapters/redis_connection_pool_spec.rb b/spec/flipper/adapters/redis_connection_pool_spec.rb new file mode 100644 index 000000000..f68b03105 --- /dev/null +++ b/spec/flipper/adapters/redis_connection_pool_spec.rb @@ -0,0 +1,60 @@ +require 'flipper/adapters/redis_connection_pool' + +RSpec.describe Flipper::Adapters::RedisConnectionPool do + let(:pool) do + options = {} + + options[:url] = ENV['REDIS_URL'] if ENV['REDIS_URL'] + + Redis.raise_deprecations = true + + ConnectionPool.new(size: 5, timeout: 5) { Redis.new(options) } + end + + subject { described_class.new(pool) } + + before do + skip_on_error(Redis::CannotConnectError, 'Redis not available') do + pool.with { |conn| conn.flushdb } + end + end + + it_should_behave_like 'a flipper adapter' + + it 'configures itself on load' do + Flipper.configuration = nil + Flipper.instance = nil + + silence { load 'flipper/adapters/redis_connection_pool.rb' } + + expect(Flipper.adapter.adapter).to be_a(Flipper::Adapters::RedisConnectionPool) + end + + describe 'with a key_prefix' do + let(:subject) { described_class.new(pool, key_prefix: "lockbox:") } + let(:feature) { Flipper::Feature.new(:search, subject) } + + it_should_behave_like 'a flipper adapter' + + it 'namespaces feature-keys' do + subject.add(feature) + + pool.with do |conn| + expect(conn.smembers("flipper_features")).to eq([]) + expect(conn.exists?("search")).to eq(false) + expect(conn.smembers("lockbox:flipper_features")).to eq(["search"]) + expect(conn.hgetall("lockbox:search")).not_to eq(nil) + end + end + + it "can remove namespaced keys" do + subject.add(feature) + + pool.with do |conn| + expect(conn.smembers("lockbox:flipper_features")).to eq(["search"]) + subject.remove(feature) + expect(conn.smembers("lockbox:flipper_features")).to be_empty + end + end + end +end From f99f3c3ee631577dc2b345d786655e4c5f7b17a6 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 08:36:35 -0500 Subject: [PATCH 20/40] Fix which redis module is being used It was trying to use the redis adapter instead of the redis-rb stuff. This forces it to the top namespace. --- lib/flipper/adapters/redis_connection_pool.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/flipper/adapters/redis_connection_pool.rb b/lib/flipper/adapters/redis_connection_pool.rb index 28e761141..a7abaa914 100644 --- a/lib/flipper/adapters/redis_connection_pool.rb +++ b/lib/flipper/adapters/redis_connection_pool.rb @@ -13,9 +13,10 @@ def self.default_pool size = ENV.fetch('FLIPPER_REDIS_POOL_SIZE', 5).to_i timeout = ENV.fetch('FLIPPER_REDIS_POOL_TIMEOUT', 5).to_i - @default_pool = ConnectionPool.new(size: size, timeout: timeout) do - Redis.new(url: ENV["FLIPPER_REDIS_URL"] || ENV["REDIS_URL"]) - end + + @default_pool = ConnectionPool.new(size: size, timeout: timeout) { + ::Redis.new(url: ENV["FLIPPER_REDIS_URL"] || ENV["REDIS_URL"] || "redis://localhost:6379") + } end attr_reader :key_prefix @@ -233,7 +234,8 @@ def unsupported_data_type(data_type) Flipper.configure do |config| config.adapter do - default_pool = Flipper::Adapters::RedisConnectionPool.default_pool - Flipper::Adapters::RedisConnectionPool.new(default_pool) + Flipper::Adapters::RedisConnectionPool.new( + Flipper::Adapters::RedisConnectionPool.default_pool + ) end end From 3668f527a541581085fea77ab9496f3af26cdff0 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 08:37:51 -0500 Subject: [PATCH 21/40] Minor cleanup of client initialization for redis Might as well add default just to make it easier for people to get going. --- lib/flipper/adapters/redis.rb | 2 +- spec/flipper/adapters/redis_cache_spec.rb | 4 +--- .../adapters/redis_connection_pool_spec.rb | 9 +++------ spec/flipper/adapters/redis_spec.rb | 6 +----- test/adapters/redis_connection_pool_test.rb | 17 +++++++++++++++++ 5 files changed, 23 insertions(+), 15 deletions(-) create mode 100644 test/adapters/redis_connection_pool_test.rb diff --git a/lib/flipper/adapters/redis.rb b/lib/flipper/adapters/redis.rb index 6e59d4acd..6d85d93be 100644 --- a/lib/flipper/adapters/redis.rb +++ b/lib/flipper/adapters/redis.rb @@ -208,7 +208,7 @@ def unsupported_data_type(data_type) Flipper.configure do |config| config.adapter do - client = Redis.new(url: ENV["FLIPPER_REDIS_URL"] || ENV["REDIS_URL"]) + client = Redis.new(url: ENV["FLIPPER_REDIS_URL"] || ENV["REDIS_URL"] || "redis://localhost:6379") Flipper::Adapters::Redis.new(client) end end diff --git a/spec/flipper/adapters/redis_cache_spec.rb b/spec/flipper/adapters/redis_cache_spec.rb index b25c92eb4..adf371bf0 100644 --- a/spec/flipper/adapters/redis_cache_spec.rb +++ b/spec/flipper/adapters/redis_cache_spec.rb @@ -3,9 +3,7 @@ RSpec.describe Flipper::Adapters::RedisCache do let(:client) do - options = {} - options[:url] = ENV['REDIS_URL'] if ENV['REDIS_URL'] - Redis.new(options) + Redis.new(url: ENV.fetch('REDIS_URL', 'redis://localhost:6379')) end let(:memory_adapter) do diff --git a/spec/flipper/adapters/redis_connection_pool_spec.rb b/spec/flipper/adapters/redis_connection_pool_spec.rb index f68b03105..f5917e7f7 100644 --- a/spec/flipper/adapters/redis_connection_pool_spec.rb +++ b/spec/flipper/adapters/redis_connection_pool_spec.rb @@ -2,13 +2,10 @@ RSpec.describe Flipper::Adapters::RedisConnectionPool do let(:pool) do - options = {} - - options[:url] = ENV['REDIS_URL'] if ENV['REDIS_URL'] - Redis.raise_deprecations = true - - ConnectionPool.new(size: 5, timeout: 5) { Redis.new(options) } + ConnectionPool.new(size: 5, timeout: 5) { + Redis.new(url: ENV.fetch('REDIS_URL', 'redis://localhost:6379')) + } end subject { described_class.new(pool) } diff --git a/spec/flipper/adapters/redis_spec.rb b/spec/flipper/adapters/redis_spec.rb index b4fb7472d..153d41e24 100644 --- a/spec/flipper/adapters/redis_spec.rb +++ b/spec/flipper/adapters/redis_spec.rb @@ -2,12 +2,8 @@ RSpec.describe Flipper::Adapters::Redis do let(:client) do - options = {} - - options[:url] = ENV['REDIS_URL'] if ENV['REDIS_URL'] - Redis.raise_deprecations = true - Redis.new(options) + Redis.new(url: ENV.fetch('REDIS_URL', 'redis://localhost:6379')) end subject { described_class.new(client) } diff --git a/test/adapters/redis_connection_pool_test.rb b/test/adapters/redis_connection_pool_test.rb new file mode 100644 index 000000000..f497a7c9c --- /dev/null +++ b/test/adapters/redis_connection_pool_test.rb @@ -0,0 +1,17 @@ +require 'test_helper' +require 'flipper/adapters/redis' + +class RedisConnectionPoolTest < MiniTest::Test + prepend Flipper::Test::SharedAdapterTests + + def setup + url = ENV.fetch('REDIS_URL', 'redis://localhost:6379') + pool = ConnectionPool.new(size: 5, timeout: 5) { + Redis.new(url: url) + } + @adapter = Flipper::Adapters::RedisConnectionPool.new(pool) + pool.with { |client| client.flushdb } + rescue Redis::CannotConnectError + ENV['CI'] ? raise : skip('Redis not available') + end +end From 5ac923518f05ab3a8c0e78f8a96e1462d82741a3 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 09:20:24 -0500 Subject: [PATCH 22/40] Allow redis or redis cache adapters work with connection pool --- Guardfile | 7 + lib/flipper/adapters/redis.rb | 53 ++-- lib/flipper/adapters/redis_cache.rb | 12 +- lib/flipper/adapters/redis_connection_pool.rb | 241 ------------------ lib/flipper/adapters/redis_shared/methods.rb | 9 + .../adapters/redis_connection_pool_spec.rb | 57 ----- spec/flipper/adapters/redis_spec.rb | 110 +++++--- test/adapters/redis_connection_pool_test.rb | 17 -- 8 files changed, 136 insertions(+), 370 deletions(-) delete mode 100644 lib/flipper/adapters/redis_connection_pool.rb create mode 100644 lib/flipper/adapters/redis_shared/methods.rb delete mode 100644 spec/flipper/adapters/redis_connection_pool_spec.rb delete mode 100644 test/adapters/redis_connection_pool_test.rb diff --git a/Guardfile b/Guardfile index e58b005cc..760ab141d 100644 --- a/Guardfile +++ b/Guardfile @@ -29,6 +29,13 @@ guard 'rspec', rspec_options do 'spec/flipper/adapters/active_support_cache_store_spec.rb', ] } + watch('lib/flipper/adapters/redis_shared/methods.rb') { + [ + 'spec/flipper/adapters/redis_spec.rb', + 'spec/flipper/adapters/redis_cache_spec.rb', + 'spec/flipper/adapters/redis_connection_pool_spec.rb', + ] + } # To run all specs on every change... (useful with focus and fit) # watch(%r{.*}) { 'spec' } diff --git a/lib/flipper/adapters/redis.rb b/lib/flipper/adapters/redis.rb index 6d85d93be..0a5b9ee91 100644 --- a/lib/flipper/adapters/redis.rb +++ b/lib/flipper/adapters/redis.rb @@ -1,11 +1,13 @@ require 'set' require 'redis' require 'flipper' +require 'flipper/adapters/redis_shared/methods' module Flipper module Adapters class Redis include ::Flipper::Adapter + include ::Flipper::Adapters::RedisShared attr_reader :key_prefix @@ -25,6 +27,9 @@ def key_for(feature_name) def initialize(client, key_prefix: nil) @client = client @key_prefix = key_prefix + @sadd_returns_boolean = with_connection do |conn| + conn.class.respond_to?(:sadd_returns_boolean) && conn.class.sadd_returns_boolean + end end # Public: The set of known features. @@ -35,9 +40,9 @@ def features # Public: Adds a feature to the set of known features. def add(feature) if redis_sadd_returns_boolean? - @client.sadd? features_key, feature.key + with_connection { |conn| conn.sadd? features_key, feature.key } else - @client.sadd features_key, feature.key + with_connection { |conn| conn.sadd features_key, feature.key } end true end @@ -45,17 +50,17 @@ def add(feature) # Public: Removes a feature from the set of known features. def remove(feature) if redis_sadd_returns_boolean? - @client.srem? features_key, feature.key + with_connection { |conn| conn.srem? features_key, feature.key } else - @client.srem features_key, feature.key + with_connection { |conn| conn.srem features_key, feature.key } end - @client.del key_for(feature.key) + with_connection { |conn| conn.del key_for(feature.key) } true end # Public: Clears the gate values for a feature. def clear(feature) - @client.del key_for(feature.key) + with_connection { |conn| conn.del key_for(feature.key) } true end @@ -88,13 +93,13 @@ def enable(feature, gate, thing) case gate.data_type when :boolean clear(feature) - @client.hset feature_key, gate.key, thing.value.to_s + with_connection { |conn| conn.hset feature_key, gate.key, thing.value.to_s } when :integer - @client.hset feature_key, gate.key, thing.value.to_s + with_connection { |conn| conn.hset feature_key, gate.key, thing.value.to_s } when :set - @client.hset feature_key, to_field(gate, thing), 1 + with_connection { |conn| conn.hset feature_key, to_field(gate, thing), 1 } when :json - @client.hset feature_key, gate.key, Typecast.to_json(thing.value) + with_connection { |conn| conn.hset feature_key, gate.key, Typecast.to_json(thing.value) } else unsupported_data_type gate.data_type end @@ -113,13 +118,13 @@ def disable(feature, gate, thing) feature_key = key_for(feature.key) case gate.data_type when :boolean - @client.del feature_key + with_connection { |conn| conn.del feature_key } when :integer - @client.hset feature_key, gate.key, thing.value.to_s + with_connection { |conn| conn.hset feature_key, gate.key, thing.value.to_s } when :set - @client.hdel feature_key, to_field(gate, thing) + with_connection { |conn| conn.hdel feature_key, to_field(gate, thing) } when :json - @client.hdel feature_key, gate.key + with_connection { |conn| conn.hdel feature_key, gate.key } else unsupported_data_type gate.data_type end @@ -130,7 +135,7 @@ def disable(feature, gate, thing) private def redis_sadd_returns_boolean? - @client.class.respond_to?(:sadd_returns_boolean) && @client.class.sadd_returns_boolean + @sadd_returns_boolean end def read_many_features(features) @@ -143,20 +148,26 @@ def read_many_features(features) end def read_feature_keys - @client.smembers(features_key).to_set + with_connection { |conn| conn.smembers(features_key).to_set } end # Private: Gets a hash of fields => values for the given feature. # # Returns a Hash of fields => values. - def doc_for(feature, pipeline: @client) - pipeline.hgetall(key_for(feature.key)) + def doc_for(feature, pipeline: nil) + if pipeline + pipeline.hgetall(key_for(feature.key)) + else + with_connection { |conn| conn.hgetall(key_for(feature.key)) } + end end def docs_for(features) - @client.pipelined do |pipeline| - features.each do |feature| - doc_for(feature, pipeline: pipeline) + with_connection do |conn| + conn.pipelined do |pipeline| + features.each do |feature| + doc_for(feature, pipeline: pipeline) + end end end end diff --git a/lib/flipper/adapters/redis_cache.rb b/lib/flipper/adapters/redis_cache.rb index 001de6e3b..68e3d6ac4 100644 --- a/lib/flipper/adapters/redis_cache.rb +++ b/lib/flipper/adapters/redis_cache.rb @@ -1,20 +1,24 @@ require 'redis' require 'flipper' require 'flipper/adapters/cache_base' +require 'flipper/adapters/redis_shared/methods' module Flipper module Adapters # Public: Adapter that wraps another adapter with the ability to cache # adapter calls in Redis. class RedisCache < CacheBase + include ::Flipper::Adapters::RedisShared + def initialize(adapter, cache, ttl = 3600, prefix: nil) + @client = cache super end private def cache_fetch(key, &block) - cached = @cache.get(key) + cached = with_connection { |conn| conn.get(key) } if cached Marshal.load(cached) else @@ -27,7 +31,7 @@ def cache_fetch(key, &block) def cache_read_multi(keys) return {} if keys.empty? - values = @cache.mget(*keys).map do |value| + values = with_connection { |conn| conn.mget(*keys) }.map do |value| value ? Marshal.load(value) : nil end @@ -35,11 +39,11 @@ def cache_read_multi(keys) end def cache_write(key, value) - @cache.setex(key, @ttl, Marshal.dump(value)) + with_connection { |conn| conn.setex(key, @ttl, Marshal.dump(value)) } end def cache_delete(key) - @cache.del(key) + with_connection { |conn| conn.del(key) } end end end diff --git a/lib/flipper/adapters/redis_connection_pool.rb b/lib/flipper/adapters/redis_connection_pool.rb deleted file mode 100644 index a7abaa914..000000000 --- a/lib/flipper/adapters/redis_connection_pool.rb +++ /dev/null @@ -1,241 +0,0 @@ -require 'set' -require 'redis' -require 'flipper' -require 'connection_pool' - -module Flipper - module Adapters - class RedisConnectionPool - include ::Flipper::Adapter - - def self.default_pool - return @default_pool if defined?(@default_pool) - - size = ENV.fetch('FLIPPER_REDIS_POOL_SIZE', 5).to_i - timeout = ENV.fetch('FLIPPER_REDIS_POOL_TIMEOUT', 5).to_i - - @default_pool = ConnectionPool.new(size: size, timeout: timeout) { - ::Redis.new(url: ENV["FLIPPER_REDIS_URL"] || ENV["REDIS_URL"] || "redis://localhost:6379") - } - end - - attr_reader :key_prefix - - def features_key - "#{key_prefix}flipper_features" - end - - def key_for(feature_name) - "#{key_prefix}#{feature_name}" - end - - # Public: Initializes a Redis flipper adapter. - # - # pool - The Redis connection pool to use. - # key_prefix - an optional prefix with which to namespace - # flipper's Redis keys - def initialize(pool, key_prefix: nil) - @pool = pool - @key_prefix = key_prefix - @sadd_returns_boolean = with_connection do |conn| - conn.class.respond_to?(:sadd_returns_boolean) && conn.class.sadd_returns_boolean - end - end - - # Public: The set of known features. - def features - read_feature_keys - end - - # Public: Adds a feature to the set of known features. - def add(feature) - if redis_sadd_returns_boolean? - with_connection { |conn| conn.sadd? features_key, feature.key } - else - with_connection { |conn| conn.sadd features_key, feature.key } - end - true - end - - # Public: Removes a feature from the set of known features. - def remove(feature) - if redis_sadd_returns_boolean? - with_connection { |conn| conn.srem? features_key, feature.key } - else - with_connection { |conn| conn.srem features_key, feature.key } - end - with_connection { |conn| conn.del key_for(feature.key) } - true - end - - # Public: Clears the gate values for a feature. - def clear(feature) - with_connection { |conn| conn.del key_for(feature.key) } - true - end - - # Public: Gets the values for all gates for a given feature. - # - # Returns a Hash of Flipper::Gate#key => value. - def get(feature) - doc = with_connection { |conn| doc_for(conn, feature) } - result_for_feature(feature, doc) - end - - def get_multi(features) - read_many_features(features) - end - - def get_all - features = read_feature_keys.map { |key| Flipper::Feature.new(key, self) } - read_many_features(features) - end - - # Public: Enables a gate for a given thing. - # - # feature - The Flipper::Feature for the gate. - # gate - The Flipper::Gate to enable. - # thing - The Flipper::Type being enabled for the gate. - # - # Returns true. - def enable(feature, gate, thing) - feature_key = key_for(feature.key) - case gate.data_type - when :boolean - clear(feature) - with_connection do |conn| - conn.hset feature_key, gate.key, thing.value.to_s - end - when :integer - with_connection do |conn| - conn.hset feature_key, gate.key, thing.value.to_s - end - when :set - with_connection do |conn| - conn.hset feature_key, to_field(gate, thing), 1 - end - when :json - with_connection do |conn| - conn.hset feature_key, gate.key, Typecast.to_json(thing.value) - end - else - unsupported_data_type gate.data_type - end - - true - end - - # Public: Disables a gate for a given thing. - # - # feature - The Flipper::Feature for the gate. - # gate - The Flipper::Gate to disable. - # thing - The Flipper::Type being disabled for the gate. - # - # Returns true. - def disable(feature, gate, thing) - feature_key = key_for(feature.key) - case gate.data_type - when :boolean - with_connection { |conn| conn.del feature_key } - when :integer - with_connection { |conn| conn.hset feature_key, gate.key, thing.value.to_s } - when :set - with_connection { |conn| conn.hdel feature_key, to_field(gate, thing) } - when :json - with_connection { |conn| conn.hdel feature_key, gate.key } - else - unsupported_data_type gate.data_type - end - - true - end - - private - - def with_connection(&block) - @pool.with(&block) - end - - def redis_sadd_returns_boolean? - @sadd_returns_boolean - end - - def read_many_features(features) - docs = docs_for(features) - result = {} - features.zip(docs) do |feature, doc| - result[feature.key] = result_for_feature(feature, doc) - end - result - end - - def read_feature_keys - with_connection { |conn| conn.smembers(features_key).to_set } - end - - def doc_for(connection, feature) - connection.hgetall(key_for(feature.key)) - end - - def docs_for(features) - with_connection do |conn| - conn.pipelined do |pipeline| - features.each do |feature| - doc_for(pipeline, feature) - end - end - end - end - - def result_for_feature(feature, doc) - result = {} - fields = doc.keys - - feature.gates.each do |gate| - result[gate.key] = - case gate.data_type - when :boolean, :integer - doc[gate.key.to_s] - when :set - fields_to_gate_value fields, gate - when :json - value = doc[gate.key.to_s] - Typecast.from_json(value) - else - unsupported_data_type gate.data_type - end - end - - result - end - - # Private: Converts gate and thing to hash key. - def to_field(gate, thing) - "#{gate.key}/#{thing.value}" - end - - # Private: Returns a set of values given an array of fields and a gate. - # - # Returns a Set of the values enabled for the gate. - def fields_to_gate_value(fields, gate) - regex = %r{^#{Regexp.escape(gate.key.to_s)}/} - keys = fields.grep(regex) - values = keys.map { |key| key.split('/', 2).last } - values.to_set - end - - # Private - def unsupported_data_type(data_type) - raise "#{data_type} is not supported by this adapter" - end - end - end -end - -Flipper.configure do |config| - config.adapter do - Flipper::Adapters::RedisConnectionPool.new( - Flipper::Adapters::RedisConnectionPool.default_pool - ) - end -end diff --git a/lib/flipper/adapters/redis_shared/methods.rb b/lib/flipper/adapters/redis_shared/methods.rb new file mode 100644 index 000000000..9b13144dc --- /dev/null +++ b/lib/flipper/adapters/redis_shared/methods.rb @@ -0,0 +1,9 @@ +module Flipper + module Adapters + module RedisShared + def with_connection(&block) + @client.respond_to?(:with) ? @client.with(&block) : yield(@client) + end + end + end +end diff --git a/spec/flipper/adapters/redis_connection_pool_spec.rb b/spec/flipper/adapters/redis_connection_pool_spec.rb deleted file mode 100644 index f5917e7f7..000000000 --- a/spec/flipper/adapters/redis_connection_pool_spec.rb +++ /dev/null @@ -1,57 +0,0 @@ -require 'flipper/adapters/redis_connection_pool' - -RSpec.describe Flipper::Adapters::RedisConnectionPool do - let(:pool) do - Redis.raise_deprecations = true - ConnectionPool.new(size: 5, timeout: 5) { - Redis.new(url: ENV.fetch('REDIS_URL', 'redis://localhost:6379')) - } - end - - subject { described_class.new(pool) } - - before do - skip_on_error(Redis::CannotConnectError, 'Redis not available') do - pool.with { |conn| conn.flushdb } - end - end - - it_should_behave_like 'a flipper adapter' - - it 'configures itself on load' do - Flipper.configuration = nil - Flipper.instance = nil - - silence { load 'flipper/adapters/redis_connection_pool.rb' } - - expect(Flipper.adapter.adapter).to be_a(Flipper::Adapters::RedisConnectionPool) - end - - describe 'with a key_prefix' do - let(:subject) { described_class.new(pool, key_prefix: "lockbox:") } - let(:feature) { Flipper::Feature.new(:search, subject) } - - it_should_behave_like 'a flipper adapter' - - it 'namespaces feature-keys' do - subject.add(feature) - - pool.with do |conn| - expect(conn.smembers("flipper_features")).to eq([]) - expect(conn.exists?("search")).to eq(false) - expect(conn.smembers("lockbox:flipper_features")).to eq(["search"]) - expect(conn.hgetall("lockbox:search")).not_to eq(nil) - end - end - - it "can remove namespaced keys" do - subject.add(feature) - - pool.with do |conn| - expect(conn.smembers("lockbox:flipper_features")).to eq(["search"]) - subject.remove(feature) - expect(conn.smembers("lockbox:flipper_features")).to be_empty - end - end - end -end diff --git a/spec/flipper/adapters/redis_spec.rb b/spec/flipper/adapters/redis_spec.rb index 153d41e24..e1852f324 100644 --- a/spec/flipper/adapters/redis_spec.rb +++ b/spec/flipper/adapters/redis_spec.rb @@ -1,51 +1,101 @@ require 'flipper/adapters/redis' +require 'connection_pool' RSpec.describe Flipper::Adapters::Redis do - let(:client) do - Redis.raise_deprecations = true - Redis.new(url: ENV.fetch('REDIS_URL', 'redis://localhost:6379')) - end + context "redis instance" do + let(:client) do + Redis.raise_deprecations = true + Redis.new(url: ENV.fetch('REDIS_URL', 'redis://localhost:6379')) + end - subject { described_class.new(client) } + subject { described_class.new(client) } - before do - skip_on_error(Redis::CannotConnectError, 'Redis not available') do - client.flushdb + before do + skip_on_error(Redis::CannotConnectError, 'Redis not available') do + client.flushdb + end end - end - it_should_behave_like 'a flipper adapter' + it_should_behave_like 'a flipper adapter' + + it 'configures itself on load' do + Flipper.configuration = nil + Flipper.instance = nil + + silence { load 'flipper/adapters/redis.rb' } + + expect(Flipper.adapter.adapter).to be_a(Flipper::Adapters::Redis) + end + + describe 'with a key_prefix' do + let(:subject) { described_class.new(client, key_prefix: "lockbox:") } + let(:feature) { Flipper::Feature.new(:search, subject) } + + it_should_behave_like 'a flipper adapter' - it 'configures itself on load' do - Flipper.configuration = nil - Flipper.instance = nil + it 'namespaces feature-keys' do + subject.add(feature) - silence { load 'flipper/adapters/redis.rb' } + expect(client.smembers("flipper_features")).to eq([]) + expect(client.exists?("search")).to eq(false) + expect(client.smembers("lockbox:flipper_features")).to eq(["search"]) + expect(client.hgetall("lockbox:search")).not_to eq(nil) + end - expect(Flipper.adapter.adapter).to be_a(Flipper::Adapters::Redis) + it "can remove namespaced keys" do + subject.add(feature) + expect(client.smembers("lockbox:flipper_features")).to eq(["search"]) + + subject.remove(feature) + expect(client.smembers("lockbox:flipper_features")).to be_empty + end + end end - describe 'with a key_prefix' do - let(:subject) { described_class.new(client, key_prefix: "lockbox:") } - let(:feature) { Flipper::Feature.new(:search, subject) } + context "with a connection pool instance" do + let(:client) do + Redis.raise_deprecations = true + ConnectionPool.new(size: 1, timeout: 1) { + Redis.new(url: ENV.fetch('REDIS_URL', 'redis://localhost:6379')) + } + end + + subject { described_class.new(client) } + + before do + skip_on_error(Redis::CannotConnectError, 'Redis not available') do + client.with { |conn| conn.flushdb } + end + end it_should_behave_like 'a flipper adapter' - it 'namespaces feature-keys' do - subject.add(feature) + describe 'with a key_prefix' do + let(:subject) { described_class.new(client, key_prefix: "lockbox:") } + let(:feature) { Flipper::Feature.new(:search, subject) } - expect(client.smembers("flipper_features")).to eq([]) - expect(client.exists?("search")).to eq(false) - expect(client.smembers("lockbox:flipper_features")).to eq(["search"]) - expect(client.hgetall("lockbox:search")).not_to eq(nil) - end + it_should_behave_like 'a flipper adapter' + + it 'namespaces feature-keys' do + subject.add(feature) + + client.with do |conn| + expect(conn.smembers("flipper_features")).to eq([]) + expect(conn.exists?("search")).to eq(false) + expect(conn.smembers("lockbox:flipper_features")).to eq(["search"]) + expect(conn.hgetall("lockbox:search")).not_to eq(nil) + end + end - it "can remove namespaced keys" do - subject.add(feature) - expect(client.smembers("lockbox:flipper_features")).to eq(["search"]) + it "can remove namespaced keys" do + client.with do |conn| + subject.add(feature) + expect(conn.smembers("lockbox:flipper_features")).to eq(["search"]) - subject.remove(feature) - expect(client.smembers("lockbox:flipper_features")).to be_empty + subject.remove(feature) + expect(conn.smembers("lockbox:flipper_features")).to be_empty + end + end end end end diff --git a/test/adapters/redis_connection_pool_test.rb b/test/adapters/redis_connection_pool_test.rb deleted file mode 100644 index f497a7c9c..000000000 --- a/test/adapters/redis_connection_pool_test.rb +++ /dev/null @@ -1,17 +0,0 @@ -require 'test_helper' -require 'flipper/adapters/redis' - -class RedisConnectionPoolTest < MiniTest::Test - prepend Flipper::Test::SharedAdapterTests - - def setup - url = ENV.fetch('REDIS_URL', 'redis://localhost:6379') - pool = ConnectionPool.new(size: 5, timeout: 5) { - Redis.new(url: url) - } - @adapter = Flipper::Adapters::RedisConnectionPool.new(pool) - pool.with { |client| client.flushdb } - rescue Redis::CannotConnectError - ENV['CI'] ? raise : skip('Redis not available') - end -end From 5ba402df9fda26e613a1d122de1cececbd159bcf Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 09:31:12 -0500 Subject: [PATCH 23/40] More lenient stub --- spec/flipper/cloud/telemetry_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/flipper/cloud/telemetry_spec.rb b/spec/flipper/cloud/telemetry_spec.rb index 8c60d5dac..d170ade86 100644 --- a/spec/flipper/cloud/telemetry_spec.rb +++ b/spec/flipper/cloud/telemetry_spec.rb @@ -25,7 +25,7 @@ expect(telemetry.interval).to eq(60) expect(telemetry.timer.execution_interval).to eq(60) - expect(stub).to have_been_requested + expect(stub).to have_been_requested.at_least_once end it "phones home and updates telemetry interval if present" do From 4774aaef219e6927293db498db408a6dbf38e5d3 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 10:35:41 -0500 Subject: [PATCH 24/40] Escape where necessary Before it was escaping everything passed to redirect and thus only using escape_path. But I need it all escaped so I can start making ../../ work. --- lib/flipper/ui/action.rb | 4 +-- lib/flipper/ui/actions/actors_gate.rb | 4 +-- lib/flipper/ui/actions/boolean_gate.rb | 2 +- lib/flipper/ui/actions/features.rb | 4 +-- lib/flipper/ui/actions/groups_gate.rb | 4 +-- .../ui/actions/percentage_of_actors_gate.rb | 4 +-- .../ui/actions/percentage_of_time_gate.rb | 4 +-- lib/flipper/ui/util.rb | 10 +++++++ lib/flipper/ui/views/add_actor.erb | 4 +-- lib/flipper/ui/views/add_group.erb | 4 +-- lib/flipper/ui/views/feature.erb | 20 ++++++------- lib/flipper/ui/views/features.erb | 2 +- spec/flipper/ui/actions/actors_gate_spec.rb | 10 +++---- spec/flipper/ui/actions/boolean_gate_spec.rb | 2 +- spec/flipper/ui/actions/features_spec.rb | 29 +++++++++++++++++-- spec/flipper/ui/actions/groups_gate_spec.rb | 10 +++---- .../actions/percentage_of_actors_gate_spec.rb | 6 ++-- .../actions/percentage_of_time_gate_spec.rb | 6 ++-- 18 files changed, 81 insertions(+), 48 deletions(-) diff --git a/lib/flipper/ui/action.rb b/lib/flipper/ui/action.rb index 0ff4053e3..dd49758ac 100644 --- a/lib/flipper/ui/action.rb +++ b/lib/flipper/ui/action.rb @@ -12,7 +12,7 @@ module FeatureNameFromRoute def feature_name @feature_name ||= begin match = request.path_info.match(self.class.route_regex) - match ? Rack::Utils.unescape(match[:feature_name]) : nil + match ? Flipper::UI::Util.unescape(match[:feature_name]) : nil end end private :feature_name @@ -164,7 +164,7 @@ def json_response(object) # location - The String location to set the Location header to. def redirect_to(location) status 302 - header 'location', "#{script_name}#{Rack::Utils.escape_path(location)}" + header 'location', "#{script_name}#{location}" halt [@code, @headers, ['']] end diff --git a/lib/flipper/ui/actions/actors_gate.rb b/lib/flipper/ui/actions/actors_gate.rb index e76a39f87..044622d18 100644 --- a/lib/flipper/ui/actions/actors_gate.rb +++ b/lib/flipper/ui/actions/actors_gate.rb @@ -25,7 +25,7 @@ def post if values.empty? error = "#{value.inspect} is not a valid actor value." - redirect_to("/features/#{feature.key}/actors?error=#{error}") + redirect_to("/features/#{Flipper::UI::Util.escape feature.key}/actors?error=#{Flipper::UI::Util.escape error}") end values.each do |value| @@ -39,7 +39,7 @@ def post end end - redirect_to("/features/#{feature.key}") + redirect_to("/features/#{Flipper::UI::Util.escape feature.key}") end end end diff --git a/lib/flipper/ui/actions/boolean_gate.rb b/lib/flipper/ui/actions/boolean_gate.rb index 76c16adae..7fc9521b1 100644 --- a/lib/flipper/ui/actions/boolean_gate.rb +++ b/lib/flipper/ui/actions/boolean_gate.rb @@ -21,7 +21,7 @@ def post feature.disable end - redirect_to "/features/#{@feature.key}" + redirect_to "/features/#{Flipper::UI::Util.escape @feature.key}" end end end diff --git a/lib/flipper/ui/actions/features.rb b/lib/flipper/ui/actions/features.rb index 96a4b9c6c..1dc6448f4 100644 --- a/lib/flipper/ui/actions/features.rb +++ b/lib/flipper/ui/actions/features.rb @@ -45,13 +45,13 @@ def post if Util.blank?(value) error = "#{value.inspect} is not a valid feature name." - redirect_to("/features/new?error=#{error}") + redirect_to("/features/new?error=#{Flipper::UI::Util.escape error}") end feature = flipper[value] feature.add - redirect_to "/features/#{value}" + redirect_to "/features/#{Flipper::UI::Util.escape value}" end end end diff --git a/lib/flipper/ui/actions/groups_gate.rb b/lib/flipper/ui/actions/groups_gate.rb index 173ccc245..ef5cf37a2 100644 --- a/lib/flipper/ui/actions/groups_gate.rb +++ b/lib/flipper/ui/actions/groups_gate.rb @@ -30,10 +30,10 @@ def post feature.disable_group value end - redirect_to("/features/#{feature.key}") + redirect_to("/features/#{Flipper::UI::Util.escape feature.key}") else error = "The group named #{value.inspect} has not been registered." - redirect_to("/features/#{feature.key}/groups?error=#{error}") + redirect_to("/features/#{Flipper::UI::Util.escape feature.key}/groups?error=#{Flipper::UI::Util.escape error}") end end end diff --git a/lib/flipper/ui/actions/percentage_of_actors_gate.rb b/lib/flipper/ui/actions/percentage_of_actors_gate.rb index 7adbc129b..a166aea38 100644 --- a/lib/flipper/ui/actions/percentage_of_actors_gate.rb +++ b/lib/flipper/ui/actions/percentage_of_actors_gate.rb @@ -19,10 +19,10 @@ def post feature.enable_percentage_of_actors params['value'] rescue ArgumentError => exception error = "Invalid percentage of actors value: #{exception.message}" - redirect_to("/features/#{@feature.key}?error=#{error}") + redirect_to("/features/#{Flipper::UI::Util.escape @feature.key}?error=#{Flipper::UI::Util.escape error}") end - redirect_to "/features/#{@feature.key}" + redirect_to "/features/#{Flipper::UI::Util.escape @feature.key}" end end end diff --git a/lib/flipper/ui/actions/percentage_of_time_gate.rb b/lib/flipper/ui/actions/percentage_of_time_gate.rb index 9889b7351..4683cadba 100644 --- a/lib/flipper/ui/actions/percentage_of_time_gate.rb +++ b/lib/flipper/ui/actions/percentage_of_time_gate.rb @@ -19,10 +19,10 @@ def post feature.enable_percentage_of_time params['value'] rescue ArgumentError => exception error = "Invalid percentage of time value: #{exception.message}" - redirect_to("/features/#{@feature.key}?error=#{error}") + redirect_to("/features/#{Flipper::UI::Util.escape @feature.key}?error=#{Flipper::UI::Util.escape error}") end - redirect_to "/features/#{@feature.key}" + redirect_to "/features/#{Flipper::UI::Util.escape @feature.key}" end end end diff --git a/lib/flipper/ui/util.rb b/lib/flipper/ui/util.rb index be0b0b1c3..e1bcf54ed 100644 --- a/lib/flipper/ui/util.rb +++ b/lib/flipper/ui/util.rb @@ -1,9 +1,19 @@ +require "rack/utils" + module Flipper module UI module Util # Private: 0x3000: fullwidth whitespace NON_WHITESPACE_REGEXP = /[^\s#{[0x3000].pack("U")}]/ + def self.escape(str) + Rack::Utils.escape(str) + end + + def self.unescape(str) + Rack::Utils.unescape(str) + end + def self.blank?(str) str.to_s !~ NON_WHITESPACE_REGEXP end diff --git a/lib/flipper/ui/views/add_actor.erb b/lib/flipper/ui/views/add_actor.erb index e401d0b7e..dfdb6cbce 100644 --- a/lib/flipper/ui/views/add_actor.erb +++ b/lib/flipper/ui/views/add_actor.erb @@ -6,7 +6,7 @@

- <%= @feature.key %> + <%= @feature.key %> / Enable Actor

@@ -14,7 +14,7 @@

Turn on this feature for actors.

-
+ <%== csrf_input_tag %>
diff --git a/lib/flipper/ui/views/add_group.erb b/lib/flipper/ui/views/add_group.erb index feb93bf88..752c99fbc 100644 --- a/lib/flipper/ui/views/add_group.erb +++ b/lib/flipper/ui/views/add_group.erb @@ -6,7 +6,7 @@

- <%= @feature.key %> + <%= @feature.key %> / Enable Group

@@ -16,7 +16,7 @@

Turn on this feature for an entire group of actors.

- + <%== csrf_input_tag %>
diff --git a/lib/flipper/ui/views/feature.erb b/lib/flipper/ui/views/feature.erb index cd106c97e..3c1245bf2 100644 --- a/lib/flipper/ui/views/feature.erb +++ b/lib/flipper/ui/views/feature.erb @@ -50,7 +50,7 @@
- + <%== csrf_input_tag %>
@@ -83,7 +83,7 @@
<% if write_allowed? %> - + <%== csrf_input_tag %> @@ -119,7 +119,7 @@ <% if @feature.disabled_groups.empty? %> All groups enabled. <% else %> - + <%== csrf_input_tag %>
@@ -150,7 +150,7 @@
<% if write_allowed? %> - + <%== csrf_input_tag %> @@ -185,7 +185,7 @@ <% if write_allowed? %>
- + <%== csrf_input_tag %>
<% @percentages.each do |number| %> @@ -193,7 +193,7 @@ <% end %>
-
+ <%== csrf_input_tag %>
0 %>value="<%= @feature.percentage_of_actors_value %>"<% end %> title="Custom percentage (26, 32, etc.)" placeholder="0" class="form-control"> @@ -226,7 +226,7 @@ <% if write_allowed? %>
- + <%== csrf_input_tag %>
<% @percentages.each do |number| %> @@ -234,7 +234,7 @@ <% end %>
-
+ <%== csrf_input_tag %>
0 %>value="<%= @feature.percentage_of_time_value %>"<% end %> title="Custom percentage (26, 32, etc.)" placeholder="0" class="form-control"> @@ -249,7 +249,7 @@ <% if write_allowed? %>
- + <%== csrf_input_tag %>
@@ -301,7 +301,7 @@

<%= Flipper::UI.configuration.delete.description %>

- + <%== csrf_input_tag %> diff --git a/lib/flipper/ui/views/features.erb b/lib/flipper/ui/views/features.erb index dba1d1490..0528b8bd0 100644 --- a/lib/flipper/ui/views/features.erb +++ b/lib/flipper/ui/views/features.erb @@ -36,7 +36,7 @@
<% @features.each do |feature| %> - " class="list-group-item list-group-item-action"> + " class="list-group-item list-group-item-action">
> diff --git a/spec/flipper/ui/actions/actors_gate_spec.rb b/spec/flipper/ui/actions/actors_gate_spec.rb index afe828bb4..a7084d651 100644 --- a/spec/flipper/ui/actions/actors_gate_spec.rb +++ b/spec/flipper/ui/actions/actors_gate_spec.rb @@ -35,7 +35,7 @@ end it 'renders add new actor form' do - form = '' + form = '' expect(last_response.body).to include(form) end end @@ -73,7 +73,7 @@ context "when feature name contains space" do before do - post 'features/sp%20ace/actors', + post 'features/sp+ace/actors', { 'value' => value, 'operation' => 'enable', 'authenticity_token' => token }, 'rack.session' => session end @@ -84,7 +84,7 @@ it "redirects back to feature" do expect(last_response.status).to be(302) - expect(last_response.headers['location']).to eq('/features/sp%20ace') + expect(last_response.headers['location']).to eq('/features/sp+ace') end end @@ -114,7 +114,7 @@ it 'redirects back to feature' do expect(last_response.status).to be(302) - expect(last_response.headers['location']).to eq('/features/search/actors?error=%22%22%20is%20not%20a%20valid%20actor%20value.') + expect(last_response.headers['location']).to eq('/features/search/actors?error=%22%22+is+not+a+valid+actor+value.') end end @@ -123,7 +123,7 @@ it 'redirects back to feature' do expect(last_response.status).to be(302) - expect(last_response.headers['location']).to eq('/features/search/actors?error=%22%22%20is%20not%20a%20valid%20actor%20value.') + expect(last_response.headers['location']).to eq('/features/search/actors?error=%22%22+is+not+a+valid+actor+value.') end end end diff --git a/spec/flipper/ui/actions/boolean_gate_spec.rb b/spec/flipper/ui/actions/boolean_gate_spec.rb index a7f959ac5..5fa510641 100644 --- a/spec/flipper/ui/actions/boolean_gate_spec.rb +++ b/spec/flipper/ui/actions/boolean_gate_spec.rb @@ -43,7 +43,7 @@ it 'redirects back to feature' do expect(last_response.status).to be(302) - expect(last_response.headers['location']).to eq('/features/sp%20ace') + expect(last_response.headers['location']).to eq('/features/sp+ace') end end diff --git a/spec/flipper/ui/actions/features_spec.rb b/spec/flipper/ui/actions/features_spec.rb index 5516227b1..19899cdf4 100644 --- a/spec/flipper/ui/actions/features_spec.rb +++ b/spec/flipper/ui/actions/features_spec.rb @@ -28,6 +28,16 @@ end end + it "escapes keys that are junky" do + flipper.add("../../../../blah") + flipper.add("this that") + flipper.add("foo/bar") + get '/features' + expect(last_response.body).to include("..%2F..%2F..%2F..%2Fblah") + expect(last_response.body).to include("this+that") + expect(last_response.body).to include("foo%2Fbar") + end + context "when there are no features to list" do before do @original_fun_enabled = Flipper::UI.configuration.fun @@ -124,7 +134,20 @@ it 'redirects to feature' do expect(last_response.status).to be(302) - expect(last_response.headers['location']).to eq('/features/notifications%20next') + expect(last_response.headers['location']).to eq('/features/notifications+next') + end + end + + context 'feature name contains ../' do + let(:feature_name) { '../../../foo' } + + it 'adds feature with space' do + expect(flipper.features.map(&:key)).to include(feature_name) + end + + it 'redirects to feature' do + expect(last_response.status).to be(302) + expect(last_response.headers['location']).to eq('/features/..%2F..%2F..%2Ffoo') end end @@ -138,7 +161,7 @@ it 'redirects back to feature' do expect(last_response.status).to be(302) - expect(last_response.headers['location']).to eq('/features/new?error=%22%22%20is%20not%20a%20valid%20feature%20name.') + expect(last_response.headers['location']).to eq('/features/new?error=%22%22+is+not+a+valid+feature+name.') end end @@ -151,7 +174,7 @@ it 'redirects back to feature' do expect(last_response.status).to be(302) - expect(last_response.headers['location']).to eq('/features/new?error=%22%22%20is%20not%20a%20valid%20feature%20name.') + expect(last_response.headers['location']).to eq('/features/new?error=%22%22+is+not+a+valid+feature+name.') end end end diff --git a/spec/flipper/ui/actions/groups_gate_spec.rb b/spec/flipper/ui/actions/groups_gate_spec.rb index c2be08cc4..283beb83d 100644 --- a/spec/flipper/ui/actions/groups_gate_spec.rb +++ b/spec/flipper/ui/actions/groups_gate_spec.rb @@ -60,7 +60,7 @@ context 'feature name contains space' do before do - post 'features/sp%20ace/groups', + post 'features/sp+ace/groups', { 'value' => group_name, 'operation' => 'enable', 'authenticity_token' => token }, 'rack.session' => session end @@ -71,7 +71,7 @@ it 'redirects back to feature' do expect(last_response.status).to be(302) - expect(last_response.headers['location']).to eq('/features/sp%20ace') + expect(last_response.headers['location']).to eq('/features/sp+ace') end end @@ -89,7 +89,7 @@ it 'redirects back to feature' do expect(last_response.status).to be(302) - expect(last_response.headers['location']).to eq('/features/search/groups?error=The%20group%20named%20%22not_here%22%20has%20not%20been%20registered.') + expect(last_response.headers['location']).to eq('/features/search/groups?error=The+group+named+%22not_here%22+has+not+been+registered.') end end @@ -98,7 +98,7 @@ it 'redirects back to feature' do expect(last_response.status).to be(302) - expect(last_response.headers['location']).to eq('/features/search/groups?error=The%20group%20named%20%22%22%20has%20not%20been%20registered.') + expect(last_response.headers['location']).to eq('/features/search/groups?error=The+group+named+%22%22+has+not+been+registered.') end end @@ -107,7 +107,7 @@ it 'redirects back to feature' do expect(last_response.status).to be(302) - expect(last_response.headers['location']).to eq('/features/search/groups?error=The%20group%20named%20%22%22%20has%20not%20been%20registered.') + expect(last_response.headers['location']).to eq('/features/search/groups?error=The+group+named+%22%22+has+not+been+registered.') end end end diff --git a/spec/flipper/ui/actions/percentage_of_actors_gate_spec.rb b/spec/flipper/ui/actions/percentage_of_actors_gate_spec.rb index 8bd190bf0..510ab7dd8 100644 --- a/spec/flipper/ui/actions/percentage_of_actors_gate_spec.rb +++ b/spec/flipper/ui/actions/percentage_of_actors_gate_spec.rb @@ -30,7 +30,7 @@ context 'with space in feature name' do before do - post 'features/sp%20ace/percentage_of_actors', + post 'features/sp+ace/percentage_of_actors', { 'value' => '24', 'authenticity_token' => token }, 'rack.session' => session end @@ -41,7 +41,7 @@ it 'redirects back to feature' do expect(last_response.status).to be(302) - expect(last_response.headers['location']).to eq('/features/sp%20ace') + expect(last_response.headers['location']).to eq('/features/sp+ace') end end @@ -58,7 +58,7 @@ it 'redirects back to feature' do expect(last_response.status).to be(302) - expect(last_response.headers['location']).to eq('/features/search?error=Invalid%20percentage%20of%20actors%20value:%20value%20must%20be%20a%20positive%20number%20less%20than%20or%20equal%20to%20100,%20but%20was%20555') + expect(last_response.headers['location']).to eq('/features/search?error=Invalid+percentage+of+actors+value%3A+value+must+be+a+positive+number+less+than+or+equal+to+100%2C+but+was+555') end end end diff --git a/spec/flipper/ui/actions/percentage_of_time_gate_spec.rb b/spec/flipper/ui/actions/percentage_of_time_gate_spec.rb index 01f67ddce..08f70d846 100644 --- a/spec/flipper/ui/actions/percentage_of_time_gate_spec.rb +++ b/spec/flipper/ui/actions/percentage_of_time_gate_spec.rb @@ -30,7 +30,7 @@ context 'with space in feature name' do before do - post 'features/sp%20ace/percentage_of_time', + post 'features/sp+ace/percentage_of_time', { 'value' => '24', 'authenticity_token' => token }, 'rack.session' => session end @@ -41,7 +41,7 @@ it 'redirects back to feature' do expect(last_response.status).to be(302) - expect(last_response.headers['location']).to eq('/features/sp%20ace') + expect(last_response.headers['location']).to eq('/features/sp+ace') end end @@ -58,7 +58,7 @@ it 'redirects back to feature' do expect(last_response.status).to be(302) - expect(last_response.headers['location']).to eq('/features/search?error=Invalid%20percentage%20of%20time%20value:%20value%20must%20be%20a%20positive%20number%20less%20than%20or%20equal%20to%20100,%20but%20was%20555') + expect(last_response.headers['location']).to eq('/features/search?error=Invalid+percentage+of+time+value%3A+value+must+be+a+positive+number+less+than+or+equal+to+100%2C+but+was+555') end end end From f792375c12c6cc7e69cbaf37bdc5263449e7d68e Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 10:54:52 -0500 Subject: [PATCH 25/40] Get ../../blah keys to render I only ever wanted to use rack protection authenticity token and thought that is what I was doing. I now realize they include several by default unless you explicitly turn them off. Instead i'm just going to include what i want which is auth token stuff. --- lib/flipper/ui.rb | 9 +++++++-- spec/flipper/ui/actions/feature_spec.rb | 14 ++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/lib/flipper/ui.rb b/lib/flipper/ui.rb index 007c9fad8..782d66ece 100644 --- a/lib/flipper/ui.rb +++ b/lib/flipper/ui.rb @@ -20,12 +20,17 @@ def self.root def self.app(flipper = nil, options = {}) env_key = options.fetch(:env_key, 'flipper') - rack_protection_options = options.fetch(:rack_protection, use: :authenticity_token) + + if options.key?(:rack_protection) + warn "[DEPRECATION] `rack_protection` option is deprecated. " + + "Flipper::UI now only includes Rack::Protection::AuthenticityToken middleware. " + + "If you need additional protection, you can add it yourself." + end app = ->(_) { [200, { Rack::CONTENT_TYPE => 'text/html' }, ['']] } builder = Rack::Builder.new yield builder if block_given? - builder.use Rack::Protection, rack_protection_options + builder.use Rack::Protection::AuthenticityToken builder.use Rack::MethodOverride builder.use Flipper::Middleware::SetupEnv, flipper, env_key: env_key builder.use Flipper::UI::Middleware, flipper: flipper, env_key: env_key diff --git a/spec/flipper/ui/actions/feature_spec.rb b/spec/flipper/ui/actions/feature_spec.rb index c605d9935..3cc38094c 100644 --- a/spec/flipper/ui/actions/feature_spec.rb +++ b/spec/flipper/ui/actions/feature_spec.rb @@ -197,4 +197,18 @@ expect(last_response.body).to include('a/b') end end + + describe 'GET /features/:feature with dot dot slash repeated in feature name' do + before do + get '/features/..%2F..%2F..%2F..%2Fblah' + end + + it 'responds with success' do + expect(last_response.status).to be(200) + end + + it 'renders template' do + expect(last_response.body).to include('../../../../blah') + end + end end From 2e9299fae759c2b16fd3ac148a043047e2cd18d0 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 13:14:24 -0500 Subject: [PATCH 26/40] Fix all the warnings and turn them on for CI --- .github/workflows/ci.yml | 2 +- Gemfile | 1 + Guardfile | 2 +- lib/flipper/adapters/http/error.rb | 2 +- lib/flipper/adapters/poll.rb | 2 +- lib/flipper/api/v1/actions/expression_gate.rb | 2 +- lib/flipper/cloud/configuration.rb | 1 - lib/flipper/export.rb | 2 -- lib/flipper/expressions/all.rb | 2 -- lib/flipper/instrumentation/log_subscriber.rb | 1 - lib/flipper/instrumentation/statsd.rb | 5 +++-- lib/flipper/instrumentation/subscriber.rb | 4 ---- lib/flipper/poller.rb | 4 ++-- lib/flipper/ui/configuration.rb | 4 ++-- spec/flipper/adapters/active_record_spec.rb | 5 +++-- spec/flipper/adapters/mongo_spec.rb | 2 +- spec/flipper/api/action_spec.rb | 4 ++-- spec/flipper/cloud/dsl_spec.rb | 2 +- spec/flipper/dsl_spec.rb | 3 --- spec/flipper/feature_spec.rb | 13 ++----------- spec/flipper/instrumentation/log_subscriber_spec.rb | 1 + .../instrumentation/statsd_subscriber_spec.rb | 2 +- spec/flipper/middleware/memoizer_spec.rb | 9 ++++----- spec/flipper_spec.rb | 2 +- spec/spec_helper.rb | 5 +++++ 25 files changed, 34 insertions(+), 48 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5415968e3..5b78b2f8d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -104,4 +104,4 @@ jobs: ruby-version: ${{ matrix.ruby }} bundler-cache: true # 'bundle install' and cache gems - name: Run Rake with Rails ${{ matrix.rails }} - run: bundle exec rake + run: RUBYOPT="-w" bundle exec rake diff --git a/Gemfile b/Gemfile index b4551fa64..0bc6660ee 100644 --- a/Gemfile +++ b/Gemfile @@ -31,6 +31,7 @@ gem 'mysql2' gem 'pg' gem 'cuprite' gem 'puma' +gem 'warning' group(:guard) do gem 'guard' diff --git a/Guardfile b/Guardfile index 760ab141d..ef0874a94 100644 --- a/Guardfile +++ b/Guardfile @@ -10,7 +10,7 @@ rspec_options = { all_after_pass: false, all_on_start: false, failed_mode: :keep, - cmd: 'bundle exec rspec', + cmd: 'bundle exec ruby -w $(which rspec)', } guard 'rspec', rspec_options do diff --git a/lib/flipper/adapters/http/error.rb b/lib/flipper/adapters/http/error.rb index 7b4758635..343152309 100644 --- a/lib/flipper/adapters/http/error.rb +++ b/lib/flipper/adapters/http/error.rb @@ -20,7 +20,7 @@ def initialize(response) if more_info = data["more_info"] message << "\n#{data["more_info"]}" end - rescue => exception + rescue # welp we tried end diff --git a/lib/flipper/adapters/poll.rb b/lib/flipper/adapters/poll.rb index 5c9269f50..20cd9e920 100644 --- a/lib/flipper/adapters/poll.rb +++ b/lib/flipper/adapters/poll.rb @@ -25,7 +25,7 @@ def initialize(poller, adapter) if adapter.features.empty? begin @poller.sync - rescue => error + rescue # TODO: Warn here that it's possible that no data has been synced # and flags are being evaluated without flag data being present # until a sync completes. We rescue to avoid flipper being down diff --git a/lib/flipper/api/v1/actions/expression_gate.rb b/lib/flipper/api/v1/actions/expression_gate.rb index 756714578..aee0ca6a5 100644 --- a/lib/flipper/api/v1/actions/expression_gate.rb +++ b/lib/flipper/api/v1/actions/expression_gate.rb @@ -18,7 +18,7 @@ def post feature.enable_expression expression decorated_feature = Decorators::Feature.new(feature) json_response(decorated_feature.as_json, 200) - rescue NameError, ArgumentError => exception + rescue NameError, ArgumentError json_error_response(:expression_invalid) end end diff --git a/lib/flipper/cloud/configuration.rb b/lib/flipper/cloud/configuration.rb index 930ba7742..9924f49df 100644 --- a/lib/flipper/cloud/configuration.rb +++ b/lib/flipper/cloud/configuration.rb @@ -3,7 +3,6 @@ require "flipper/adapters/http" require "flipper/adapters/poll" require "flipper/poller" -require "flipper/adapters/memory" require "flipper/adapters/dual_write" require "flipper/adapters/sync/synchronizer" require "flipper/cloud/telemetry" diff --git a/lib/flipper/export.rb b/lib/flipper/export.rb index 18c4f42f1..c8dba0a4d 100644 --- a/lib/flipper/export.rb +++ b/lib/flipper/export.rb @@ -1,5 +1,3 @@ -require "flipper/adapters/memory" - module Flipper class Export attr_reader :contents, :format, :version diff --git a/lib/flipper/expressions/all.rb b/lib/flipper/expressions/all.rb index aa11f5202..f6578d981 100644 --- a/lib/flipper/expressions/all.rb +++ b/lib/flipper/expressions/all.rb @@ -1,5 +1,3 @@ -require "flipper/expression" - module Flipper module Expressions class All diff --git a/lib/flipper/instrumentation/log_subscriber.rb b/lib/flipper/instrumentation/log_subscriber.rb index 71c140712..adbae427c 100644 --- a/lib/flipper/instrumentation/log_subscriber.rb +++ b/lib/flipper/instrumentation/log_subscriber.rb @@ -53,7 +53,6 @@ def adapter_operation(event) feature_name = event.payload[:feature_name] adapter_name = event.payload[:adapter_name] - gate_name = event.payload[:gate_name] operation = event.payload[:operation] result = event.payload[:result] diff --git a/lib/flipper/instrumentation/statsd.rb b/lib/flipper/instrumentation/statsd.rb index d88cc8137..1e27de9fa 100644 --- a/lib/flipper/instrumentation/statsd.rb +++ b/lib/flipper/instrumentation/statsd.rb @@ -2,5 +2,6 @@ require 'active_support/notifications' require 'flipper/instrumentation/statsd_subscriber' -ActiveSupport::Notifications.subscribe /\.flipper$/, - Flipper::Instrumentation::StatsdSubscriber +ActiveSupport::Notifications.subscribe( + /\.flipper$/, + Flipper::Instrumentation::StatsdSubscriber) diff --git a/lib/flipper/instrumentation/subscriber.rb b/lib/flipper/instrumentation/subscriber.rb index 0fee54db6..8e32da05f 100644 --- a/lib/flipper/instrumentation/subscriber.rb +++ b/lib/flipper/instrumentation/subscriber.rb @@ -42,7 +42,6 @@ def update # Private def update_feature_operation_metrics feature_name = @payload[:feature_name] - gate_name = @payload[:gate_name] operation = strip_trailing_question_mark(@payload[:operation]) result = @payload[:result] @@ -64,9 +63,6 @@ def update_feature_operation_metrics def update_adapter_operation_metrics adapter_name = @payload[:adapter_name] operation = @payload[:operation] - result = @payload[:result] - value = @payload[:value] - key = @payload[:key] update_timer "flipper.adapter.#{adapter_name}.#{operation}" end diff --git a/lib/flipper/poller.rb b/lib/flipper/poller.rb index 30a74cdaf..3129c098b 100644 --- a/lib/flipper/poller.rb +++ b/lib/flipper/poller.rb @@ -59,10 +59,10 @@ def stop def run loop do sleep jitter - start = Concurrent.monotonic_time + Concurrent.monotonic_time begin sync - rescue => exception + rescue # you can instrument these using poller.flipper end diff --git a/lib/flipper/ui/configuration.rb b/lib/flipper/ui/configuration.rb index 4582ea595..45ad29b7c 100644 --- a/lib/flipper/ui/configuration.rb +++ b/lib/flipper/ui/configuration.rb @@ -5,8 +5,8 @@ module UI class Configuration attr_reader :delete - attr_accessor :banner_text, - :banner_class + attr_accessor :banner_text + attr_reader :banner_class # Public: Is the UI in read only mode or not. Default is false. This # supersedes all other write-related options such as diff --git a/spec/flipper/adapters/active_record_spec.rb b/spec/flipper/adapters/active_record_spec.rb index 905739451..ca2a8779a 100644 --- a/spec/flipper/adapters/active_record_spec.rb +++ b/spec/flipper/adapters/active_record_spec.rb @@ -9,9 +9,10 @@ before(:all) do # Eval migration template so we can run migration against each database - migration = ERB.new(File.read(File.join(File.dirname(__FILE__), '../../../lib/generators/flipper/templates/migration.erb'))) + template_path = File.join(File.dirname(__FILE__), '../../../lib/generators/flipper/templates/migration.erb') + migration = ERB.new(File.read(template_path)) migration_version = "[#{ActiveRecord::VERSION::MAJOR}.#{ActiveRecord::VERSION::MINOR}]" - eval migration.result(binding) # defines CreateFlipperTables + eval migration.result_with_hash(migration_version: migration_version) # defines CreateFlipperTables end [ diff --git a/spec/flipper/adapters/mongo_spec.rb b/spec/flipper/adapters/mongo_spec.rb index e592bdab3..c6b3bd6b0 100644 --- a/spec/flipper/adapters/mongo_spec.rb +++ b/spec/flipper/adapters/mongo_spec.rb @@ -30,7 +30,7 @@ Flipper.configuration = nil Flipper.instance = nil - load 'flipper/adapters/mongo.rb' + silence { load 'flipper/adapters/mongo.rb' } ENV["MONGO_URL"] = ENV.fetch("MONGO_URL", "mongodb://127.0.0.1:27017/testing") expect(Flipper.adapter.adapter).to be_a(Flipper::Adapters::Mongo) diff --git a/spec/flipper/api/action_spec.rb b/spec/flipper/api/action_spec.rb index 9dd0e8b94..31c9cb1bb 100644 --- a/spec/flipper/api/action_spec.rb +++ b/spec/flipper/api/action_spec.rb @@ -74,7 +74,7 @@ def delete response = catch(:halt) do action.json_error_response(:feature_not_found) end - status, headers, body = response + _, headers, body = response parsed_body = JSON.parse(body[0]) expect(headers[Rack::CONTENT_TYPE]).to eq('application/json') @@ -88,7 +88,7 @@ def delete response = catch(:halt) do action.json_error_response(:group_not_registered) end - status, headers, body = response + _, headers, body = response parsed_body = JSON.parse(body[0]) expect(headers[Rack::CONTENT_TYPE]).to eq('application/json') diff --git a/spec/flipper/cloud/dsl_spec.rb b/spec/flipper/cloud/dsl_spec.rb index 3eaa66e4a..021f8d6c6 100644 --- a/spec/flipper/cloud/dsl_spec.rb +++ b/spec/flipper/cloud/dsl_spec.rb @@ -45,7 +45,7 @@ end let(:cloud_configuration) do - cloud_configuration = Flipper::Cloud::Configuration.new({ + Flipper::Cloud::Configuration.new({ token: "asdf", sync_secret: "tasty", local_adapter: local_adapter diff --git a/spec/flipper/dsl_spec.rb b/spec/flipper/dsl_spec.rb index cfc35dc12..c2f18f1d8 100644 --- a/spec/flipper/dsl_spec.rb +++ b/spec/flipper/dsl_spec.rb @@ -225,9 +225,6 @@ describe '#enable_group/disable_group' do it 'enables and disables the feature for group' do - actor = Flipper::Actor.new(5) - group = Flipper.register(:fives) { |actor| actor.flipper_id == 5 } - expect(subject[:stats].groups_value).to be_empty subject.enable_group(:stats, :fives) expect(subject[:stats].groups_value).to eq(Set['fives']) diff --git a/spec/flipper/feature_spec.rb b/spec/flipper/feature_spec.rb index 84c7fb75e..600d25db3 100644 --- a/spec/flipper/feature_spec.rb +++ b/spec/flipper/feature_spec.rb @@ -215,7 +215,7 @@ def actor.nil? it 'is recorded for enable' do actor = Flipper::Types::Actor.new(Flipper::Actor.new('1')) - gate = subject.gate_for(actor) + subject.gate_for(actor) subject.enable(actor) @@ -230,7 +230,7 @@ def actor.nil? it 'always instruments flipper type instance for enable' do actor = Flipper::Actor.new('1') - gate = subject.gate_for(actor) + subject.gate_for(actor) subject.enable(actor) @@ -241,7 +241,6 @@ def actor.nil? it 'is recorded for disable' do thing = Flipper::Types::Boolean.new - gate = subject.gate_for(thing) subject.disable(thing) @@ -286,7 +285,6 @@ def actor.nil? it 'always instruments flipper type instance for disable' do actor = Flipper::Actor.new('1') - gate = subject.gate_for(actor) subject.disable(actor) @@ -729,7 +727,6 @@ def actor.nil? context "with expression instance" do it "updates gate values to equal expression or clears expression" do expression = Flipper.property(:plan).eq("basic") - other_expression = Flipper.property(:age).gte(21) expect(subject.gate_values.expression).to be(nil) subject.enable_expression(expression) expect(subject.gate_values.expression).to eq(expression.value) @@ -741,7 +738,6 @@ def actor.nil? context "with Hash" do it "updates gate values to equal expression or clears expression" do expression = Flipper.property(:plan).eq("basic") - other_expression = Flipper.property(:age).gte(21) expect(subject.gate_values.expression).to be(nil) subject.enable_expression(expression.value) expect(subject.gate_values.expression).to eq(expression.value) @@ -1098,8 +1094,6 @@ def actor.nil? describe '#enable_group/disable_group' do context 'with symbol group name' do it 'updates the gate values to include the group' do - actor = Flipper::Actor.new(5) - group = Flipper.register(:five_only) { |actor| actor.flipper_id == 5 } expect(subject.gate_values.groups).to be_empty subject.enable_group(:five_only) expect(subject.gate_values.groups).to eq(Set['five_only']) @@ -1110,8 +1104,6 @@ def actor.nil? context 'with string group name' do it 'updates the gate values to include the group' do - actor = Flipper::Actor.new(5) - group = Flipper.register(:five_only) { |actor| actor.flipper_id == 5 } expect(subject.gate_values.groups).to be_empty subject.enable_group('five_only') expect(subject.gate_values.groups).to eq(Set['five_only']) @@ -1122,7 +1114,6 @@ def actor.nil? context 'with group instance' do it 'updates the gate values for the group' do - actor = Flipper::Actor.new(5) group = Flipper.register(:five_only) { |actor| actor.flipper_id == 5 } expect(subject.gate_values.groups).to be_empty subject.enable_group(group) diff --git a/spec/flipper/instrumentation/log_subscriber_spec.rb b/spec/flipper/instrumentation/log_subscriber_spec.rb index 12628b0c6..ee28f2123 100644 --- a/spec/flipper/instrumentation/log_subscriber_spec.rb +++ b/spec/flipper/instrumentation/log_subscriber_spec.rb @@ -1,4 +1,5 @@ require 'logger' +require 'active_support/core_ext/object/blank' require 'flipper/instrumentation/log_subscriber' require 'flipper/adapters/instrumented' diff --git a/spec/flipper/instrumentation/statsd_subscriber_spec.rb b/spec/flipper/instrumentation/statsd_subscriber_spec.rb index 74e7402d1..1918d527b 100644 --- a/spec/flipper/instrumentation/statsd_subscriber_spec.rb +++ b/spec/flipper/instrumentation/statsd_subscriber_spec.rb @@ -18,7 +18,7 @@ Flipper.new(adapter, instrumenter: ActiveSupport::Notifications) end - let(:user) { user = Flipper::Actor.new('1') } + let(:user) { Flipper::Actor.new('1') } before do described_class.client = statsd_client diff --git a/spec/flipper/middleware/memoizer_spec.rb b/spec/flipper/middleware/memoizer_spec.rb index 1a409c67a..0a3bb1a13 100644 --- a/spec/flipper/middleware/memoizer_spec.rb +++ b/spec/flipper/middleware/memoizer_spec.rb @@ -80,7 +80,7 @@ context 'with preload: true' do let(:app) do # ensure scoped for builder block, annoying... - instance = flipper + flipper middleware = described_class Rack::Builder.new do @@ -141,7 +141,7 @@ context 'with preload specific' do let(:app) do # ensure scoped for builder block, annoying... - instance = flipper + flipper middleware = described_class Rack::Builder.new do @@ -266,7 +266,7 @@ context 'with multiple instances' do let(:app) do # ensure scoped for builder block, annoying... - instance = flipper + flipper middleware = described_class Rack::Builder.new do @@ -316,7 +316,7 @@ def get(uri, params = {}, env = {}, &block) context 'with flipper setup in env' do let(:app) do # ensure scoped for builder block, annoying... - instance = flipper + flipper middleware = described_class Rack::Builder.new do @@ -460,7 +460,6 @@ def get(uri, params = {}, env = {}, &block) cache.clear cached = Flipper::Adapters::ActiveSupportCacheStore.new(logged_memory, cache) logged_cached = Flipper::Adapters::OperationLogger.new(cached) - memo = {} flipper = Flipper.new(logged_cached) flipper[:stats].enable flipper[:shiny].enable diff --git a/spec/flipper_spec.rb b/spec/flipper_spec.rb index cbdb95e94..c64cfc756 100644 --- a/spec/flipper_spec.rb +++ b/spec/flipper_spec.rb @@ -313,7 +313,7 @@ describe '.group_exists' do it 'returns true if the group is already created' do - group = described_class.register('admins', &:admin?) + described_class.register('admins', &:admin?) expect(described_class.group_exists?(:admins)).to eq(true) end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index b334a1d19..4c75a8b51 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -25,6 +25,11 @@ # Disable telemetry logging in specs. ENV["FLIPPER_CLOUD_LOGGING_ENABLED"] = "false" +require 'warning' +Warning.ignore(/lib\/statsd.rb/) +Warning.ignore(/lib\/moneta\/transformer.rb/) +Warning.ignore(/lib\/mongo\/uri.rb/) + RSpec.configure do |config| config.before(:example) do # default stub for telemetry From 97e71d6ee4e95c6e2815c8121b79eb219340248f Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 13:19:23 -0500 Subject: [PATCH 27/40] Try silencing around AR stuff --- spec/flipper/adapters/active_record_spec.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spec/flipper/adapters/active_record_spec.rb b/spec/flipper/adapters/active_record_spec.rb index ca2a8779a..a1d112551 100644 --- a/spec/flipper/adapters/active_record_spec.rb +++ b/spec/flipper/adapters/active_record_spec.rb @@ -53,8 +53,10 @@ end before(:each) do - ActiveRecord::Tasks::DatabaseTasks.purge(config) - CreateFlipperTables.migrate(:up) + silence do + ActiveRecord::Tasks::DatabaseTasks.purge(config) + CreateFlipperTables.migrate(:up) + end end after(:all) do From 8dd22d4a11b1e2be90a3b0f4933eb9e24c03e249 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 13:21:55 -0500 Subject: [PATCH 28/40] Initialize started --- spec/flipper/adapters/http_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/flipper/adapters/http_spec.rb b/spec/flipper/adapters/http_spec.rb index 582035e1c..ac1095216 100644 --- a/spec/flipper/adapters/http_spec.rb +++ b/spec/flipper/adapters/http_spec.rb @@ -28,6 +28,7 @@ end before :all do + @started = false dir = FlipperRoot.join('tmp').tap(&:mkpath) log_path = dir.join('flipper_adapters_http_spec.log') @pstore_file = dir.join('flipper.pstore') From 2dc2ac77afbbfd2c1045950834fdb5523a2f4c3d Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 13:23:12 -0500 Subject: [PATCH 29/40] More liberal ignoring of some warnings --- spec/spec_helper.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 4c75a8b51..19512f426 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -26,9 +26,9 @@ ENV["FLIPPER_CLOUD_LOGGING_ENABLED"] = "false" require 'warning' -Warning.ignore(/lib\/statsd.rb/) -Warning.ignore(/lib\/moneta\/transformer.rb/) -Warning.ignore(/lib\/mongo\/uri.rb/) +Warning.ignore(/lib\/statsd/) +Warning.ignore(/lib\/moneta\//) +Warning.ignore(/lib\/mongo\//) RSpec.configure do |config| config.before(:example) do From c98675f2292e848d394d4a497efc06643150d2d4 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 14:14:00 -0500 Subject: [PATCH 30/40] Fix the to_ary issue This is the worst. It's only an issue with active record objects wrapped by DelegateClass and passed in as an actor for flipper. --- lib/flipper/feature.rb | 11 ++++++++--- spec/flipper/model/active_record_spec.rb | 11 +++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/lib/flipper/feature.rb b/lib/flipper/feature.rb index 2a63f4311..a4dcc63ec 100644 --- a/lib/flipper/feature.rb +++ b/lib/flipper/feature.rb @@ -100,9 +100,14 @@ def clear # # Returns true if enabled, false if not. def enabled?(*actors) - # Allows null object pattern. See PR for more. - # https://github.com/flippercloud/flipper/pull/887 - actors = actors.flatten.reject(&:nil?).map { |actor| Types::Actor.wrap(actor) } + actors = Array(actors). + # Avoids to_ary warning that happens when passing DelegateClass of an + # ActiveRecord object and using flatten here. This is tested in + # spec/flipper/model/active_record_spec.rb. + flat_map { |actor| actor.is_a?(Array) ? actor : [actor] }. + # Allows null object pattern. See PR for more. https://github.com/flippercloud/flipper/pull/887 + reject(&:nil?). + map { |actor| Types::Actor.wrap(actor) } actors = nil if actors.empty? # thing is left for backwards compatibility diff --git a/spec/flipper/model/active_record_spec.rb b/spec/flipper/model/active_record_spec.rb index 304141031..d7bc90ddd 100644 --- a/spec/flipper/model/active_record_spec.rb +++ b/spec/flipper/model/active_record_spec.rb @@ -30,9 +30,20 @@ class User < ActiveRecord::Base include Flipper::Model::ActiveRecord end + class DelegatedUser < DelegateClass(User) + end + class Admin < User end + it "doesn't warn for to_ary" do + # looks like we should remove this but you are wrong, we have specs that + # fail if there are warnings and if this regresses it will print a warning + # so it is in fact testing something + user = User.create!(name: "Test") + Flipper.enabled?(:something, DelegatedUser.new(user)) + end + describe "flipper_id" do it "returns class name and id" do expect(User.new(id: 1).flipper_id).to eq("User;1") From a50e817d98a381641502aa1a062abbe05036375e Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 14:14:10 -0500 Subject: [PATCH 31/40] Move warning suppression higher --- spec/spec_helper.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 19512f426..d4f7069ff 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -9,6 +9,11 @@ Bundler.setup(:default) +require 'warning' +Warning.ignore(/lib\/statsd/) +Warning.ignore(/lib\/moneta\//) +Warning.ignore(/lib\/mongo\//) + require 'debug' require 'statsd' require 'webmock/rspec' @@ -25,11 +30,6 @@ # Disable telemetry logging in specs. ENV["FLIPPER_CLOUD_LOGGING_ENABLED"] = "false" -require 'warning' -Warning.ignore(/lib\/statsd/) -Warning.ignore(/lib\/moneta\//) -Warning.ignore(/lib\/mongo\//) - RSpec.configure do |config| config.before(:example) do # default stub for telemetry From 3f91066db406e937137c32add1dae154bb79114d Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 14:21:33 -0500 Subject: [PATCH 32/40] Fix up warnings --- test_rails/generators/flipper/update_generator_test.rb | 2 +- test_rails/helper.rb | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/test_rails/generators/flipper/update_generator_test.rb b/test_rails/generators/flipper/update_generator_test.rb index 55e08ab10..6c848c26c 100644 --- a/test_rails/generators/flipper/update_generator_test.rb +++ b/test_rails/generators/flipper/update_generator_test.rb @@ -65,7 +65,7 @@ class UpdateGeneratorTest < Rails::Generators::TestCase assert_migration "db/migrate/create_flipper_tables.rb" do |migration| assert_method :up, migration do |up| - assert_match /text :value/, up + assert_match(/text :value/, up) end end diff --git a/test_rails/helper.rb b/test_rails/helper.rb index 10a98c326..335f377f0 100644 --- a/test_rails/helper.rb +++ b/test_rails/helper.rb @@ -4,6 +4,9 @@ require 'rails' require 'rails/test_help' +require 'warning' +Warning.ignore(/lib\/capybara\//) + begin ActiveSupport::TestCase.test_order = :random rescue NoMethodError From ac480386e00571213813a9300fc84516f47e2dbc Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 14:24:28 -0500 Subject: [PATCH 33/40] Ignore ice age and debug warnings --- spec/spec_helper.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index d4f7069ff..6bf6b816f 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -11,6 +11,8 @@ require 'warning' Warning.ignore(/lib\/statsd/) +Warning.ignore(/lib\/debug\//) +Warning.ignore(/lib\/ice_age\//) Warning.ignore(/lib\/moneta\//) Warning.ignore(/lib\/mongo\//) From 38732215b5a7fcb62cb26053b4c81e3442184817 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 14:25:19 -0500 Subject: [PATCH 34/40] Only use ivar if defined --- lib/flipper/ui/views/layout.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/flipper/ui/views/layout.erb b/lib/flipper/ui/views/layout.erb index 431867388..dc6e57034 100644 --- a/lib/flipper/ui/views/layout.erb +++ b/lib/flipper/ui/views/layout.erb @@ -1,7 +1,7 @@ - <%= @page_title ? "#{@page_title} // " : "" %>Flipper + <%= defined?(@page_title) ? "#{@page_title} // " : "" %>Flipper From 6cfb5a49db6bd945e85cfee872fddbf07857b079 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 14:26:19 -0500 Subject: [PATCH 35/40] Give up on warnings for now Not sure why so many in CI that aren't local. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5b78b2f8d..5415968e3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -104,4 +104,4 @@ jobs: ruby-version: ${{ matrix.ruby }} bundler-cache: true # 'bundle install' and cache gems - name: Run Rake with Rails ${{ matrix.rails }} - run: RUBYOPT="-w" bundle exec rake + run: bundle exec rake From 553ec45382d9bad284a94ac3c924105134ce4454 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 14:36:41 -0500 Subject: [PATCH 36/40] Just use bundler/setup --- spec/spec_helper.rb | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 6bf6b816f..68a74dc73 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -2,12 +2,7 @@ require 'pp' require 'pathname' -FlipperRoot = Pathname(__FILE__).dirname.join('..').expand_path - -require 'rubygems' -require 'bundler' - -Bundler.setup(:default) +require 'bundler/setup' require 'warning' Warning.ignore(/lib\/statsd/) @@ -27,6 +22,7 @@ require 'flipper/ui' require 'flipper/test_help' +FlipperRoot = Pathname(__FILE__).dirname.join('..').expand_path Dir[FlipperRoot.join('spec/support/**/*.rb')].sort.each { |f| require f } # Disable telemetry logging in specs. From b2ec155912291a3f6f966aaecca2acd9cc303e2f Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 14:52:25 -0500 Subject: [PATCH 37/40] More liberal stubs --- spec/flipper/cloud/telemetry_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/flipper/cloud/telemetry_spec.rb b/spec/flipper/cloud/telemetry_spec.rb index d170ade86..55992c85f 100644 --- a/spec/flipper/cloud/telemetry_spec.rb +++ b/spec/flipper/cloud/telemetry_spec.rb @@ -45,7 +45,7 @@ expect(telemetry.interval).to eq(120) expect(telemetry.timer.execution_interval).to eq(120) - expect(stub).to have_been_requested + expect(stub).to have_been_requested.at_least_once end it "phones home and requests shutdown if telemetry-shutdown header is true" do @@ -67,7 +67,7 @@ result: true, }) telemetry.stop - expect(stub).to have_been_requested + expect(stub).to have_been_requested.at_least_once expect(output.string).to match(/action=telemetry_shutdown message=The server has requested that telemetry be shut down./) end @@ -90,7 +90,7 @@ result: true, }) telemetry.stop - expect(stub).to have_been_requested + expect(stub).to have_been_requested.at_least_once expect(output.string).not_to match(/action=telemetry_shutdown message=The server has requested that telemetry be shut down./) end From 513e7eb62912a6526cfb5c0817c59699760fd5de Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 14:58:04 -0500 Subject: [PATCH 38/40] Minor formatting [ciskip] --- lib/flipper/instrumentation/statsd.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/flipper/instrumentation/statsd.rb b/lib/flipper/instrumentation/statsd.rb index 1e27de9fa..d641f22d7 100644 --- a/lib/flipper/instrumentation/statsd.rb +++ b/lib/flipper/instrumentation/statsd.rb @@ -4,4 +4,5 @@ ActiveSupport::Notifications.subscribe( /\.flipper$/, - Flipper::Instrumentation::StatsdSubscriber) + Flipper::Instrumentation::StatsdSubscriber +) From ed49fca47559de95461d7893bce1e8bc46b396bb Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 19 Feb 2025 14:58:40 -0500 Subject: [PATCH 39/40] Remove unused call --- lib/flipper/poller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/flipper/poller.rb b/lib/flipper/poller.rb index 3129c098b..a92ef1d09 100644 --- a/lib/flipper/poller.rb +++ b/lib/flipper/poller.rb @@ -59,7 +59,7 @@ def stop def run loop do sleep jitter - Concurrent.monotonic_time + begin sync rescue From d30327325ed527eb897916a775e75eeda7093787 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Mon, 24 Feb 2025 08:12:43 -0500 Subject: [PATCH 40/40] Release 1.3.3 --- lib/flipper/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/flipper/version.rb b/lib/flipper/version.rb index c18a1866b..0f4a94d85 100644 --- a/lib/flipper/version.rb +++ b/lib/flipper/version.rb @@ -1,5 +1,5 @@ module Flipper - VERSION = '1.3.2'.freeze + VERSION = '1.3.3'.freeze REQUIRED_RUBY_VERSION = '2.6'.freeze NEXT_REQUIRED_RUBY_VERSION = '3.0'.freeze