From a6d36a6614d22f72696d3b579a0e0eff9352b882 Mon Sep 17 00:00:00 2001
From: Fabio Pitino <fpitino@gitlab.com>
Date: Mon, 9 May 2022 16:20:31 +0100
Subject: [PATCH 1/8] Support push options when creating merge request
 pipelines

Allow merge request pipeline creation to take in input
Git push options in order to support `ci.skip`.

Changelog: other
EE: true
---
 app/services/git/branch_push_service.rb       |  1 +
 app/services/merge_requests/base_service.rb   |  5 ++-
 .../merge_requests/create_pipeline_service.rb | 14 +++---
 .../merge_requests/refresh_service.rb         |  5 ++-
 .../merge_requests/create_pipeline_worker.rb  |  9 +++-
 app/workers/update_merge_requests_worker.rb   |  8 +++-
 .../merge_requests/create_pipeline_service.rb | 18 ++++----
 .../create_pipeline_service_spec.rb           | 14 ++++++
 .../create_pipeline_service_spec.rb           | 13 ++++++
 .../merge_requests/refresh_service_spec.rb    | 13 ++++++
 .../create_pipeline_worker_spec.rb            | 31 ++++++++++---
 .../update_merge_requests_worker_spec.rb      | 43 ++++++++++++-------
 12 files changed, 131 insertions(+), 43 deletions(-)

diff --git a/app/services/git/branch_push_service.rb b/app/services/git/branch_push_service.rb
index 3c27ad56ebb486..91f1425160819d 100644
--- a/app/services/git/branch_push_service.rb
+++ b/app/services/git/branch_push_service.rb
@@ -39,6 +39,7 @@ def execute
     def enqueue_update_mrs
       return if params[:merge_request_branches]&.exclude?(branch_name)
 
+      # TODO: pass params[:push_options] to worker
       UpdateMergeRequestsWorker.perform_async(
         project.id,
         current_user.id,
diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index 30ad549db915bb..cb6b46c0aa91f0 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/app/services/merge_requests/base_service.rb
@@ -183,11 +183,12 @@ def create_reviewer_note(merge_request, old_reviewers)
         merge_request, merge_request.project, current_user, old_reviewers)
     end
 
-    def create_pipeline_for(merge_request, user, async: false)
+    def create_pipeline_for(merge_request, user, async: false, push_options: nil)
       if async
+        # TODO: pass push_options to worker
         MergeRequests::CreatePipelineWorker.perform_async(project.id, user.id, merge_request.id)
       else
-        MergeRequests::CreatePipelineService.new(project: project, current_user: user).execute(merge_request)
+        MergeRequests::CreatePipelineService.new(project: project, current_user: user).execute(merge_request, push_options: push_options)
       end
     end
 
diff --git a/app/services/merge_requests/create_pipeline_service.rb b/app/services/merge_requests/create_pipeline_service.rb
index 9d7f8393ba52cb..a3b0eec6d24ebe 100644
--- a/app/services/merge_requests/create_pipeline_service.rb
+++ b/app/services/merge_requests/create_pipeline_service.rb
@@ -2,16 +2,18 @@
 
 module MergeRequests
   class CreatePipelineService < MergeRequests::BaseService
-    def execute(merge_request)
+    def execute(merge_request, push_options: nil)
       return cannot_create_pipeline_error unless can_create_pipeline_for?(merge_request)
 
-      create_detached_merge_request_pipeline(merge_request)
+      create_detached_merge_request_pipeline(merge_request, push_options)
     end
 
-    def create_detached_merge_request_pipeline(merge_request)
-      Ci::CreatePipelineService.new(pipeline_project(merge_request),
-                                    current_user,
-                                    ref: pipeline_ref_for_detached_merge_request_pipeline(merge_request))
+    def create_detached_merge_request_pipeline(merge_request, push_options)
+      Ci::CreatePipelineService
+        .new(pipeline_project(merge_request),
+          current_user,
+          ref: pipeline_ref_for_detached_merge_request_pipeline(merge_request),
+          push_options: push_options)
         .execute(:merge_request_event, merge_request: merge_request)
     end
 
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index f7a0f90b95f2bc..3a6d9794a049a9 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/app/services/merge_requests/refresh_service.rb
@@ -5,8 +5,9 @@ class RefreshService < MergeRequests::BaseService
     include Gitlab::Utils::StrongMemoize
     attr_reader :push
 
-    def execute(oldrev, newrev, ref)
+    def execute(oldrev, newrev, ref, push_options: nil)
       @push = Gitlab::Git::Push.new(@project, oldrev, newrev, ref)
+      @push_options = push_options
       return true unless @push.branch_push?
 
       refresh_merge_requests!
@@ -163,7 +164,7 @@ def outdate_service
     end
 
     def refresh_pipelines_on_merge_requests(merge_request)
-      create_pipeline_for(merge_request, current_user, async: true)
+      create_pipeline_for(merge_request, current_user, async: true, push_options: @push_options)
     end
 
     def abort_auto_merges(merge_request)
diff --git a/app/workers/merge_requests/create_pipeline_worker.rb b/app/workers/merge_requests/create_pipeline_worker.rb
index ee42a3dee08ac9..e237e1fef42d39 100644
--- a/app/workers/merge_requests/create_pipeline_worker.rb
+++ b/app/workers/merge_requests/create_pipeline_worker.rb
@@ -15,7 +15,7 @@ class CreatePipelineWorker
     worker_resource_boundary :cpu
     idempotent!
 
-    def perform(project_id, user_id, merge_request_id)
+    def perform(project_id, user_id, merge_request_id, params = {})
       project = Project.find_by_id(project_id)
       return unless project
 
@@ -25,7 +25,12 @@ def perform(project_id, user_id, merge_request_id)
       merge_request = MergeRequest.find_by_id(merge_request_id)
       return unless merge_request
 
-      MergeRequests::CreatePipelineService.new(project: project, current_user: user).execute(merge_request)
+      params.deep_symbolize_keys!
+
+      MergeRequests::CreatePipelineService
+        .new(project: project, current_user: user)
+        .execute(merge_request, push_options: params[:push_options])
+
       merge_request.update_head_pipeline
     end
   end
diff --git a/app/workers/update_merge_requests_worker.rb b/app/workers/update_merge_requests_worker.rb
index 5c96257cb632ec..ccc7af285740e9 100644
--- a/app/workers/update_merge_requests_worker.rb
+++ b/app/workers/update_merge_requests_worker.rb
@@ -13,13 +13,17 @@ class UpdateMergeRequestsWorker # rubocop:disable Scalability/IdempotentWorker
   weight 3
   loggable_arguments 2, 3, 4
 
-  def perform(project_id, user_id, oldrev, newrev, ref)
+  def perform(project_id, user_id, oldrev, newrev, ref, params = {})
     project = Project.find_by_id(project_id)
     return unless project
 
     user = User.find_by_id(user_id)
     return unless user
 
-    MergeRequests::RefreshService.new(project: project, current_user: user).execute(oldrev, newrev, ref)
+    params.deep_symbolize_keys!
+
+    MergeRequests::RefreshService
+      .new(project: project, current_user: user)
+      .execute(oldrev, newrev, ref, push_options: params[:push_options])
   end
 end
diff --git a/ee/app/services/ee/merge_requests/create_pipeline_service.rb b/ee/app/services/ee/merge_requests/create_pipeline_service.rb
index 1c2127cc34169f..ed48941d1a574c 100644
--- a/ee/app/services/ee/merge_requests/create_pipeline_service.rb
+++ b/ee/app/services/ee/merge_requests/create_pipeline_service.rb
@@ -6,15 +6,15 @@ module CreatePipelineService
       extend ::Gitlab::Utils::Override
 
       override :execute
-      def execute(merge_request)
-        response = create_merged_result_pipeline_for(merge_request)
+      def execute(merge_request, push_options: nil)
+        response = create_merged_result_pipeline_for(merge_request, push_options)
 
         return response if response.success?
 
         super
       end
 
-      def create_merged_result_pipeline_for(merge_request)
+      def create_merged_result_pipeline_for(merge_request, push_options)
         return cannot_create_pipeline_error unless can_create_merged_result_pipeline_for?(merge_request)
 
         result = ::MergeRequests::MergeabilityCheckService.new(merge_request).execute(recheck: true)
@@ -22,11 +22,13 @@ def create_merged_result_pipeline_for(merge_request)
         if result.success? && merge_request.mergeable_state?(skip_ci_check: true, skip_discussions_check: true)
           ref_payload = result.payload.fetch(:merge_ref_head)
 
-          ::Ci::CreatePipelineService.new(merge_request.target_project, current_user,
-                                          ref: merge_request.merge_ref_path,
-                                          checkout_sha: ref_payload[:commit_id],
-                                          target_sha: ref_payload[:target_id],
-                                          source_sha: ref_payload[:source_id])
+          ::Ci::CreatePipelineService
+            .new(merge_request.target_project, current_user,
+              ref: merge_request.merge_ref_path,
+              checkout_sha: ref_payload[:commit_id],
+              target_sha: ref_payload[:target_id],
+              source_sha: ref_payload[:source_id],
+              push_options: push_options)
             .execute(:merge_request_event, merge_request: merge_request)
         else
           cannot_create_pipeline_error
diff --git a/ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb b/ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb
index e6196e7fa59c33..ffc8442a5e38fc 100644
--- a/ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb
+++ b/ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb
@@ -72,6 +72,20 @@
       expect(subject.payload).to eq(Ci::Pipeline.last)
     end
 
+    context 'when push options contain ci.skip' do
+      subject { service.execute(merge_request, push_options: { 'ci' => { 'skip' => true } }) }
+
+      it 'creates a skipped pipeline' do
+        subject
+
+        expect(merge_request.all_pipelines.count).to eq(1)
+        pipeline = merge_request.all_pipelines.last
+        expect(pipeline).to be_merged_result_pipeline
+        expect(pipeline.builds).to be_empty
+        expect(pipeline).to be_skipped
+      end
+    end
+
     context 'when merge request is WIP' do
       before do
         merge_request.update!(title: merge_request.wip_title)
diff --git a/spec/services/merge_requests/create_pipeline_service_spec.rb b/spec/services/merge_requests/create_pipeline_service_spec.rb
index d84ce8d15b4b42..ce73e1625bc170 100644
--- a/spec/services/merge_requests/create_pipeline_service_spec.rb
+++ b/spec/services/merge_requests/create_pipeline_service_spec.rb
@@ -50,6 +50,19 @@
       expect(response.payload.source).to eq('merge_request_event')
     end
 
+    context 'when push options contain ci.skip' do
+      let(:response) { service.execute(merge_request, push_options: { 'ci' => { 'skip' => true } }) }
+
+      it 'creates a skipped pipeline' do
+        expect { response }.to change { Ci::Pipeline.count }.by(1)
+
+        expect(response).to be_success
+        expect(response.payload).to be_persisted
+        expect(response.payload.builds).to be_empty
+        expect(response.payload).to be_skipped
+      end
+    end
+
     context 'with fork merge request' do
       let_it_be(:forked_project) { fork_project(project, nil, repository: true, target_project: create(:project, :private, :repository)) }
 
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index 6e6b4a91e0db2c..309ecba70c632e 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/spec/services/merge_requests/refresh_service_spec.rb
@@ -228,6 +228,19 @@
           expect(@another_merge_request.has_commits?).to be_falsy
         end
 
+        context 'when "push_options: nil" is passed' do
+          subject { service.new(project: project, current_user: @user).execute(@oldrev, @newrev, ref, push_options: nil) }
+
+          it 'create detached merge request pipeline with commits' do
+            expect { subject }
+              .to change { @merge_request.pipelines_for_merge_request.count }.by(1)
+              .and change { @another_merge_request.pipelines_for_merge_request.count }.by(0)
+
+            expect(@merge_request.has_commits?).to be_truthy
+            expect(@another_merge_request.has_commits?).to be_falsy
+          end
+        end
+
         it 'does not create detached merge request pipeline for forked project' do
           expect { subject }
             .not_to change { @fork_merge_request.pipelines_for_merge_request.count }
diff --git a/spec/workers/merge_requests/create_pipeline_worker_spec.rb b/spec/workers/merge_requests/create_pipeline_worker_spec.rb
index 06d44c45706e7e..7efed0ca9363ae 100644
--- a/spec/workers/merge_requests/create_pipeline_worker_spec.rb
+++ b/spec/workers/merge_requests/create_pipeline_worker_spec.rb
@@ -3,24 +3,44 @@
 require 'spec_helper'
 
 RSpec.describe MergeRequests::CreatePipelineWorker do
-  subject(:worker) { described_class.new }
-
   describe '#perform' do
     let(:user) { create(:user) }
     let(:project) { create(:project) }
     let(:merge_request) { create(:merge_request) }
+    let(:worker) { described_class.new }
+
+    subject { worker.perform(project.id, user.id, merge_request.id) }
 
     context 'when the objects exist' do
       it 'calls the merge request create pipeline service and calls update head pipeline' do
         aggregate_failures do
           expect_next_instance_of(MergeRequests::CreatePipelineService, project: project, current_user: user) do |service|
-            expect(service).to receive(:execute).with(merge_request)
+            expect(service).to receive(:execute).with(merge_request, push_options: nil)
           end
 
           expect(MergeRequest).to receive(:find_by_id).with(merge_request.id).and_return(merge_request)
           expect(merge_request).to receive(:update_head_pipeline)
 
-          subject.perform(project.id, user.id, merge_request.id)
+          subject
+        end
+      end
+
+      context 'when push options are passed to the worker' do
+        let(:extra_params) { { 'push_options' => { 'ci' => { 'skip' => true } } } }
+
+        subject { worker.perform(project.id, user.id, merge_request.id, extra_params) }
+
+        it 'calls the merge request create pipeline service and calls update head pipeline' do
+          aggregate_failures do
+            expect_next_instance_of(MergeRequests::CreatePipelineService, project: project, current_user: user) do |service|
+              expect(service).to receive(:execute).with(merge_request, push_options: { ci: { skip: true } })
+            end
+
+            expect(MergeRequest).to receive(:find_by_id).with(merge_request.id).and_return(merge_request)
+            expect(merge_request).to receive(:update_head_pipeline)
+
+            subject
+          end
         end
       end
     end
@@ -29,8 +49,7 @@
       it 'does not call the create pipeline service' do
         expect(MergeRequests::CreatePipelineService).not_to receive(:new)
 
-        expect { subject.perform(project.id, user.id, merge_request.id) }
-          .not_to raise_exception
+        expect { subject }.not_to raise_exception
       end
     end
 
diff --git a/spec/workers/update_merge_requests_worker_spec.rb b/spec/workers/update_merge_requests_worker_spec.rb
index bd0dc2f9ef4ccf..fd5a6ee20b54d5 100644
--- a/spec/workers/update_merge_requests_worker_spec.rb
+++ b/spec/workers/update_merge_requests_worker_spec.rb
@@ -3,28 +3,41 @@
 require 'spec_helper'
 
 RSpec.describe UpdateMergeRequestsWorker do
-  include RepoHelpers
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:user)    { create(:user) }
+  let_it_be(:oldrev)  { "123456" }
+  let_it_be(:newrev)  { "789012" }
+  let_it_be(:ref)     { "refs/heads/test" }
 
-  let(:project) { create(:project, :repository) }
-  let(:user) { create(:user) }
-
-  subject { described_class.new }
+  let(:worker) { described_class.new }
 
   describe '#perform' do
-    let(:oldrev) { "123456" }
-    let(:newrev) { "789012" }
-    let(:ref)    { "refs/heads/test" }
-
-    def perform
-      subject.perform(project.id, user.id, oldrev, newrev, ref)
-    end
+    subject { worker.perform(project.id, user.id, oldrev, newrev, ref) }
 
     it 'executes MergeRequests::RefreshService with expected values' do
-      expect_next_instance_of(MergeRequests::RefreshService, project: project, current_user: user) do |refresh_service|
-        expect(refresh_service).to receive(:execute).with(oldrev, newrev, ref)
+      expect_next_instance_of(MergeRequests::RefreshService, project: project, current_user: user) do |service|
+        expect(service)
+          .to receive(:execute)
+          .with(oldrev, newrev, ref, push_options: nil)
       end
 
-      perform
+      subject
+    end
+
+    context 'when push options are passed' do
+      let(:extra_params) { { 'push_options' => { 'ci' => { 'skip' => true } } } }
+
+      subject { worker.perform(project.id, user.id, oldrev, newrev, ref, extra_params) }
+
+      it 'executes MergeRequests::RefreshService with expected values' do
+        expect_next_instance_of(MergeRequests::RefreshService, project: project, current_user: user) do |service|
+          expect(service)
+            .to receive(:execute)
+            .with(oldrev, newrev, ref, push_options: { ci: { skip: true } })
+        end
+
+        subject
+      end
     end
   end
 end
-- 
GitLab


From 9854f1c3d9634573012ee1325f1361ed43f0baf1 Mon Sep 17 00:00:00 2001
From: Avielle Wolfe <awolfe@gitlab.com>
Date: Mon, 16 May 2022 12:05:00 +0000
Subject: [PATCH 2/8] Apply 1 suggestion(s) to 1 file(s)

---
 spec/services/merge_requests/refresh_service_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index 309ecba70c632e..3f697562725cb0 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/spec/services/merge_requests/refresh_service_spec.rb
@@ -231,7 +231,7 @@
         context 'when "push_options: nil" is passed' do
           subject { service.new(project: project, current_user: @user).execute(@oldrev, @newrev, ref, push_options: nil) }
 
-          it 'create detached merge request pipeline with commits' do
+          it 'creates a detached merge request pipeline with commits' do
             expect { subject }
               .to change { @merge_request.pipelines_for_merge_request.count }.by(1)
               .and change { @another_merge_request.pipelines_for_merge_request.count }.by(0)
-- 
GitLab


From 313a58ea5e55e1bc69db4d3e07ad553f4151f5a6 Mon Sep 17 00:00:00 2001
From: Fabio Pitino <fpitino@gitlab.com>
Date: Tue, 17 May 2022 11:33:07 +0100
Subject: [PATCH 3/8] Add ApplicationWorker#parse_json_arg

---
 app/workers/concerns/application_worker.rb    | 20 +++++++++++
 .../concerns/application_worker_spec.rb       | 36 +++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/app/workers/concerns/application_worker.rb b/app/workers/concerns/application_worker.rb
index c1fec4f01966da..71682c7a7edfb9 100644
--- a/app/workers/concerns/application_worker.rb
+++ b/app/workers/concerns/application_worker.rb
@@ -216,4 +216,24 @@ def process_schedule_at_for_batch(schedule_at, index)
       schedule_at
     end
   end
+
+  # In Sidekiq 7.0, Hash arguments are not considered safe for serialization.
+  # We should dump the hash parameter to JSON when calling `perform_async` and
+  # we can use this method to convert it back.
+  # This method supports backward compatibility to allow converting existing
+  # Hash arguments to JSON arguments.
+  # For more info see https://github.com/mperham/sidekiq/wiki/Best-Practices
+  def parse_json_arg(params)
+    case params
+    when Hash then params.deep_symbolize_keys!
+    when nil  then {}
+    when String
+      result = Gitlab::Json.parse(params)
+      result.present? ? result.deep_symbolize_keys! : {}
+    else
+      raise ArgumentError, "Argument type not supported, got: #{params}"
+    end
+  rescue JSON::ParserError
+    raise ArgumentError, "Argument cannot be parsed, got: #{params}"
+  end
 end
diff --git a/spec/workers/concerns/application_worker_spec.rb b/spec/workers/concerns/application_worker_spec.rb
index 707fa0c9c78402..6a8f9646b05cec 100644
--- a/spec/workers/concerns/application_worker_spec.rb
+++ b/spec/workers/concerns/application_worker_spec.rb
@@ -537,4 +537,40 @@ def self.name
       end
     end
   end
+
+  describe '#parse_json_arg' do
+    using RSpec::Parameterized::TableSyntax
+
+    subject { instance.parse_json_arg(param) }
+
+    where(:param, :result) do
+      nil                                   | {}
+      "null"                                | {}
+      { foo: 123 }                          | { foo: 123 } # rubocop: disable Lint/BinaryOperatorWithIdenticalOperands
+      { 'foo' => 123 }                      | { foo: 123 }
+      { 'foo' => { 'bar' => 123 } }         | { foo: { bar: 123 } }
+      { test: 123 }.to_json                 | { test: 123 }
+      { 'foo' => { 'bar' => 123 } }.to_json | { foo: { bar: 123 } }
+    end
+
+    with_them do
+      it { is_expected.to eq(result) }
+    end
+
+    context 'when parameter type is not supported' do
+      let(:param) { 123 }
+
+      it 'raises an error' do
+        expect { subject }.to raise_error(ArgumentError, 'Argument type not supported, got: 123')
+      end
+    end
+
+    context 'when parameter is not a JSON string' do
+      let(:param) { 'test' }
+
+      it 'raises an error' do
+        expect { subject }.to raise_error(ArgumentError, 'Argument cannot be parsed, got: test')
+      end
+    end
+  end
 end
-- 
GitLab


From 93b0cd43f48791fe0d9705b28779e1baf8cef273 Mon Sep 17 00:00:00 2001
From: Fabio Pitino <fpitino@gitlab.com>
Date: Tue, 17 May 2022 11:33:46 +0100
Subject: [PATCH 4/8] Parse json parameter in worker

---
 .../merge_requests/create_pipeline_worker.rb  |  4 +-
 app/workers/update_merge_requests_worker.rb   |  4 +-
 .../create_pipeline_service_spec.rb           |  2 +-
 .../create_pipeline_service_spec.rb           |  2 +-
 .../create_pipeline_worker_spec.rb            | 40 ++++++++++++++++++-
 .../update_merge_requests_worker_spec.rb      | 34 +++++++++++++++-
 6 files changed, 78 insertions(+), 8 deletions(-)

diff --git a/app/workers/merge_requests/create_pipeline_worker.rb b/app/workers/merge_requests/create_pipeline_worker.rb
index e237e1fef42d39..07fbe2ddb7242c 100644
--- a/app/workers/merge_requests/create_pipeline_worker.rb
+++ b/app/workers/merge_requests/create_pipeline_worker.rb
@@ -15,7 +15,7 @@ class CreatePipelineWorker
     worker_resource_boundary :cpu
     idempotent!
 
-    def perform(project_id, user_id, merge_request_id, params = {})
+    def perform(project_id, user_id, merge_request_id, params = nil)
       project = Project.find_by_id(project_id)
       return unless project
 
@@ -25,7 +25,7 @@ def perform(project_id, user_id, merge_request_id, params = {})
       merge_request = MergeRequest.find_by_id(merge_request_id)
       return unless merge_request
 
-      params.deep_symbolize_keys!
+      params = parse_json_arg(params)
 
       MergeRequests::CreatePipelineService
         .new(project: project, current_user: user)
diff --git a/app/workers/update_merge_requests_worker.rb b/app/workers/update_merge_requests_worker.rb
index ccc7af285740e9..c43eeab468f758 100644
--- a/app/workers/update_merge_requests_worker.rb
+++ b/app/workers/update_merge_requests_worker.rb
@@ -13,14 +13,14 @@ class UpdateMergeRequestsWorker # rubocop:disable Scalability/IdempotentWorker
   weight 3
   loggable_arguments 2, 3, 4
 
-  def perform(project_id, user_id, oldrev, newrev, ref, params = {})
+  def perform(project_id, user_id, oldrev, newrev, ref, params = nil)
     project = Project.find_by_id(project_id)
     return unless project
 
     user = User.find_by_id(user_id)
     return unless user
 
-    params.deep_symbolize_keys!
+    params = parse_json_arg(params)
 
     MergeRequests::RefreshService
       .new(project: project, current_user: user)
diff --git a/ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb b/ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb
index ffc8442a5e38fc..fae71f4b71c262 100644
--- a/ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb
+++ b/ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb
@@ -73,7 +73,7 @@
     end
 
     context 'when push options contain ci.skip' do
-      subject { service.execute(merge_request, push_options: { 'ci' => { 'skip' => true } }) }
+      subject { service.execute(merge_request, push_options: { ci: { skip: true } }) }
 
       it 'creates a skipped pipeline' do
         subject
diff --git a/spec/services/merge_requests/create_pipeline_service_spec.rb b/spec/services/merge_requests/create_pipeline_service_spec.rb
index ce73e1625bc170..0c1087a1f99c6d 100644
--- a/spec/services/merge_requests/create_pipeline_service_spec.rb
+++ b/spec/services/merge_requests/create_pipeline_service_spec.rb
@@ -51,7 +51,7 @@
     end
 
     context 'when push options contain ci.skip' do
-      let(:response) { service.execute(merge_request, push_options: { 'ci' => { 'skip' => true } }) }
+      let(:response) { service.execute(merge_request, push_options: { ci: { skip: true } }) }
 
       it 'creates a skipped pipeline' do
         expect { response }.to change { Ci::Pipeline.count }.by(1)
diff --git a/spec/workers/merge_requests/create_pipeline_worker_spec.rb b/spec/workers/merge_requests/create_pipeline_worker_spec.rb
index 7efed0ca9363ae..b8a9551de545fb 100644
--- a/spec/workers/merge_requests/create_pipeline_worker_spec.rb
+++ b/spec/workers/merge_requests/create_pipeline_worker_spec.rb
@@ -25,7 +25,7 @@
         end
       end
 
-      context 'when push options are passed to the worker' do
+      context 'when push options are passed as Hash to the worker' do
         let(:extra_params) { { 'push_options' => { 'ci' => { 'skip' => true } } } }
 
         subject { worker.perform(project.id, user.id, merge_request.id, extra_params) }
@@ -43,6 +43,44 @@
           end
         end
       end
+
+      context 'when push options are passed as JSON string to the worker' do
+        let(:extra_params) { { 'push_options' => { 'ci' => { 'skip' => true } } }.to_json }
+
+        subject { worker.perform(project.id, user.id, merge_request.id, extra_params) }
+
+        it 'calls the merge request create pipeline service and calls update head pipeline' do
+          aggregate_failures do
+            expect_next_instance_of(MergeRequests::CreatePipelineService, project: project, current_user: user) do |service|
+              expect(service).to receive(:execute).with(merge_request, push_options: { ci: { skip: true } })
+            end
+
+            expect(MergeRequest).to receive(:find_by_id).with(merge_request.id).and_return(merge_request)
+            expect(merge_request).to receive(:update_head_pipeline)
+
+            subject
+          end
+        end
+      end
+
+      context 'when extra parameters are omitted and defaulted to "null"' do
+        let(:extra_params) { "null" }
+
+        subject { worker.perform(project.id, user.id, merge_request.id, extra_params) }
+
+        it 'calls the merge request create pipeline service and calls update head pipeline' do
+          aggregate_failures do
+            expect_next_instance_of(MergeRequests::CreatePipelineService, project: project, current_user: user) do |service|
+              expect(service).to receive(:execute).with(merge_request, push_options: nil)
+            end
+
+            expect(MergeRequest).to receive(:find_by_id).with(merge_request.id).and_return(merge_request)
+            expect(merge_request).to receive(:update_head_pipeline)
+
+            subject
+          end
+        end
+      end
     end
 
     shared_examples 'when object does not exist' do
diff --git a/spec/workers/update_merge_requests_worker_spec.rb b/spec/workers/update_merge_requests_worker_spec.rb
index fd5a6ee20b54d5..2f9972578ba461 100644
--- a/spec/workers/update_merge_requests_worker_spec.rb
+++ b/spec/workers/update_merge_requests_worker_spec.rb
@@ -24,7 +24,7 @@
       subject
     end
 
-    context 'when push options are passed' do
+    context 'when push options are passed as Hash' do
       let(:extra_params) { { 'push_options' => { 'ci' => { 'skip' => true } } } }
 
       subject { worker.perform(project.id, user.id, oldrev, newrev, ref, extra_params) }
@@ -39,5 +39,37 @@
         subject
       end
     end
+
+    context 'when push options are passed as JSON string' do
+      let(:extra_params) { { 'push_options' => { 'ci' => { 'skip' => true } } }.to_json }
+
+      subject { worker.perform(project.id, user.id, oldrev, newrev, ref, extra_params) }
+
+      it 'executes MergeRequests::RefreshService with expected values' do
+        expect_next_instance_of(MergeRequests::RefreshService, project: project, current_user: user) do |service|
+          expect(service)
+            .to receive(:execute)
+            .with(oldrev, newrev, ref, push_options: { ci: { skip: true } })
+        end
+
+        subject
+      end
+    end
+
+    context 'when push options is "null"' do
+      let(:extra_params) { "null" }
+
+      subject { worker.perform(project.id, user.id, oldrev, newrev, ref, extra_params) }
+
+      it 'executes MergeRequests::RefreshService with expected values' do
+        expect_next_instance_of(MergeRequests::RefreshService, project: project, current_user: user) do |service|
+          expect(service)
+            .to receive(:execute)
+            .with(oldrev, newrev, ref, push_options: nil)
+        end
+
+        subject
+      end
+    end
   end
 end
-- 
GitLab


From 19fb27beeeac9f2c3786b4367dd7c24a71467e74 Mon Sep 17 00:00:00 2001
From: Fabio Pitino <fpitino@gitlab.com>
Date: Fri, 20 May 2022 12:04:17 +0100
Subject: [PATCH 5/8] Move push_options to service params

---
 app/services/merge_requests/base_service.rb   |  6 ++-
 .../merge_requests/create_pipeline_service.rb |  8 +--
 .../merge_requests/refresh_service.rb         |  5 +-
 .../merge_requests/create_pipeline_worker.rb  |  8 +--
 app/workers/update_merge_requests_worker.rb   |  8 +--
 .../merge_requests/create_pipeline_service.rb |  8 +--
 .../create_pipeline_service_spec.rb           |  2 +-
 .../merge_requests/refresh_service_spec.rb    |  4 +-
 .../create_pipeline_worker_spec.rb            | 52 ++++---------------
 .../update_merge_requests_worker_spec.rb      | 46 ++++------------
 10 files changed, 46 insertions(+), 101 deletions(-)

diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index cb6b46c0aa91f0..d3be3c039165c5 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/app/services/merge_requests/base_service.rb
@@ -183,12 +183,14 @@ def create_reviewer_note(merge_request, old_reviewers)
         merge_request, merge_request.project, current_user, old_reviewers)
     end
 
-    def create_pipeline_for(merge_request, user, async: false, push_options: nil)
+    def create_pipeline_for(merge_request, user, async: false)
       if async
         # TODO: pass push_options to worker
         MergeRequests::CreatePipelineWorker.perform_async(project.id, user.id, merge_request.id)
       else
-        MergeRequests::CreatePipelineService.new(project: project, current_user: user).execute(merge_request, push_options: push_options)
+        MergeRequests::CreatePipelineService
+          .new(project: project, current_user: user, params: { push_options: params[:push_options] })
+          .execute(merge_request)
       end
     end
 
diff --git a/app/services/merge_requests/create_pipeline_service.rb b/app/services/merge_requests/create_pipeline_service.rb
index a3b0eec6d24ebe..37c734613e791d 100644
--- a/app/services/merge_requests/create_pipeline_service.rb
+++ b/app/services/merge_requests/create_pipeline_service.rb
@@ -2,18 +2,18 @@
 
 module MergeRequests
   class CreatePipelineService < MergeRequests::BaseService
-    def execute(merge_request, push_options: nil)
+    def execute(merge_request)
       return cannot_create_pipeline_error unless can_create_pipeline_for?(merge_request)
 
-      create_detached_merge_request_pipeline(merge_request, push_options)
+      create_detached_merge_request_pipeline(merge_request)
     end
 
-    def create_detached_merge_request_pipeline(merge_request, push_options)
+    def create_detached_merge_request_pipeline(merge_request)
       Ci::CreatePipelineService
         .new(pipeline_project(merge_request),
           current_user,
           ref: pipeline_ref_for_detached_merge_request_pipeline(merge_request),
-          push_options: push_options)
+          push_options: params[:push_options])
         .execute(:merge_request_event, merge_request: merge_request)
     end
 
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index 3a6d9794a049a9..f7a0f90b95f2bc 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/app/services/merge_requests/refresh_service.rb
@@ -5,9 +5,8 @@ class RefreshService < MergeRequests::BaseService
     include Gitlab::Utils::StrongMemoize
     attr_reader :push
 
-    def execute(oldrev, newrev, ref, push_options: nil)
+    def execute(oldrev, newrev, ref)
       @push = Gitlab::Git::Push.new(@project, oldrev, newrev, ref)
-      @push_options = push_options
       return true unless @push.branch_push?
 
       refresh_merge_requests!
@@ -164,7 +163,7 @@ def outdate_service
     end
 
     def refresh_pipelines_on_merge_requests(merge_request)
-      create_pipeline_for(merge_request, current_user, async: true, push_options: @push_options)
+      create_pipeline_for(merge_request, current_user, async: true)
     end
 
     def abort_auto_merges(merge_request)
diff --git a/app/workers/merge_requests/create_pipeline_worker.rb b/app/workers/merge_requests/create_pipeline_worker.rb
index 07fbe2ddb7242c..b40408cf647af2 100644
--- a/app/workers/merge_requests/create_pipeline_worker.rb
+++ b/app/workers/merge_requests/create_pipeline_worker.rb
@@ -15,7 +15,7 @@ class CreatePipelineWorker
     worker_resource_boundary :cpu
     idempotent!
 
-    def perform(project_id, user_id, merge_request_id, params = nil)
+    def perform(project_id, user_id, merge_request_id, params = {})
       project = Project.find_by_id(project_id)
       return unless project
 
@@ -25,11 +25,11 @@ def perform(project_id, user_id, merge_request_id, params = nil)
       merge_request = MergeRequest.find_by_id(merge_request_id)
       return unless merge_request
 
-      params = parse_json_arg(params)
+      push_options = params.with_indifferent_access[:push_options]
 
       MergeRequests::CreatePipelineService
-        .new(project: project, current_user: user)
-        .execute(merge_request, push_options: params[:push_options])
+        .new(project: project, current_user: user, params: { push_options: push_options })
+        .execute(merge_request)
 
       merge_request.update_head_pipeline
     end
diff --git a/app/workers/update_merge_requests_worker.rb b/app/workers/update_merge_requests_worker.rb
index c43eeab468f758..eb69c0eaba68a5 100644
--- a/app/workers/update_merge_requests_worker.rb
+++ b/app/workers/update_merge_requests_worker.rb
@@ -13,17 +13,17 @@ class UpdateMergeRequestsWorker # rubocop:disable Scalability/IdempotentWorker
   weight 3
   loggable_arguments 2, 3, 4
 
-  def perform(project_id, user_id, oldrev, newrev, ref, params = nil)
+  def perform(project_id, user_id, oldrev, newrev, ref, params = {})
     project = Project.find_by_id(project_id)
     return unless project
 
     user = User.find_by_id(user_id)
     return unless user
 
-    params = parse_json_arg(params)
+    push_options = params.with_indifferent_access[:push_options]
 
     MergeRequests::RefreshService
-      .new(project: project, current_user: user)
-      .execute(oldrev, newrev, ref, push_options: params[:push_options])
+      .new(project: project, current_user: user, params: { push_options: push_options })
+      .execute(oldrev, newrev, ref)
   end
 end
diff --git a/ee/app/services/ee/merge_requests/create_pipeline_service.rb b/ee/app/services/ee/merge_requests/create_pipeline_service.rb
index ed48941d1a574c..1d6324aadd107b 100644
--- a/ee/app/services/ee/merge_requests/create_pipeline_service.rb
+++ b/ee/app/services/ee/merge_requests/create_pipeline_service.rb
@@ -6,15 +6,15 @@ module CreatePipelineService
       extend ::Gitlab::Utils::Override
 
       override :execute
-      def execute(merge_request, push_options: nil)
-        response = create_merged_result_pipeline_for(merge_request, push_options)
+      def execute(merge_request)
+        response = create_merged_result_pipeline_for(merge_request)
 
         return response if response.success?
 
         super
       end
 
-      def create_merged_result_pipeline_for(merge_request, push_options)
+      def create_merged_result_pipeline_for(merge_request)
         return cannot_create_pipeline_error unless can_create_merged_result_pipeline_for?(merge_request)
 
         result = ::MergeRequests::MergeabilityCheckService.new(merge_request).execute(recheck: true)
@@ -28,7 +28,7 @@ def create_merged_result_pipeline_for(merge_request, push_options)
               checkout_sha: ref_payload[:commit_id],
               target_sha: ref_payload[:target_id],
               source_sha: ref_payload[:source_id],
-              push_options: push_options)
+              push_options: params[:push_options])
             .execute(:merge_request_event, merge_request: merge_request)
         else
           cannot_create_pipeline_error
diff --git a/spec/services/merge_requests/create_pipeline_service_spec.rb b/spec/services/merge_requests/create_pipeline_service_spec.rb
index 0c1087a1f99c6d..08ad05b54da9bf 100644
--- a/spec/services/merge_requests/create_pipeline_service_spec.rb
+++ b/spec/services/merge_requests/create_pipeline_service_spec.rb
@@ -51,7 +51,7 @@
     end
 
     context 'when push options contain ci.skip' do
-      let(:response) { service.execute(merge_request, push_options: { ci: { skip: true } }) }
+      let(:params) { { push_options: { ci: { skip: true } } } }
 
       it 'creates a skipped pipeline' do
         expect { response }.to change { Ci::Pipeline.count }.by(1)
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index 3f697562725cb0..7560f0d5e240ba 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/spec/services/merge_requests/refresh_service_spec.rb
@@ -229,7 +229,9 @@
         end
 
         context 'when "push_options: nil" is passed' do
-          subject { service.new(project: project, current_user: @user).execute(@oldrev, @newrev, ref, push_options: nil) }
+          let(:service_instance) { service.new(project: project, current_user: @user, params: { push_options: nil }) }
+
+          subject { service_instance.execute(@oldrev, @newrev, ref) }
 
           it 'creates a detached merge request pipeline with commits' do
             expect { subject }
diff --git a/spec/workers/merge_requests/create_pipeline_worker_spec.rb b/spec/workers/merge_requests/create_pipeline_worker_spec.rb
index b8a9551de545fb..441d765221964c 100644
--- a/spec/workers/merge_requests/create_pipeline_worker_spec.rb
+++ b/spec/workers/merge_requests/create_pipeline_worker_spec.rb
@@ -14,8 +14,11 @@
     context 'when the objects exist' do
       it 'calls the merge request create pipeline service and calls update head pipeline' do
         aggregate_failures do
-          expect_next_instance_of(MergeRequests::CreatePipelineService, project: project, current_user: user) do |service|
-            expect(service).to receive(:execute).with(merge_request, push_options: nil)
+          expect_next_instance_of(MergeRequests::CreatePipelineService,
+            project: project,
+            current_user: user,
+            params: { push_options: nil }) do |service|
+            expect(service).to receive(:execute).with(merge_request)
           end
 
           expect(MergeRequest).to receive(:find_by_id).with(merge_request.id).and_return(merge_request)
@@ -32,46 +35,11 @@
 
         it 'calls the merge request create pipeline service and calls update head pipeline' do
           aggregate_failures do
-            expect_next_instance_of(MergeRequests::CreatePipelineService, project: project, current_user: user) do |service|
-              expect(service).to receive(:execute).with(merge_request, push_options: { ci: { skip: true } })
-            end
-
-            expect(MergeRequest).to receive(:find_by_id).with(merge_request.id).and_return(merge_request)
-            expect(merge_request).to receive(:update_head_pipeline)
-
-            subject
-          end
-        end
-      end
-
-      context 'when push options are passed as JSON string to the worker' do
-        let(:extra_params) { { 'push_options' => { 'ci' => { 'skip' => true } } }.to_json }
-
-        subject { worker.perform(project.id, user.id, merge_request.id, extra_params) }
-
-        it 'calls the merge request create pipeline service and calls update head pipeline' do
-          aggregate_failures do
-            expect_next_instance_of(MergeRequests::CreatePipelineService, project: project, current_user: user) do |service|
-              expect(service).to receive(:execute).with(merge_request, push_options: { ci: { skip: true } })
-            end
-
-            expect(MergeRequest).to receive(:find_by_id).with(merge_request.id).and_return(merge_request)
-            expect(merge_request).to receive(:update_head_pipeline)
-
-            subject
-          end
-        end
-      end
-
-      context 'when extra parameters are omitted and defaulted to "null"' do
-        let(:extra_params) { "null" }
-
-        subject { worker.perform(project.id, user.id, merge_request.id, extra_params) }
-
-        it 'calls the merge request create pipeline service and calls update head pipeline' do
-          aggregate_failures do
-            expect_next_instance_of(MergeRequests::CreatePipelineService, project: project, current_user: user) do |service|
-              expect(service).to receive(:execute).with(merge_request, push_options: nil)
+            expect_next_instance_of(MergeRequests::CreatePipelineService,
+              project: project,
+              current_user: user,
+              params: { push_options: { ci: { skip: true } } }) do |service|
+              expect(service).to receive(:execute).with(merge_request)
             end
 
             expect(MergeRequest).to receive(:find_by_id).with(merge_request.id).and_return(merge_request)
diff --git a/spec/workers/update_merge_requests_worker_spec.rb b/spec/workers/update_merge_requests_worker_spec.rb
index 2f9972578ba461..64fcc2bd388040 100644
--- a/spec/workers/update_merge_requests_worker_spec.rb
+++ b/spec/workers/update_merge_requests_worker_spec.rb
@@ -15,10 +15,13 @@
     subject { worker.perform(project.id, user.id, oldrev, newrev, ref) }
 
     it 'executes MergeRequests::RefreshService with expected values' do
-      expect_next_instance_of(MergeRequests::RefreshService, project: project, current_user: user) do |service|
+      expect_next_instance_of(MergeRequests::RefreshService,
+        project: project,
+        current_user: user,
+        params: { push_options: nil }) do |service|
         expect(service)
           .to receive(:execute)
-          .with(oldrev, newrev, ref, push_options: nil)
+          .with(oldrev, newrev, ref)
       end
 
       subject
@@ -30,42 +33,13 @@
       subject { worker.perform(project.id, user.id, oldrev, newrev, ref, extra_params) }
 
       it 'executes MergeRequests::RefreshService with expected values' do
-        expect_next_instance_of(MergeRequests::RefreshService, project: project, current_user: user) do |service|
+        expect_next_instance_of(MergeRequests::RefreshService,
+          project: project,
+          current_user: user,
+          params: { push_options: { ci: { skip: true } } }) do |service|
           expect(service)
             .to receive(:execute)
-            .with(oldrev, newrev, ref, push_options: { ci: { skip: true } })
-        end
-
-        subject
-      end
-    end
-
-    context 'when push options are passed as JSON string' do
-      let(:extra_params) { { 'push_options' => { 'ci' => { 'skip' => true } } }.to_json }
-
-      subject { worker.perform(project.id, user.id, oldrev, newrev, ref, extra_params) }
-
-      it 'executes MergeRequests::RefreshService with expected values' do
-        expect_next_instance_of(MergeRequests::RefreshService, project: project, current_user: user) do |service|
-          expect(service)
-            .to receive(:execute)
-            .with(oldrev, newrev, ref, push_options: { ci: { skip: true } })
-        end
-
-        subject
-      end
-    end
-
-    context 'when push options is "null"' do
-      let(:extra_params) { "null" }
-
-      subject { worker.perform(project.id, user.id, oldrev, newrev, ref, extra_params) }
-
-      it 'executes MergeRequests::RefreshService with expected values' do
-        expect_next_instance_of(MergeRequests::RefreshService, project: project, current_user: user) do |service|
-          expect(service)
-            .to receive(:execute)
-            .with(oldrev, newrev, ref, push_options: nil)
+            .with(oldrev, newrev, ref)
         end
 
         subject
-- 
GitLab


From 2fe05bb06fa9a03c7b3d62423ccc088fb423536a Mon Sep 17 00:00:00 2001
From: Fabio Pitino <fpitino@gitlab.com>
Date: Fri, 20 May 2022 12:05:53 +0100
Subject: [PATCH 6/8] Remove parse_json_arg in favor of with_indifferent_access

---
 app/workers/concerns/application_worker.rb    | 20 -----------
 .../concerns/application_worker_spec.rb       | 36 -------------------
 2 files changed, 56 deletions(-)

diff --git a/app/workers/concerns/application_worker.rb b/app/workers/concerns/application_worker.rb
index 71682c7a7edfb9..c1fec4f01966da 100644
--- a/app/workers/concerns/application_worker.rb
+++ b/app/workers/concerns/application_worker.rb
@@ -216,24 +216,4 @@ def process_schedule_at_for_batch(schedule_at, index)
       schedule_at
     end
   end
-
-  # In Sidekiq 7.0, Hash arguments are not considered safe for serialization.
-  # We should dump the hash parameter to JSON when calling `perform_async` and
-  # we can use this method to convert it back.
-  # This method supports backward compatibility to allow converting existing
-  # Hash arguments to JSON arguments.
-  # For more info see https://github.com/mperham/sidekiq/wiki/Best-Practices
-  def parse_json_arg(params)
-    case params
-    when Hash then params.deep_symbolize_keys!
-    when nil  then {}
-    when String
-      result = Gitlab::Json.parse(params)
-      result.present? ? result.deep_symbolize_keys! : {}
-    else
-      raise ArgumentError, "Argument type not supported, got: #{params}"
-    end
-  rescue JSON::ParserError
-    raise ArgumentError, "Argument cannot be parsed, got: #{params}"
-  end
 end
diff --git a/spec/workers/concerns/application_worker_spec.rb b/spec/workers/concerns/application_worker_spec.rb
index 6a8f9646b05cec..707fa0c9c78402 100644
--- a/spec/workers/concerns/application_worker_spec.rb
+++ b/spec/workers/concerns/application_worker_spec.rb
@@ -537,40 +537,4 @@ def self.name
       end
     end
   end
-
-  describe '#parse_json_arg' do
-    using RSpec::Parameterized::TableSyntax
-
-    subject { instance.parse_json_arg(param) }
-
-    where(:param, :result) do
-      nil                                   | {}
-      "null"                                | {}
-      { foo: 123 }                          | { foo: 123 } # rubocop: disable Lint/BinaryOperatorWithIdenticalOperands
-      { 'foo' => 123 }                      | { foo: 123 }
-      { 'foo' => { 'bar' => 123 } }         | { foo: { bar: 123 } }
-      { test: 123 }.to_json                 | { test: 123 }
-      { 'foo' => { 'bar' => 123 } }.to_json | { foo: { bar: 123 } }
-    end
-
-    with_them do
-      it { is_expected.to eq(result) }
-    end
-
-    context 'when parameter type is not supported' do
-      let(:param) { 123 }
-
-      it 'raises an error' do
-        expect { subject }.to raise_error(ArgumentError, 'Argument type not supported, got: 123')
-      end
-    end
-
-    context 'when parameter is not a JSON string' do
-      let(:param) { 'test' }
-
-      it 'raises an error' do
-        expect { subject }.to raise_error(ArgumentError, 'Argument cannot be parsed, got: test')
-      end
-    end
-  end
 end
-- 
GitLab


From ed5f2daee52bb4007b5ea62e376f5517b2e7fee4 Mon Sep 17 00:00:00 2001
From: Fabio Pitino <fpitino@gitlab.com>
Date: Fri, 20 May 2022 12:46:16 +0100
Subject: [PATCH 7/8] Fix failing spec

---
 .../ee/merge_requests/create_pipeline_service_spec.rb        | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb b/ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb
index fae71f4b71c262..c91b69ad1c8369 100644
--- a/ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb
+++ b/ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb
@@ -8,9 +8,10 @@
   describe '#execute' do
     subject { service.execute(merge_request) }
 
-    let(:service) { described_class.new(project: source_project, current_user: user) }
+    let(:service) { described_class.new(project: source_project, current_user: user, params: params) }
     let(:project) { create(:project, :repository) }
     let(:user) { create(:user) }
+    let(:params) { {} }
     let(:title) { 'Awesome merge request' }
     let(:merge_pipelines_enabled) { true }
     let(:merge_pipelines_license) { true }
@@ -73,7 +74,7 @@
     end
 
     context 'when push options contain ci.skip' do
-      subject { service.execute(merge_request, push_options: { ci: { skip: true } }) }
+      let(:params) { { push_options: { ci: { skip: true } } } }
 
       it 'creates a skipped pipeline' do
         subject
-- 
GitLab


From c6c2206e7eebef7c9145c50f07211821809cfe96 Mon Sep 17 00:00:00 2001
From: Marius Bobin <mbobin@gitlab.com>
Date: Mon, 23 May 2022 13:59:54 +0000
Subject: [PATCH 8/8] Apply 1 suggestion(s) to 1 file(s)

---
 app/services/merge_requests/base_service.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index d3be3c039165c5..74a8d9d6732e21 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/app/services/merge_requests/base_service.rb
@@ -189,7 +189,7 @@ def create_pipeline_for(merge_request, user, async: false)
         MergeRequests::CreatePipelineWorker.perform_async(project.id, user.id, merge_request.id)
       else
         MergeRequests::CreatePipelineService
-          .new(project: project, current_user: user, params: { push_options: params[:push_options] })
+          .new(project: project, current_user: user, params: params.slice(:push_options))
           .execute(merge_request)
       end
     end
-- 
GitLab