Merge branch 'rz-safe-navigate-sidekiq-monitoring-settings' into 'master'

Add safe navigators to sidekiq monitoring settings

See merge request gitlab-org/gitlab!77187

sidekiq_cluster/cli.rb

@@ -200,9 +200,7 @@ module Gitlab
       end
 
       def sidekiq_exporter_enabled?
-        ::Settings.monitoring.sidekiq_exporter.enabled
-      rescue Settingslogic::MissingSetting
-        nil
+        ::Settings.dig('monitoring', 'sidekiq_exporter', 'enabled')
       end
 
       def exporter_has_a_unique_port?
@@ -216,15 +214,11 @@ module Gitlab
       end
 
       def sidekiq_exporter_port
-        ::Settings.monitoring.sidekiq_exporter.port
-      rescue Settingslogic::MissingSetting
-        nil
+        ::Settings.dig('monitoring', 'sidekiq_exporter', 'port')
       end
 
       def sidekiq_health_check_port
-        ::Settings.monitoring.sidekiq_health_checks.port
-      rescue Settingslogic::MissingSetting
-        nil
+        ::Settings.dig('monitoring', 'sidekiq_health_checks', 'port')
       end
 
       def metrics_server_enabled?

spec/commands/sidekiq_cluster/cli_spec.rb

@@ -5,7 +5,7 @@ require 'rspec-parameterized'
 
 require_relative '../../../sidekiq_cluster/cli'
 
-RSpec.describe Gitlab::SidekiqCluster::CLI do # rubocop:disable RSpec/FilePath
+RSpec.describe Gitlab::SidekiqCluster::CLI, stubbing_settings_source: true do # rubocop:disable RSpec/FilePath
   let(:cli) { described_class.new('/dev/null') }
   let(:timeout) { Gitlab::SidekiqCluster::DEFAULT_SOFT_TIMEOUT_SECONDS }
   let(:default_options) do
@@ -16,19 +16,39 @@ RSpec.describe Gitlab::SidekiqCluster::CLI do # rubocop:disable RSpec/FilePath
   let(:sidekiq_exporter_port) { '3807' }
   let(:sidekiq_health_checks_port) { '3807' }
 
-  before do
-    stub_env('RAILS_ENV', 'test')
-    stub_config(
-      monitoring: {
-        sidekiq_exporter: {
-          enabled: sidekiq_exporter_enabled,
-          port: sidekiq_exporter_port
+  let(:config_file) { Tempfile.new('gitlab.yml') }
+  let(:config) do
+    {
+      'test' => {
+        'monitoring' => {
+          'sidekiq_exporter' => {
+            'address' => 'localhost',
+            'enabled' => sidekiq_exporter_enabled,
+            'port' => sidekiq_exporter_port
           },
-        sidekiq_health_checks: {
-          port: sidekiq_health_checks_port
+          'sidekiq_health_checks' => {
+            'address' => 'localhost',
+            'enabled' => sidekiq_exporter_enabled,
+            'port' => sidekiq_health_checks_port
+          }
+        }
       }
     }
-    )
+  end
+
+  before do
+    stub_env('RAILS_ENV', 'test')
+
+    config_file.write(YAML.dump(config))
+    config_file.close
+
+    allow(::Settings).to receive(:source).and_return(config_file.path)
+
+    ::Settings.reload!
+  end
+
+  after do
+    config_file.unlink
   end
 
   describe '#run' do
@@ -272,16 +292,9 @@ RSpec.describe Gitlab::SidekiqCluster::CLI do # rubocop:disable RSpec/FilePath
       context 'starting the server' do
         context 'without --dryrun' do
           context 'when there are no sidekiq_health_checks settings set' do
-            before do
-              stub_config(
-                monitoring: {
-                  sidekiq_exporter: {
-                    enabled: true,
-                    port: sidekiq_exporter_port
-                  }
-                }
-              )
+            let(:sidekiq_exporter_enabled) { true }
 
+            before do
               allow(Gitlab::SidekiqCluster).to receive(:start)
               allow(cli).to receive(:write_pid)
               allow(cli).to receive(:trap_signals)
@@ -293,25 +306,42 @@ RSpec.describe Gitlab::SidekiqCluster::CLI do # rubocop:disable RSpec/FilePath
 
               cli.run(%w(foo))
             end
-
-            it 'rescues Settingslogic::MissingSetting' do
-              expect { cli.run(%w(foo)) }.not_to raise_error(Settingslogic::MissingSetting)
-            end
           end
 
           context 'when the sidekiq_exporter.port setting is not set' do
+            let(:sidekiq_exporter_enabled) { true }
+
             before do
-              stub_config(
-                monitoring: {
-                  sidekiq_exporter: {
-                    enabled: true
-                  },
-                  sidekiq_health_checks: {
-                    port: sidekiq_health_checks_port
+              allow(Gitlab::SidekiqCluster).to receive(:start)
+              allow(cli).to receive(:write_pid)
+              allow(cli).to receive(:trap_signals)
+              allow(cli).to receive(:start_loop)
+            end
+
+            it 'does not start a sidekiq metrics server' do
+              expect(MetricsServer).not_to receive(:spawn)
+
+              cli.run(%w(foo))
+            end
+          end
+
+          context 'when sidekiq_exporter.enabled setting is not set' do
+            let(:config) do
+              {
+                'test' => {
+                  'monitoring' => {
+                    'sidekiq_exporter' => {},
+                    'sidekiq_health_checks' => {
+                      'address' => 'localhost',
+                      'enabled' => sidekiq_exporter_enabled,
+                      'port' => sidekiq_health_checks_port
                     }
                   }
-              )
+                }
+              }
+            end
 
+            before do
               allow(Gitlab::SidekiqCluster).to receive(:start)
               allow(cli).to receive(:write_pid)
               allow(cli).to receive(:trap_signals)
@@ -323,23 +353,21 @@ RSpec.describe Gitlab::SidekiqCluster::CLI do # rubocop:disable RSpec/FilePath
 
               cli.run(%w(foo))
             end
-
-            it 'rescues Settingslogic::MissingSetting' do
-              expect { cli.run(%w(foo)) }.not_to raise_error(Settingslogic::MissingSetting)
-            end
           end
 
-          context 'when sidekiq_exporter.enabled setting is not set' do
-            before do
-              stub_config(
-                monitoring: {
-                  sidekiq_exporter: {},
-                  sidekiq_health_checks: {
-                    port: sidekiq_health_checks_port
+          context 'with a blank sidekiq_exporter setting' do
+            let(:config) do
+              {
+                'test' => {
+                  'monitoring' => {
+                    'sidekiq_exporter' => nil,
+                    'sidekiq_health_checks' => nil
                   }
                 }
-              )
+              }
+            end
 
+            before do
               allow(Gitlab::SidekiqCluster).to receive(:start)
               allow(cli).to receive(:write_pid)
               allow(cli).to receive(:trap_signals)
@@ -351,6 +379,10 @@ RSpec.describe Gitlab::SidekiqCluster::CLI do # rubocop:disable RSpec/FilePath
 
               cli.run(%w(foo))
             end
+
+            it 'does not throw an error' do
+              expect { cli.run(%w(foo)) }.not_to raise_error
+            end
           end
 
           context 'with valid settings' do

spec/spec_helper.rb

@@ -463,6 +463,14 @@ RSpec.configure do |config|
     $stdout = STDOUT
   end
 
+  config.around(:each, stubbing_settings_source: true) do |example|
+    original_instance = ::Settings.instance_variable_get(:@instance)
+
+    example.run
+
+    ::Settings.instance_variable_set(:@instance, original_instance)
+  end
+
   config.disable_monkey_patching!
 end