From f66b0a05acbff3c9ae903c2af53969d71aded2ed Mon Sep 17 00:00:00 2001
From: drew <dscimino@gmail.com>
Date: Thu, 21 Sep 2023 21:34:50 -0400
Subject: [PATCH 1/8] Verify our creating of merged MergeTrain::Car records
 with a spec

---
 ee/spec/models/merge_trains/car_spec.rb | 26 +++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/ee/spec/models/merge_trains/car_spec.rb b/ee/spec/models/merge_trains/car_spec.rb
index c963ea0e99a714..63cebdad57dedc 100644
--- a/ee/spec/models/merge_trains/car_spec.rb
+++ b/ee/spec/models/merge_trains/car_spec.rb
@@ -23,6 +23,32 @@
     let_it_be(:merging_car) { create(:merge_train_car, :merging) }
   end
 
+  describe '.create' do
+    let(:merge_request) { create(:merge_request, :merged) }
+
+    let(:base_attributes) do
+      {
+        user: merge_request.author,
+        merge_request: merge_request,
+        target_project: merge_request.target_project,
+        target_branch: merge_request.target_branch
+      }
+    end
+
+    subject { described_class.create!(attributes) }
+
+    context 'with merged_at and a completed status' do
+      let(:attributes) { base_attributes.merge(merged_at: Time.current, status: 1) }
+
+      it 'creates a completed car with no pipeline', :aggregate_failures do
+        expect(subject).to be_persisted
+        expect(subject).to be_merged
+        expect(subject.pipeline).to be_nil
+        expect(subject.merged_at).to be_present
+      end
+    end
+  end
+
   describe '.active' do
     subject { described_class.active }
 
-- 
GitLab


From c6905f8c43fb9ad4c16b34e11cc40697f6a12248 Mon Sep 17 00:00:00 2001
From: drew <dscimino@gmail.com>
Date: Wed, 20 Sep 2023 11:48:32 -0400
Subject: [PATCH 2/8] Create completed merge train cars when merging with no
 refresh

By creating a new car (with no CI/pipeline) in a "merged" state,
cars currently on the train will not detect new changes, and in
turn will not restart themselves.
---
 app/services/merge_requests/merge_service.rb  |  2 +
 .../ee/merge_requests/merge_service.rb        | 28 ++++++++++++
 ee/lib/ee/api/merge_requests.rb               | 10 +++++
 ee/spec/requests/api/merge_requests_spec.rb   | 14 +++++-
 .../merge_requests/merge_service_spec.rb      | 45 +++++++++++++++++++
 lib/api/merge_requests.rb                     | 15 ++++++-
 6 files changed, 112 insertions(+), 2 deletions(-)
 create mode 100644 ee/app/services/ee/merge_requests/merge_service.rb

diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index 29aba3c86795eb..89e5920a4fb4e0 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -181,3 +181,5 @@ def exclusive_lease(merge_request_id)
     end
   end
 end
+
+MergeRequests::MergeService.prepend_mod
diff --git a/ee/app/services/ee/merge_requests/merge_service.rb b/ee/app/services/ee/merge_requests/merge_service.rb
new file mode 100644
index 00000000000000..9cb8a919316fdb
--- /dev/null
+++ b/ee/app/services/ee/merge_requests/merge_service.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+module EE
+  module MergeRequests
+    module MergeService
+      def after_merge
+        create_completed_car if skipping_active_merge_train?
+
+        super
+      end
+
+      def skipping_active_merge_train?
+        @options[:skip_merge_train] && # rubocop:disable Gitlab/ModuleWithInstanceVariables
+          project.merge_trains_skip_train_allowed?
+      end
+
+      def create_completed_car
+        merge_request.build_merge_train_car(
+          user: current_user,
+          target_project: merge_request.target_project,
+          target_branch: merge_request.target_branch,
+          merged_at: Time.current,
+          status: 1
+        )
+      end
+    end
+  end
+end
diff --git a/ee/lib/ee/api/merge_requests.rb b/ee/lib/ee/api/merge_requests.rb
index aa03f1ebb8b77b..f642579d30e91e 100644
--- a/ee/lib/ee/api/merge_requests.rb
+++ b/ee/lib/ee/api/merge_requests.rb
@@ -28,6 +28,16 @@ module MergeRequests
             usernames'
             mutually_exclusive :approved_by_ids, :approved_by_usernames
           end
+
+          params :optional_merge_params_ee do
+            optional :skip_merge_train,
+              type: Grape::API::Boolean,
+              desc: 'If `true` skips train restart when merging immediately in a merge train configured project.'
+          end
+
+          def ci_params
+            super.merge(skip_merge_train: params[:skip_merge_train])
+          end
         end
 
         resource :projects, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
diff --git a/ee/spec/requests/api/merge_requests_spec.rb b/ee/spec/requests/api/merge_requests_spec.rb
index 059dfc939f2469..ddf89a833043e3 100644
--- a/ee/spec/requests/api/merge_requests_spec.rb
+++ b/ee/spec/requests/api/merge_requests_spec.rb
@@ -343,7 +343,7 @@ def create_merge_request(args)
     end
   end
 
-  describe "PUT /projects/:id/merge_requests/:merge_request_iid/merge" do
+  describe 'PUT /projects/:id/merge_requests/:merge_request_iid/merge' do
     it 'returns 405 if merge request was not approved' do
       project.add_developer(create(:user))
       project.update!(approvals_before_merge: 1)
@@ -363,6 +363,18 @@ def create_merge_request(args)
 
       expect(response).to have_gitlab_http_status(:ok)
     end
+
+    context 'when the requests asks to skip the train' do
+      let(:skip_params) { { skip_merge_train: true } }
+
+      it 'forwards the param to the MergeRequests::MergeService', :aggregate_failures do
+        expect_next_instance_of(::MergeRequests::MergeService) do |instance|
+          expect(instance).to receive(:execute).with(merge_request, skip_params)
+        end
+
+        put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user), params: skip_params
+      end
+    end
   end
 
   describe "DELETE /projects/:id/merge_requests/:merge_request_iid" do
diff --git a/ee/spec/services/merge_requests/merge_service_spec.rb b/ee/spec/services/merge_requests/merge_service_spec.rb
index 7e08e8c9e3e880..9cdd6aee691cc4 100644
--- a/ee/spec/services/merge_requests/merge_service_spec.rb
+++ b/ee/spec/services/merge_requests/merge_service_spec.rb
@@ -164,6 +164,51 @@
         end
       end
     end
+
+    context 'when skip_merge_train is true', :aggregate_failures do
+      subject { service.execute(merge_request, { skip_merge_train: true }) }
+
+      let(:newest_car) { MergeTrains::Car.last }
+      let(:train)      { MergeTrains::Train.new(merge_request.project_id, merge_request.target_branch) }
+
+      context 'with merge_trains_skip_merge_train_allowed ProjectCiCdSetting enabled' do
+        before do
+          project.ci_cd_settings.update!(merge_trains_skip_train_allowed: true)
+        end
+
+        it 'creates a new merged Car record and adds the revision to the train history' do
+          expect { subject }.to change { MergeTrains::Car.count }.by(1)
+          expect(newest_car.merge_request).to eq(merge_request)
+          expect(newest_car).to be_merged
+
+          expect(train.sha_exists_in_history?(merge_request.merge_commit_sha)).to be_truthy
+        end
+      end
+
+      context 'with merge_trains_skip_merge_train_allowed ProjectCiCdSetting disabled' do
+        before do
+          project.ci_cd_settings.update!(merge_trains_skip_train_allowed: false)
+        end
+
+        it 'does not create any new Car record or add to the train revision history' do
+          expect { subject }.not_to change { MergeTrains::Car.count }
+          expect(merge_request).to be_merged
+          expect(train.sha_exists_in_history?(merge_request.merge_commit_sha)).to be_falsy
+        end
+      end
+
+      context 'when the merge_train_skip_trains feature flag is disabled' do
+        before do
+          stub_feature_flags(merge_trains_skip_train: false)
+        end
+
+        it 'does not create any new Car record or add to the train revision history' do
+          expect { subject }.not_to change { MergeTrains::Car.count }
+          expect(merge_request).to be_merged
+          expect(train.sha_exists_in_history?(merge_request.merge_commit_sha)).to be_falsy
+        end
+      end
+    end
   end
 
   it_behaves_like 'merge validation hooks', persisted: true
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index 11f25599be5c07..c4e1cd2d55ed4c 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -31,8 +31,15 @@ class MergeRequests < ::API::Base
       params :optional_params_ee do
       end
 
+      params :optional_merge_params_ee do
+      end
+
       params :optional_merge_requests_search_params do
       end
+
+      def ci_params
+        {}
+      end
     end
 
     def self.update_params_at_least_one_of
@@ -232,6 +239,10 @@ def batch_process_mergeability_checks(merge_requests)
 
           use :optional_params_ee
         end
+
+        params :optional_merge_params do
+          use :optional_merge_params_ee
+        end
       end
 
       desc 'List project merge requests' do
@@ -642,6 +653,8 @@ def batch_process_mergeability_checks(merge_requests)
                                                 desc: 'If `true`, the merge request is merged when the pipeline succeeds.'
         optional :sha, type: String, desc: 'If present, then this SHA must match the HEAD of the source branch, otherwise the merge fails.'
         optional :squash, type: Grape::API::Boolean, desc: 'If `true`, the commits are squashed into a single commit on merge.'
+
+        use :optional_merge_params
       end
       put ':id/merge_requests/:merge_request_iid/merge', feature_category: :code_review_workflow, urgency: :low do
         Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/4796')
@@ -675,7 +688,7 @@ def batch_process_mergeability_checks(merge_requests)
         if immediately_mergeable
           ::MergeRequests::MergeService
             .new(project: merge_request.target_project, current_user: current_user, params: merge_params)
-            .execute(merge_request)
+            .execute(merge_request, ci_params)
         elsif automatically_mergeable
           AutoMergeService.new(merge_request.target_project, current_user, merge_params)
             .execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
-- 
GitLab


From 2b51874684918692e7b66f707ef83b7576aa9878 Mon Sep 17 00:00:00 2001
From: drew <dscimino@gmail.com>
Date: Mon, 25 Sep 2023 14:47:36 -0400
Subject: [PATCH 3/8] Move service class param to initialization

We prefer to pass params into #initialize, not #execute

https://docs.gitlab.com/ee/development/reusing_abstractions.html#service-classes
---
 ee/app/services/ee/merge_requests/merge_service.rb |  2 +-
 ee/spec/requests/api/merge_requests_spec.rb        | 14 +++++++++-----
 .../services/merge_requests/merge_service_spec.rb  |  3 ++-
 lib/api/merge_requests.rb                          |  4 ++--
 4 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/ee/app/services/ee/merge_requests/merge_service.rb b/ee/app/services/ee/merge_requests/merge_service.rb
index 9cb8a919316fdb..0f2faf50f4a544 100644
--- a/ee/app/services/ee/merge_requests/merge_service.rb
+++ b/ee/app/services/ee/merge_requests/merge_service.rb
@@ -10,7 +10,7 @@ def after_merge
       end
 
       def skipping_active_merge_train?
-        @options[:skip_merge_train] && # rubocop:disable Gitlab/ModuleWithInstanceVariables
+        params[:skip_merge_train] &&
           project.merge_trains_skip_train_allowed?
       end
 
diff --git a/ee/spec/requests/api/merge_requests_spec.rb b/ee/spec/requests/api/merge_requests_spec.rb
index ddf89a833043e3..24d1cb613c9e07 100644
--- a/ee/spec/requests/api/merge_requests_spec.rb
+++ b/ee/spec/requests/api/merge_requests_spec.rb
@@ -365,15 +365,19 @@ def create_merge_request(args)
     end
 
     context 'when the requests asks to skip the train' do
-      let(:skip_params) { { skip_merge_train: true } }
+      before do
+        project.ci_cd_settings.update!(merge_trains_skip_train_allowed: true)
+      end
 
-      it 'forwards the param to the MergeRequests::MergeService', :aggregate_failures do
-        expect_next_instance_of(::MergeRequests::MergeService) do |instance|
-          expect(instance).to receive(:execute).with(merge_request, skip_params)
-        end
+      let(:skip_params) { { skip_merge_train: true } }
 
+      let(:request) do
         put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user), params: skip_params
       end
+
+      it 'creates a new merged train car to represent the merged MR', :aggregate_failures do
+        expect { request }.to change { MergeTrains::Car.count }.by(1)
+      end
     end
   end
 
diff --git a/ee/spec/services/merge_requests/merge_service_spec.rb b/ee/spec/services/merge_requests/merge_service_spec.rb
index 9cdd6aee691cc4..b60baac590598c 100644
--- a/ee/spec/services/merge_requests/merge_service_spec.rb
+++ b/ee/spec/services/merge_requests/merge_service_spec.rb
@@ -166,10 +166,11 @@
     end
 
     context 'when skip_merge_train is true', :aggregate_failures do
-      subject { service.execute(merge_request, { skip_merge_train: true }) }
+      subject { service.execute(merge_request) }
 
       let(:newest_car) { MergeTrains::Car.last }
       let(:train)      { MergeTrains::Train.new(merge_request.project_id, merge_request.target_branch) }
+      let(:params)     { { sha: merge_request.diff_head_sha, commit_message: 'A message', skip_merge_train: true } }
 
       context 'with merge_trains_skip_merge_train_allowed ProjectCiCdSetting enabled' do
         before do
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index c4e1cd2d55ed4c..b8285bbd109d17 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -683,12 +683,12 @@ def batch_process_mergeability_checks(merge_requests)
           squash_commit_message: params[:squash_commit_message],
           should_remove_source_branch: params[:should_remove_source_branch],
           sha: params[:sha] || merge_request.diff_head_sha
-        ).compact
+        ).merge(ci_params).compact
 
         if immediately_mergeable
           ::MergeRequests::MergeService
             .new(project: merge_request.target_project, current_user: current_user, params: merge_params)
-            .execute(merge_request, ci_params)
+            .execute(merge_request)
         elsif automatically_mergeable
           AutoMergeService.new(merge_request.target_project, current_user, merge_params)
             .execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
-- 
GitLab


From 43d1ef0d8558b36c22c4f08b861d0b9001c64d9b Mon Sep 17 00:00:00 2001
From: drew <dscimino@gmail.com>
Date: Tue, 26 Sep 2023 12:11:46 -0400
Subject: [PATCH 4/8] Use labeled status value for readability

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

diff --git a/ee/app/services/ee/merge_requests/merge_service.rb b/ee/app/services/ee/merge_requests/merge_service.rb
index 0f2faf50f4a544..08336b2cc41e09 100644
--- a/ee/app/services/ee/merge_requests/merge_service.rb
+++ b/ee/app/services/ee/merge_requests/merge_service.rb
@@ -20,7 +20,7 @@ def create_completed_car
           target_project: merge_request.target_project,
           target_branch: merge_request.target_branch,
           merged_at: Time.current,
-          status: 1
+          status: MergeTrains::Car.state_machine.states[:merged].value
         )
       end
     end
-- 
GitLab


From c667b67bb75bc9429f1df4bedadb8c76edc0d554 Mon Sep 17 00:00:00 2001
From: drew <dscimino@gmail.com>
Date: Tue, 26 Sep 2023 14:04:26 -0400
Subject: [PATCH 5/8] Create a new MergeTrains::Car status for MRs that skip
 the train

---
 ee/app/models/merge_trains/car.rb                     |  3 ++-
 ee/app/services/ee/merge_requests/merge_service.rb    |  2 +-
 ee/spec/models/merge_trains/car_spec.rb               | 11 ++++++++---
 ee/spec/services/merge_requests/merge_service_spec.rb |  2 +-
 4 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/ee/app/models/merge_trains/car.rb b/ee/app/models/merge_trains/car.rb
index 5b62299a8a64d9..30ae8c529d4c6f 100644
--- a/ee/app/models/merge_trains/car.rb
+++ b/ee/app/models/merge_trains/car.rb
@@ -10,7 +10,7 @@ class Car < ApplicationRecord
     self.table_name = 'merge_trains'
 
     ACTIVE_STATUSES = %w[idle stale fresh].freeze
-    COMPLETE_STATUSES = %w[merged merging].freeze
+    COMPLETE_STATUSES = %w[merged merging skip_merged].freeze
 
     belongs_to :target_project, class_name: "Project"
     belongs_to :merge_request, inverse_of: :merge_train_car
@@ -73,6 +73,7 @@ class Car < ApplicationRecord
       state :stale, value: 2
       state :fresh, value: 3
       state :merging, value: 4
+      state :skip_merged, value: 5
     end
 
     scope :active, -> { with_status(*ACTIVE_STATUSES) }
diff --git a/ee/app/services/ee/merge_requests/merge_service.rb b/ee/app/services/ee/merge_requests/merge_service.rb
index 08336b2cc41e09..21c2df00edd598 100644
--- a/ee/app/services/ee/merge_requests/merge_service.rb
+++ b/ee/app/services/ee/merge_requests/merge_service.rb
@@ -20,7 +20,7 @@ def create_completed_car
           target_project: merge_request.target_project,
           target_branch: merge_request.target_branch,
           merged_at: Time.current,
-          status: MergeTrains::Car.state_machine.states[:merged].value
+          status: MergeTrains::Car.state_machine.states[:skip_merged].value
         )
       end
     end
diff --git a/ee/spec/models/merge_trains/car_spec.rb b/ee/spec/models/merge_trains/car_spec.rb
index 63cebdad57dedc..c802d662022365 100644
--- a/ee/spec/models/merge_trains/car_spec.rb
+++ b/ee/spec/models/merge_trains/car_spec.rb
@@ -37,12 +37,17 @@
 
     subject { described_class.create!(attributes) }
 
-    context 'with merged_at and a completed status' do
-      let(:attributes) { base_attributes.merge(merged_at: Time.current, status: 1) }
+    context 'with merged_at and a skip_merged status' do
+      let(:attributes) do
+        base_attributes.merge(
+          merged_at: Time.current,
+          status: described_class.state_machine.states[:skip_merged].value
+        )
+      end
 
       it 'creates a completed car with no pipeline', :aggregate_failures do
         expect(subject).to be_persisted
-        expect(subject).to be_merged
+        expect(subject).to be_skip_merged
         expect(subject.pipeline).to be_nil
         expect(subject.merged_at).to be_present
       end
diff --git a/ee/spec/services/merge_requests/merge_service_spec.rb b/ee/spec/services/merge_requests/merge_service_spec.rb
index b60baac590598c..5792bf241ae415 100644
--- a/ee/spec/services/merge_requests/merge_service_spec.rb
+++ b/ee/spec/services/merge_requests/merge_service_spec.rb
@@ -180,7 +180,7 @@
         it 'creates a new merged Car record and adds the revision to the train history' do
           expect { subject }.to change { MergeTrains::Car.count }.by(1)
           expect(newest_car.merge_request).to eq(merge_request)
-          expect(newest_car).to be_merged
+          expect(newest_car).to be_skip_merged
 
           expect(train.sha_exists_in_history?(merge_request.merge_commit_sha)).to be_truthy
         end
-- 
GitLab


From c93cefdc069c82ad37c3fa045c025be047d6bdbe Mon Sep 17 00:00:00 2001
From: drew <dscimino@gmail.com>
Date: Wed, 27 Sep 2023 15:42:31 -0400
Subject: [PATCH 6/8] Rewrite a flaky test

All the specs in this file share a single MergeRequest record, which
has seemingly worked so far, but is problematic in the section where
we actually perform merges because it changes the state and becomes
no longer mergeable. This is something we should refactor later, but
for now I'm just isolating the state for these tests to keep the
suite reliable.
---
 ee/spec/requests/api/merge_requests_spec.rb | 31 +++++++++++++++++----
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/ee/spec/requests/api/merge_requests_spec.rb b/ee/spec/requests/api/merge_requests_spec.rb
index 24d1cb613c9e07..5cf5a606d464a1 100644
--- a/ee/spec/requests/api/merge_requests_spec.rb
+++ b/ee/spec/requests/api/merge_requests_spec.rb
@@ -364,19 +364,38 @@ def create_merge_request(args)
       expect(response).to have_gitlab_http_status(:ok)
     end
 
-    context 'when the requests asks to skip the train' do
+    context 'when the requests asks to skip the train', :aggregate_failures do
+      let(:project)       { create(:project, :public, :repository, creator: user, namespace: user.namespace) }
+      let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+      let(:skip_params)   { { skip_merge_train: true } }
+
+      let(:request) do
+        put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user), params: skip_params
+      end
+
       before do
+        project.update!(approvals_before_merge: 0)
         project.ci_cd_settings.update!(merge_trains_skip_train_allowed: true)
       end
 
-      let(:skip_params) { { skip_merge_train: true } }
+      it 'creates a new merged train car to represent the merged MR' do
+        expect { request }.to change { MergeTrains::Car.count }.by(1)
 
-      let(:request) do
-        put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user), params: skip_params
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(merge_request.reload).to be_merged
       end
 
-      it 'creates a new merged train car to represent the merged MR', :aggregate_failures do
-        expect { request }.to change { MergeTrains::Car.count }.by(1)
+      context 'with merge_trains_skip_train disabled' do
+        before do
+          stub_feature_flags(merge_trains_skip_train: false)
+        end
+
+        it 'creates a new merged train car to represent the merged MR' do
+          expect { request }.not_to change { MergeTrains::Car.count }
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(merge_request.reload).to be_merged
+        end
       end
     end
   end
-- 
GitLab


From d42eef12d5bb05cd088e977f47230c673d32c428 Mon Sep 17 00:00:00 2001
From: drew <dscimino@gmail.com>
Date: Thu, 28 Sep 2023 17:01:15 -0400
Subject: [PATCH 7/8] Move skip_merged Car creation into Car class method

---
 ee/app/models/merge_trains/car.rb             | 21 +++++++++++++++++++
 .../ee/merge_requests/merge_service.rb        | 15 ++-----------
 ee/spec/models/merge_trains/car_spec.rb       | 15 +++++++++++++
 3 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/ee/app/models/merge_trains/car.rb b/ee/app/models/merge_trains/car.rb
index 30ae8c529d4c6f..7709296a772ced 100644
--- a/ee/app/models/merge_trains/car.rb
+++ b/ee/app/models/merge_trains/car.rb
@@ -86,6 +86,27 @@ class Car < ApplicationRecord
         .merge(MergeRequest.preload_routables)
     end
 
+    # The purpose of creating a skip-merged car is to include a merge request
+    # in the completed car history to avoid refreshing the train after an
+    # immediate merge of the associated merge request
+    def self.insert_skip_merged_car_for(merge_request, merged_by)
+      # We currently create this Car directly after the git merge,
+      # in EE::MergeRequests::MergeService#after_merge, before the
+      # PostMergeService actually marks the MR as merged. If we
+      # change the order of operations, we should change this guard.
+      # But right now we're writing this to be used in as specific
+      # of a use case as possible.
+      return unless merge_request.locked?
+
+      merge_request.create_merge_train_car(
+        user: merged_by,
+        target_project: merge_request.target_project,
+        target_branch: merge_request.target_branch,
+        merged_at: Time.current,
+        status: MergeTrains::Car.state_machine.states[:skip_merged].value
+      )
+    end
+
     def all_next
       train.all_cars.where(
         MergeTrains::Car.arel_table[:id].gt(id)
diff --git a/ee/app/services/ee/merge_requests/merge_service.rb b/ee/app/services/ee/merge_requests/merge_service.rb
index 21c2df00edd598..cb7c2b27b85192 100644
--- a/ee/app/services/ee/merge_requests/merge_service.rb
+++ b/ee/app/services/ee/merge_requests/merge_service.rb
@@ -4,24 +4,13 @@ module EE
   module MergeRequests
     module MergeService
       def after_merge
-        create_completed_car if skipping_active_merge_train?
+        MergeTrains::Car.insert_skip_merged_car_for(merge_request, current_user) if skipping_active_merge_train?
 
         super
       end
 
       def skipping_active_merge_train?
-        params[:skip_merge_train] &&
-          project.merge_trains_skip_train_allowed?
-      end
-
-      def create_completed_car
-        merge_request.build_merge_train_car(
-          user: current_user,
-          target_project: merge_request.target_project,
-          target_branch: merge_request.target_branch,
-          merged_at: Time.current,
-          status: MergeTrains::Car.state_machine.states[:skip_merged].value
-        )
+        params[:skip_merge_train] && project.merge_trains_skip_train_allowed?
       end
     end
   end
diff --git a/ee/spec/models/merge_trains/car_spec.rb b/ee/spec/models/merge_trains/car_spec.rb
index c802d662022365..82f19736d5f067 100644
--- a/ee/spec/models/merge_trains/car_spec.rb
+++ b/ee/spec/models/merge_trains/car_spec.rb
@@ -110,6 +110,21 @@
     end
   end
 
+  describe '.insert_skip_merged_car_for', :aggregate_failures do
+    let(:maintainer)    { create(:user).tap { |u| merge_request.project.add_maintainer(u) } }
+    let(:merge_train)   { MergeTrains::Train.new(merge_request.project_id, merge_request.target_branch) }
+    let(:merge_request) { create(:merge_request, :locked) }
+
+    subject { described_class.insert_skip_merged_car_for(merge_request, maintainer) }
+
+    it 'creates and returns a new completed MergeTrain Car record' do
+      expect { subject }.to change { MergeTrains::Car.count }.by(1)
+      expect(subject).to be_persisted
+      expect(subject).to be_skip_merged
+      expect(subject.merge_request).to eq(merge_request)
+    end
+  end
+
   describe '#all_next' do
     subject { car.all_next }
 
-- 
GitLab


From 738f595b876d5c3e8a4047a4235edc2f32b6b59f Mon Sep 17 00:00:00 2001
From: drew <dscimino@gmail.com>
Date: Thu, 28 Sep 2023 18:43:06 -0400
Subject: [PATCH 8/8] Disallow skipping merge trains if they are not enabled,
 regardless of skip setting

---
 ee/app/models/ee/project_ci_cd_setting.rb     |  2 +-
 .../ci/project_ci_cd_settings_update_spec.rb  |  4 ++-
 ee/spec/models/project_ci_cd_setting_spec.rb  | 30 +++++++++++++++++++
 ee/spec/requests/api/merge_requests_spec.rb   |  9 +++++-
 .../merge_requests/merge_service_spec.rb      | 10 ++++++-
 5 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/ee/app/models/ee/project_ci_cd_setting.rb b/ee/app/models/ee/project_ci_cd_setting.rb
index 1d7e03009e1881..f4f3aa3139fc78 100644
--- a/ee/app/models/ee/project_ci_cd_setting.rb
+++ b/ee/app/models/ee/project_ci_cd_setting.rb
@@ -29,7 +29,7 @@ def auto_rollback_enabled?
     end
 
     def merge_trains_skip_train_allowed?
-      merge_trains_skip_train_allowed && ::Feature.enabled?(:merge_trains_skip_train, project)
+      merge_trains_skip_train_allowed && merge_trains_enabled? && ::Feature.enabled?(:merge_trains_skip_train, project)
     end
 
     private
diff --git a/ee/spec/graphql/ee/mutations/ci/project_ci_cd_settings_update_spec.rb b/ee/spec/graphql/ee/mutations/ci/project_ci_cd_settings_update_spec.rb
index 62cdb81e697e9d..17d0dd38d92555 100644
--- a/ee/spec/graphql/ee/mutations/ci/project_ci_cd_settings_update_spec.rb
+++ b/ee/spec/graphql/ee/mutations/ci/project_ci_cd_settings_update_spec.rb
@@ -65,7 +65,9 @@
       end
 
       it 'updates the value' do
-        expect(project.ci_cd_settings.merge_trains_skip_train_allowed?).to eq(true)
+        # This column value is not the same as #merge_trains_skip_train_allowed?
+        # which has added business logic
+        expect(project.ci_cd_settings.merge_trains_skip_train_allowed).to eq(true)
       end
     end
   end
diff --git a/ee/spec/models/project_ci_cd_setting_spec.rb b/ee/spec/models/project_ci_cd_setting_spec.rb
index 5891c8e52751a9..82169563a9b02f 100644
--- a/ee/spec/models/project_ci_cd_setting_spec.rb
+++ b/ee/spec/models/project_ci_cd_setting_spec.rb
@@ -78,6 +78,36 @@
     end
   end
 
+  describe '#merge_trains_skip_train_allowed?' do
+    subject(:result) { project.merge_trains_skip_train_allowed? }
+
+    let(:project) { create(:project) }
+
+    where(:merge_pipelines_enabled, :merge_trains_enabled, :skip_train_allowed, :feature_available, :expected_result) do
+      true  | true  | true  | true  | true
+      false | true  | true  | true  | false
+      true  | false | true  | true  | false
+      true  | true  | false | true  | false
+      true  | true  | true  | false | false
+    end
+
+    with_them do
+      before do
+        stub_licensed_features(merge_pipelines: feature_available, merge_trains: feature_available)
+      end
+
+      it 'returns true only if all of the related settings and features are true' do
+        project.update!(
+          merge_pipelines_enabled: merge_pipelines_enabled,
+          merge_trains_enabled: merge_trains_enabled,
+          merge_trains_skip_train_allowed: skip_train_allowed
+        )
+
+        expect(result).to eq(expected_result)
+      end
+    end
+  end
+
   describe '#auto_rollback_enabled?' do
     using RSpec::Parameterized::TableSyntax
 
diff --git a/ee/spec/requests/api/merge_requests_spec.rb b/ee/spec/requests/api/merge_requests_spec.rb
index 5cf5a606d464a1..463276629c051d 100644
--- a/ee/spec/requests/api/merge_requests_spec.rb
+++ b/ee/spec/requests/api/merge_requests_spec.rb
@@ -374,8 +374,15 @@ def create_merge_request(args)
       end
 
       before do
+        stub_licensed_features(merge_pipelines: true, merge_trains: true)
+        stub_feature_flags(disable_merge_trains: false)
+
         project.update!(approvals_before_merge: 0)
-        project.ci_cd_settings.update!(merge_trains_skip_train_allowed: true)
+        project.ci_cd_settings.update!(
+          merge_pipelines_enabled: true,
+          merge_trains_enabled: true,
+          merge_trains_skip_train_allowed: true
+        )
       end
 
       it 'creates a new merged train car to represent the merged MR' do
diff --git a/ee/spec/services/merge_requests/merge_service_spec.rb b/ee/spec/services/merge_requests/merge_service_spec.rb
index 5792bf241ae415..b06b10f72499a8 100644
--- a/ee/spec/services/merge_requests/merge_service_spec.rb
+++ b/ee/spec/services/merge_requests/merge_service_spec.rb
@@ -174,7 +174,15 @@
 
       context 'with merge_trains_skip_merge_train_allowed ProjectCiCdSetting enabled' do
         before do
-          project.ci_cd_settings.update!(merge_trains_skip_train_allowed: true)
+          stub_licensed_features(merge_pipelines: true, merge_trains: true)
+          stub_feature_flags(disable_merge_trains: false)
+
+          project.update!(approvals_before_merge: 0)
+          project.ci_cd_settings.update!(
+            merge_pipelines_enabled: true,
+            merge_trains_enabled: true,
+            merge_trains_skip_train_allowed: true
+          )
         end
 
         it 'creates a new merged Car record and adds the revision to the train history' do
-- 
GitLab