Merge branch 'mk/refactor-request-service' into 'master'

Refactor usages of Geo RequestService

See merge request gitlab-org/gitlab!48420

ee/app/services/geo/metrics_update_service.rb

@@ -29,7 +29,7 @@ module Geo
     end
 
     def send_status_to_primary(node, status)
-      if !NodeStatusRequestService.new.execute(status) && prometheus_enabled?
+      if !NodeStatusRequestService.new(status).execute && prometheus_enabled?
         increment_failed_status_counter(node)
       end
     end

ee/app/services/geo/node_status_request_service.rb

@@ -4,7 +4,13 @@ module Geo
   class NodeStatusRequestService < RequestService
     include Gitlab::Geo::LogHelpers
 
-    def execute(status)
+    attr_reader :status
+
+    def initialize(status)
+      @status = status
+    end
+
+    def execute
       return false unless primary_node.present?
 
       super(primary_status_url, payload(status))

ee/app/services/geo/replication_toggle_request_service.rb

@@ -4,7 +4,13 @@ module Geo
   class ReplicationToggleRequestService < RequestService
     include Gitlab::Geo::LogHelpers
 
-    def execute(enabled:)
+    attr_reader :enabled
+
+    def initialize(enabled:)
+      @enabled = enabled
+    end
+
+    def execute
       return false unless primary_node.present?
 
       success = super(primary_node_api_url, payload(enabled), method: Net::HTTP::Put)

ee/lib/tasks/geo/replication.rake

 namespace :geo do
   namespace :replication do
     task pause: :gitlab_environment do
-      Geo::ReplicationToggleRequestService.new.execute(enabled: false)
+      Geo::ReplicationToggleRequestService.new(enabled: false).execute
     end
 
     task resume: :gitlab_environment do
-      Geo::ReplicationToggleRequestService.new.execute(enabled: true)
+      Geo::ReplicationToggleRequestService.new(enabled: true).execute
     end
   end
 end

ee/spec/services/geo/node_status_request_service_spec.rb

@@ -14,7 +14,7 @@ RSpec.describe Geo::NodeStatusRequestService, :geo do
   end
 
   it_behaves_like 'a geo RequestService' do
-    let(:args) { secondary.find_or_build_status }
+    subject { described_class.new(secondary.find_or_build_status) }
   end
 
   describe '#execute' do
@@ -23,6 +23,13 @@ RSpec.describe Geo::NodeStatusRequestService, :geo do
     end
 
     it 'does not include id in the payload' do
+      args = GeoNodeStatus.new({
+        geo_node_id: secondary.id,
+        status_message: nil,
+        db_replication_lag_seconds: 0,
+        projects_count: 10
+      })
+
       expect(Gitlab::HTTP).to receive(:perform_request)
                                 .with(
                                   Net::HTTP::Post,
@@ -30,15 +37,17 @@ RSpec.describe Geo::NodeStatusRequestService, :geo do
                                   hash_including(body: hash_not_including('id')))
                                 .and_return(double(success?: true))
 
-      subject.execute(GeoNodeStatus.new({
+      described_class.new(args).execute
+    end
+
+    it 'sends geo_node_id in the request' do
+      args = GeoNodeStatus.new({
         geo_node_id: secondary.id,
         status_message: nil,
         db_replication_lag_seconds: 0,
         projects_count: 10
-      }))
-    end
+      })
 
-    it 'sends geo_node_id in the request' do
       expect(Gitlab::HTTP).to receive(:perform_request)
                                 .with(
                                   Net::HTTP::Post,
@@ -46,15 +55,12 @@ RSpec.describe Geo::NodeStatusRequestService, :geo do
                                   hash_including(body: hash_including('geo_node_id' => secondary.id)))
                                 .and_return(double(success?: true))
 
-      subject.execute(GeoNodeStatus.new({
-        geo_node_id: secondary.id,
-        status_message: nil,
-        db_replication_lag_seconds: 0,
-        projects_count: 10
-      }))
+      described_class.new(args).execute
     end
 
     it 'sends all of the data in the status JSONB field in the request' do
+      args = create(:geo_node_status, :healthy)
+
       expect(Gitlab::HTTP).to receive(:perform_request)
                                 .with(
                                   Net::HTTP::Post,
@@ -65,7 +71,7 @@ RSpec.describe Geo::NodeStatusRequestService, :geo do
                                         *GeoNodeStatus::RESOURCE_STATUS_FIELDS))))
                                 .and_return(double(success?: true))
 
-      subject.execute(create(:geo_node_status, :healthy))
+      described_class.new(args).execute
     end
   end
 end

ee/spec/services/geo/replication_toggle_request_service_spec.rb

@@ -8,7 +8,8 @@ RSpec.describe Geo::ReplicationToggleRequestService, :geo do
 
   let_it_be(:secondary) { create(:geo_node) }
   let_it_be(:primary) { create(:geo_node, :primary) }
-  let(:args) { { enabled: false } }
+
+  subject { described_class.new(enabled: false) }
 
   before do
     stub_current_geo_node(secondary)
@@ -22,7 +23,7 @@ RSpec.describe Geo::ReplicationToggleRequestService, :geo do
     allow(Gitlab::HTTP).to receive(:perform_request).and_return(response)
     expect(Gitlab::Geo).to receive(:expire_cache!)
 
-    expect(subject.execute(**args)).to be_truthy
+    expect(subject.execute).to be_truthy
   end
 
   it 'does not expire the geo cache on failure' do
@@ -34,6 +35,6 @@ RSpec.describe Geo::ReplicationToggleRequestService, :geo do
     allow(Gitlab::HTTP).to receive(:perform_request).and_return(response)
     expect(Gitlab::Geo).not_to receive(:expire_cache!)
 
-    expect(subject.execute(**args)).to be_falsey
+    expect(subject.execute).to be_falsey
   end
 end

ee/spec/support/shared_examples/services/geo/geo_request_service_shared_examples.rb

@@ -6,8 +6,6 @@ RSpec.shared_examples 'a geo RequestService' do
 
   let_it_be(:primary) { create(:geo_node, :primary) } unless method_defined?(:primary)
 
-  let(:args) { raise 'args must be supplied in a let variable in order to execute the request' } unless method_defined?(:args)
-
   describe '#execute' do
     it 'parses a 401 response' do
       response = double(success?: false,
@@ -17,14 +15,14 @@ RSpec.shared_examples 'a geo RequestService' do
       allow(Gitlab::HTTP).to receive(:perform_request).and_return(response)
       expect(subject).to receive(:log_error).with("Could not connect to Geo primary node - HTTP Status Code: 401 Unauthorized\nTest")
 
-      expect(subject.execute(args)).to be_falsey
+      expect(subject.execute).to be_falsey
     end
 
     it 'alerts on bad SSL certficate' do
       allow(Gitlab::HTTP).to receive(:perform_request).and_raise(OpenSSL::SSL::SSLError.new('bad certificate'))
       expect(subject).to receive(:log_error).with(/Failed to Net::HTTP::(Put|Post) to primary url: /, kind_of(OpenSSL::SSL::SSLError))
 
-      expect(subject.execute(args)).to be_falsey
+      expect(subject.execute).to be_falsey
     end
 
     it 'handles connection refused' do
@@ -32,7 +30,7 @@ RSpec.shared_examples 'a geo RequestService' do
 
       expect(subject).to receive(:log_error).with(/Failed to Net::HTTP::(Put|Post) to primary url: /, kind_of(Errno::ECONNREFUSED))
 
-      expect(subject.execute(args)).to be_falsey
+      expect(subject.execute).to be_falsey
     end
 
     it 'returns meaningful error message when primary uses incorrect db key' do
@@ -43,13 +41,13 @@ RSpec.shared_examples 'a geo RequestService' do
         kind_of(OpenSSL::Cipher::CipherError)
       )
 
-      expect(subject.execute(args)).to be_falsey
+      expect(subject.execute).to be_falsey
     end
 
     it 'gracefully handles case when primary is deleted' do
       primary.destroy!
 
-      expect(subject.execute(args)).to be_falsey
+      expect(subject.execute).to be_falsey
     end
   end
 end