diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 29aba3c86795ebd421975154403341b4e932b1b7..89e5920a4fb4e03abe4f9b0a22ac10274a45542c 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/models/ee/project_ci_cd_setting.rb b/ee/app/models/ee/project_ci_cd_setting.rb index 1d7e03009e1881730ebd172801074690c961d819..f4f3aa3139fc783dfea5101513dcdab38cf7da4f 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/app/models/merge_trains/car.rb b/ee/app/models/merge_trains/car.rb index 5b62299a8a64d98c590a63847da1db3d734b2fa7..7709296a772ced0ff6b7ace707ed9214217acc72 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) } @@ -85,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 new file mode 100644 index 0000000000000000000000000000000000000000..cb7c2b27b8519281810fb72d806969cac3158cb8 --- /dev/null +++ b/ee/app/services/ee/merge_requests/merge_service.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module EE + module MergeRequests + module MergeService + def after_merge + 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 + end + end +end diff --git a/ee/lib/ee/api/merge_requests.rb b/ee/lib/ee/api/merge_requests.rb index aa03f1ebb8b77b1fb9a3a09ada5af55b787728eb..f642579d30e91e808d678c6f7c8179d294168e4a 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/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 62cdb81e697e9d65611c6bb0e7ee54c925741f64..17d0dd38d92555151ba480afc1b5a9e3b39e0f7e 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/merge_trains/car_spec.rb b/ee/spec/models/merge_trains/car_spec.rb index c963ea0e99a714b8643390db745bff8b70567d4d..82f19736d5f067f15ebd41635f07e14423ffd024 100644 --- a/ee/spec/models/merge_trains/car_spec.rb +++ b/ee/spec/models/merge_trains/car_spec.rb @@ -23,6 +23,37 @@ 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 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_skip_merged + expect(subject.pipeline).to be_nil + expect(subject.merged_at).to be_present + end + end + end + describe '.active' do subject { described_class.active } @@ -79,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 } diff --git a/ee/spec/models/project_ci_cd_setting_spec.rb b/ee/spec/models/project_ci_cd_setting_spec.rb index 5891c8e52751a9660bce5feff9f6393a42a65993..82169563a9b02f1c05dd254c9e90e5f4f76385a5 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 059dfc939f24693f5b52b80ce0741b036a6ede27..463276629c051d25dc5177c852ec97f39571388d 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,48 @@ def create_merge_request(args) expect(response).to have_gitlab_http_status(:ok) end + + 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 + 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 train car to represent the merged MR' do + expect { request }.to change { MergeTrains::Car.count }.by(1) + + expect(response).to have_gitlab_http_status(:ok) + expect(merge_request.reload).to be_merged + end + + 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 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 7e08e8c9e3e880ce6b871d1d5ebfdfc736f7ca55..b06b10f72499a80ef5c6b93387fa9d5bae193afc 100644 --- a/ee/spec/services/merge_requests/merge_service_spec.rb +++ b/ee/spec/services/merge_requests/merge_service_spec.rb @@ -164,6 +164,60 @@ end end end + + context 'when skip_merge_train is true', :aggregate_failures do + 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 + 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 + expect { subject }.to change { MergeTrains::Car.count }.by(1) + expect(newest_car.merge_request).to eq(merge_request) + expect(newest_car).to be_skip_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 11f25599be5c07adccfa2a0f901818f60ed368d6..b8285bbd109d17cb93ab21ed2aea5035597d5cae 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') @@ -670,7 +683,7 @@ 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