Merge branch 'move-pipeline-hook-logic-to-a-service' into 'master'

Move pipeline hook logic from model to a service

See merge request gitlab-org/gitlab!71757

app/models/ci/pipeline.rb

@@ -862,11 +862,6 @@ module Ci
       self.duration = Gitlab::Ci::Pipeline::Duration.from_pipeline(self)
     end
 
-    def execute_hooks
-      project.execute_hooks(pipeline_data, :pipeline_hooks) if project.has_active_hooks?(:pipeline_hooks)
-      project.execute_integrations(pipeline_data, :pipeline_hooks) if project.has_active_integrations?(:pipeline_hooks)
-    end
-
     # All the merge requests for which the current pipeline runs/ran against
     def all_merge_requests
       @all_merge_requests ||=
@@ -1252,12 +1247,6 @@ module Ci
       messages.build(severity: severity, content: content)
     end
 
-    def pipeline_data
-      strong_memoize(:pipeline_data) do
-        Gitlab::DataBuilder::Pipeline.build(self)
-      end
-    end
-
     def merge_request_diff_sha
       return unless merge_request?
 

app/services/ci/pipelines/hook_service.rb

+# frozen_string_literal: true
+
+module Ci
+  module Pipelines
+    class HookService
+      include Gitlab::Utils::StrongMemoize
+
+      HOOK_NAME = :pipeline_hooks
+
+      def initialize(pipeline)
+        @pipeline = pipeline
+      end
+
+      def execute
+        project.execute_hooks(hook_data, HOOK_NAME) if project.has_active_hooks?(HOOK_NAME)
+        project.execute_integrations(hook_data, HOOK_NAME) if project.has_active_integrations?(HOOK_NAME)
+      end
+
+      private
+
+      attr_reader :pipeline
+
+      def project
+        @project ||= pipeline.project
+      end
+
+      def hook_data
+        strong_memoize(:hook_data) do
+          Gitlab::DataBuilder::Pipeline.build(pipeline)
+        end
+      end
+    end
+  end
+end

app/workers/pipeline_hooks_worker.rb

@@ -12,9 +12,10 @@ class PipelineHooksWorker # rubocop:disable Scalability/IdempotentWorker
 
   # rubocop: disable CodeReuse/ActiveRecord
   def perform(pipeline_id)
-    Ci::Pipeline
-      .find_by(id: pipeline_id)
-      .try(:execute_hooks)
+    pipeline = Ci::Pipeline.find_by(id: pipeline_id)
+    return unless pipeline
+
+    Ci::Pipelines::HookService.new(pipeline).execute
   end
   # rubocop: enable CodeReuse/ActiveRecord
 end

spec/models/ci/pipeline_spec.rb

@@ -2883,122 +2883,31 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
     end
   end
 
-  describe '#execute_hooks' do
+  describe 'hooks trigerring' do
     let_it_be(:pipeline) { create(:ci_empty_pipeline, :created) }
 
-    let!(:build_a) { create_build('a', 0) }
-    let!(:build_b) { create_build('b', 0) }
+    %i[
+      enqueue
+      request_resource
+      prepare
+      run
+      skip
+      drop
+      succeed
+      cancel
+      block
+      delay
+    ].each do |action|
+      context "when pipeline action is #{action}" do
+        let(:pipeline_action) { action }
 
-    let!(:hook) do
-      create(:project_hook, pipeline_events: enabled)
-    end
-
-    before do
-      WebHookWorker.drain
-    end
-
-    context 'with pipeline hooks enabled' do
-      let(:enabled) { true }
-
-      before do
-        stub_full_request(hook.url, method: :post)
-      end
-
-      context 'with multiple builds', :sidekiq_inline do
-        context 'when build is queued' do
-          before do
-            build_a.reload.enqueue
-            build_b.reload.enqueue
-          end
+        it 'schedules a new PipelineHooksWorker job' do
+          expect(PipelineHooksWorker).to receive(:perform_async).with(pipeline.id)
 
-          it 'receives a pending event once' do
-            expect(WebMock).to have_requested_pipeline_hook('pending').once
+          pipeline.reload.public_send(pipeline_action)
         end
-
-          it 'builds hook data once' do
-            create(:pipelines_email_integration)
-
-            expect(Gitlab::DataBuilder::Pipeline).to receive(:build).once.and_call_original
-
-            pipeline.execute_hooks
-          end
-        end
-
-        context 'when build is run' do
-          before do
-            build_a.reload.enqueue
-            build_a.reload.run!
-            build_b.reload.enqueue
-            build_b.reload.run!
-          end
-
-          it 'receives a running event once' do
-            expect(WebMock).to have_requested_pipeline_hook('running').once
-          end
-        end
-
-        context 'when all builds succeed' do
-          before do
-            build_a.success
-
-            # We have to reload build_b as this is in next stage and it gets triggered by PipelineProcessWorker
-            build_b.reload.success
-          end
-
-          it 'receives a success event once' do
-            expect(WebMock).to have_requested_pipeline_hook('success').once
       end
     end
-
-        context 'when stage one failed' do
-          let!(:build_b) { create_build('b', 1) }
-
-          before do
-            build_a.drop
-          end
-
-          it 'receives a failed event once' do
-            expect(WebMock).to have_requested_pipeline_hook('failed').once
-          end
-        end
-
-        def have_requested_pipeline_hook(status)
-          have_requested(:post, stubbed_hostname(hook.url)).with do |req|
-            json_body = Gitlab::Json.parse(req.body)
-            json_body['object_attributes']['status'] == status &&
-              json_body['builds'].length == 2
-          end
-        end
-      end
-    end
-
-    context 'with pipeline hooks disabled' do
-      let(:enabled) { false }
-
-      before do
-        build_a.enqueue
-        build_b.enqueue
-      end
-
-      it 'did not execute pipeline_hook after touched' do
-        expect(WebMock).not_to have_requested(:post, hook.url)
-      end
-
-      it 'does not build hook data' do
-        expect(Gitlab::DataBuilder::Pipeline).not_to receive(:build)
-
-        pipeline.execute_hooks
-      end
-    end
-
-    def create_build(name, stage_idx)
-      create(:ci_build,
-             :created,
-             pipeline: pipeline,
-             name: name,
-             stage: "stage:#{stage_idx}",
-             stage_idx: stage_idx)
-    end
   end
 
   describe "#merge_requests_as_head_pipeline" do

spec/services/ci/pipelines/hook_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::Pipelines::HookService do
+  describe '#execute_hooks' do
+    let_it_be(:namespace) { create(:namespace) }
+    let_it_be(:project) { create(:project, :repository, namespace: namespace) }
+    let_it_be(:pipeline) { create(:ci_empty_pipeline, :created, project: project) }
+
+    let(:hook_enabled) { true }
+    let!(:hook) { create(:project_hook, project: project, pipeline_events: hook_enabled) }
+    let(:hook_data) { double }
+
+    subject(:service) { described_class.new(pipeline) }
+
+    describe 'HOOK_NAME' do
+      specify { expect(described_class::HOOK_NAME).to eq(:pipeline_hooks) }
+    end
+
+    context 'with pipeline hooks enabled' do
+      before do
+        allow(Gitlab::DataBuilder::Pipeline).to receive(:build).with(pipeline).once.and_return(hook_data)
+      end
+
+      it 'calls pipeline.project.execute_hooks and pipeline.project.execute_integrations' do
+        create(:pipelines_email_integration, project: project)
+
+        expect(pipeline.project).to receive(:execute_hooks).with(hook_data, described_class::HOOK_NAME)
+        expect(pipeline.project).to receive(:execute_integrations).with(hook_data, described_class::HOOK_NAME)
+
+        service.execute
+      end
+    end
+
+    context 'with pipeline hooks and integrations disabled' do
+      let(:hook_enabled) { false }
+
+      it 'does not call pipeline.project.execute_hooks and pipeline.project.execute_integrations' do
+        expect(pipeline.project).not_to receive(:execute_hooks)
+        expect(pipeline.project).not_to receive(:execute_integrations)
+
+        service.execute
+      end
+    end
+  end
+end

spec/workers/pipeline_hooks_worker_spec.rb

@@ -8,8 +8,10 @@ RSpec.describe PipelineHooksWorker do
       let(:pipeline) { create(:ci_pipeline) }
 
       it 'executes hooks for the pipeline' do
-        expect_any_instance_of(Ci::Pipeline)
-          .to receive(:execute_hooks)
+        hook_service = double
+
+        expect(Ci::Pipelines::HookService).to receive(:new).and_return(hook_service)
+        expect(hook_service).to receive(:execute)
 
         described_class.new.perform(pipeline.id)
       end
@@ -17,6 +19,8 @@ RSpec.describe PipelineHooksWorker do
 
     context 'when pipeline does not exist' do
       it 'does not raise exception' do
+        expect(Ci::Pipelines::HookService).not_to receive(:new)
+
         expect { described_class.new.perform(123) }
           .not_to raise_error
       end