Merge branch 'move-mr-pipeline-creation-to-sidekiq' into 'master'

Move pipeline creation on MR create to Sidekiq

See merge request gitlab-org/gitlab!26172

app/services/merge_requests/after_create_service.rb

+# frozen_string_literal: true
+
+module MergeRequests
+  class AfterCreateService < MergeRequests::BaseService
+    def execute(merge_request)
+      event_service.open_mr(merge_request, current_user)
+      notification_service.new_merge_request(merge_request, current_user)
+
+      # https://gitlab.com/gitlab-org/gitlab/issues/208813
+      if ::Feature.enabled?(:create_merge_request_pipelines_in_sidekiq, project)
+        create_pipeline_for(merge_request, current_user)
+        merge_request.update_head_pipeline
+      end
+
+      merge_request.diffs(include_stats: false).write_cache
+      merge_request.create_cross_references!(current_user)
+    end
+  end
+end

app/services/merge_requests/create_service.rb

@@ -13,19 +13,20 @@ module MergeRequests
       create(merge_request)
     end
 
-    def before_create(merge_request)
-      # current_user (defined in BaseService) is not available within run_after_commit block
-      user = current_user
-      merge_request.run_after_commit do
-        NewMergeRequestWorker.perform_async(merge_request.id, user.id)
-      end
-    end
-
     def after_create(issuable)
+      # Add new items to MergeRequests::AfterCreateService if they can
+      # be performed in Sidekiq
+      NewMergeRequestWorker.perform_async(issuable.id, current_user.id)
+
       todo_service.new_merge_request(issuable, current_user)
       issuable.cache_merge_request_closes_issues!(current_user)
-      create_pipeline_for(issuable, current_user)
-      issuable.update_head_pipeline
+
+      # https://gitlab.com/gitlab-org/gitlab/issues/208813
+      unless ::Feature.enabled?(:create_merge_request_pipelines_in_sidekiq, project)
+        create_pipeline_for(issuable, current_user)
+        issuable.update_head_pipeline
+      end
+
       Gitlab::UsageDataCounters::MergeRequestCounter.count(:create)
       link_lfs_objects(issuable)
 

app/workers/new_merge_request_worker.rb

@@ -12,11 +12,9 @@ class NewMergeRequestWorker # rubocop:disable Scalability/IdempotentWorker
   def perform(merge_request_id, user_id)
     return unless objects_found?(merge_request_id, user_id)
 
-    EventCreateService.new.open_mr(issuable, user)
-    NotificationService.new.new_merge_request(issuable, user)
-
-    issuable.diffs(include_stats: false).write_cache
-    issuable.create_cross_references!(user)
+    MergeRequests::AfterCreateService
+      .new(issuable.target_project, user)
+      .execute(issuable)
   end
 
   def issuable_class

spec/services/merge_requests/after_create_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequests::AfterCreateService do
+  let_it_be(:merge_request) { create(:merge_request) }
+
+  subject(:after_create_service) do
+    described_class.new(merge_request.target_project, merge_request.author)
+  end
+
+  describe '#execute' do
+    let(:event_service) { instance_double('EventCreateService', open_mr: true) }
+    let(:notification_service) { instance_double('NotificationService', new_merge_request: true) }
+
+    before do
+      allow(after_create_service).to receive(:event_service).and_return(event_service)
+      allow(after_create_service).to receive(:notification_service).and_return(notification_service)
+    end
+
+    it 'creates a merge request open event' do
+      expect(event_service)
+        .to receive(:open_mr).with(merge_request, merge_request.author)
+
+      after_create_service.execute(merge_request)
+    end
+
+    it 'creates a new merge request notification' do
+      expect(notification_service)
+        .to receive(:new_merge_request).with(merge_request, merge_request.author)
+
+      after_create_service.execute(merge_request)
+    end
+
+    it 'writes diffs to the cache' do
+      expect(merge_request)
+        .to receive_message_chain(:diffs, :write_cache)
+
+      after_create_service.execute(merge_request)
+    end
+
+    it 'creates cross references' do
+      expect(merge_request)
+        .to receive(:create_cross_references!).with(merge_request.author)
+
+      after_create_service.execute(merge_request)
+    end
+
+    it 'creates a pipeline and updates the HEAD pipeline' do
+      expect(after_create_service)
+        .to receive(:create_pipeline_for).with(merge_request, merge_request.author)
+      expect(merge_request).to receive(:update_head_pipeline)
+
+      after_create_service.execute(merge_request)
+    end
+
+    # https://gitlab.com/gitlab-org/gitlab/issues/208813
+    context 'when the create_merge_request_pipelines_in_sidekiq flag is disabled' do
+      before do
+        stub_feature_flags(create_merge_request_pipelines_in_sidekiq: false)
+      end
+
+      it 'does not create a pipeline or update the HEAD pipeline' do
+        expect(after_create_service).not_to receive(:create_pipeline_for)
+        expect(merge_request).not_to receive(:update_head_pipeline)
+
+        after_create_service.execute(merge_request)
+      end
+    end
+  end
+end

spec/services/merge_requests/create_service_spec.rb

@@ -129,7 +129,23 @@ describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
         end
       end
 
-      context 'when head pipelines already exist for merge request source branch' do
+      # https://gitlab.com/gitlab-org/gitlab/issues/208813
+      context 'when the create_merge_request_pipelines_in_sidekiq flag is disabled' do
+        before do
+          stub_feature_flags(create_merge_request_pipelines_in_sidekiq: false)
+        end
+
+        it 'creates a pipeline and updates the HEAD pipeline' do
+          expect(service).to receive(:create_pipeline_for)
+          expect_next_instance_of(MergeRequest) do |merge_request|
+            expect(merge_request).to receive(:update_head_pipeline)
+          end
+
+          service.execute
+        end
+      end
+
+      context 'when head pipelines already exist for merge request source branch', :sidekiq_inline do
         let(:shas) { project.repository.commits(opts[:source_branch], limit: 2).map(&:id) }
         let!(:pipeline_1) { create(:ci_pipeline, project: project, ref: opts[:source_branch], project_id: project.id, sha: shas[1]) }
         let!(:pipeline_2) { create(:ci_pipeline, project: project, ref: opts[:source_branch], project_id: project.id, sha: shas[0]) }
@@ -175,7 +191,7 @@ describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
         end
       end
 
-      describe 'Pipelines for merge requests' do
+      describe 'Pipelines for merge requests', :sidekiq_inline do
         before do
           stub_ci_pipeline_yaml_file(config)
         end
@@ -216,7 +232,9 @@ describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
               target_project.add_maintainer(user)
             end
 
-            it 'create legacy detached merge request pipeline for fork merge request', :sidekiq_might_not_need_inline do
+            it 'create legacy detached merge request pipeline for fork merge request' do
+              merge_request.reload
+
               expect(merge_request.actual_head_pipeline)
                 .to be_legacy_detached_merge_request_pipeline
             end
@@ -228,6 +246,8 @@ describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
             end
 
             it 'create legacy detached merge request pipeline for non-fork merge request' do
+              merge_request.reload
+
               expect(merge_request.actual_head_pipeline)
                 .to be_legacy_detached_merge_request_pipeline
             end
@@ -262,6 +282,8 @@ describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
             end
 
             it 'sets the latest detached merge request pipeline as the head pipeline' do
+              merge_request.reload
+
               expect(merge_request.actual_head_pipeline).to be_merge_request_event
             end
           end