From 9ab4d38d8a7b30d1a412fa2eab509647f8a99666 Mon Sep 17 00:00:00 2001 From: A Browne <abrowne@gitlab.com> Date: Wed, 30 Aug 2023 15:38:55 +0000 Subject: [PATCH] Move merge train code to EE Add a template and non template example Use shared examples to DRY the specs --- .../merge_requests/create_ref_service.rb | 17 +- .../standard_merge_train_ref_merge_commit.yml | 8 + .../merge_trains/merge_commit_message.rb | 12 + .../ee/merge_requests/create_ref_service.rb | 22 ++ .../merge_trains/create_pipeline_service.rb | 12 +- .../merge_requests/create_ref_service_spec.rb | 311 ++++++++++++++++++ .../create_pipeline_service_spec.rb | 38 ++- .../merge_requests/create_ref_service_spec.rb | 143 ++++++-- 8 files changed, 499 insertions(+), 64 deletions(-) create mode 100644 config/feature_flags/development/standard_merge_train_ref_merge_commit.yml create mode 100644 ee/app/models/merge_trains/merge_commit_message.rb create mode 100644 ee/app/services/ee/merge_requests/create_ref_service.rb create mode 100644 ee/spec/services/ee/merge_requests/create_ref_service_spec.rb diff --git a/app/services/merge_requests/create_ref_service.rb b/app/services/merge_requests/create_ref_service.rb index e0f10183bac9..7e70d634113a 100644 --- a/app/services/merge_requests/create_ref_service.rb +++ b/app/services/merge_requests/create_ref_service.rb @@ -9,14 +9,13 @@ class CreateRefService CreateRefError = Class.new(StandardError) def initialize( - current_user:, merge_request:, target_ref:, first_parent_ref:, - source_sha: nil, merge_commit_message: nil) - + current_user:, merge_request:, target_ref:, first_parent_ref:, source_sha: nil + ) @current_user = current_user @merge_request = merge_request @initial_source_sha = source_sha @target_ref = target_ref - @merge_commit_message = merge_commit_message + @first_parent_ref = first_parent_ref @first_parent_sha = target_project.commit(first_parent_ref)&.sha end @@ -46,7 +45,7 @@ def execute private - attr_reader :current_user, :merge_request, :target_ref, :first_parent_sha, :initial_source_sha + attr_reader :current_user, :merge_request, :target_ref, :first_parent_ref, :first_parent_sha, :initial_source_sha delegate :target_project, to: :merge_request delegate :repository, to: :target_project @@ -119,12 +118,10 @@ def squash_commit_message strong_memoize_attr :squash_commit_message def merge_commit_message - return @merge_commit_message if @merge_commit_message.present? - - @merge_commit_message = ( - merge_request.merge_params['commit_message'].presence || + merge_request.merge_params['commit_message'].presence || merge_request.default_merge_commit_message(user: current_user) - ) end end end + +MergeRequests::CreateRefService.prepend_mod_with('MergeRequests::CreateRefService') diff --git a/config/feature_flags/development/standard_merge_train_ref_merge_commit.yml b/config/feature_flags/development/standard_merge_train_ref_merge_commit.yml new file mode 100644 index 000000000000..93e73e1c6b77 --- /dev/null +++ b/config/feature_flags/development/standard_merge_train_ref_merge_commit.yml @@ -0,0 +1,8 @@ +--- +name: standard_merge_train_ref_merge_commit +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129820 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/422724 +milestone: '16.4' +type: development +group: group::pipeline execution +default_enabled: false diff --git a/ee/app/models/merge_trains/merge_commit_message.rb b/ee/app/models/merge_trains/merge_commit_message.rb new file mode 100644 index 000000000000..ada5d0d129e5 --- /dev/null +++ b/ee/app/models/merge_trains/merge_commit_message.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module MergeTrains + module MergeCommitMessage + # Remove with merge_trains_create_ref_service + # and standard_merge_train_ref_merge_commit flag + def self.legacy_value(merge_request, previous_ref) + "Merge branch #{merge_request.source_branch} with #{previous_ref} " \ + "into #{merge_request.train_ref_path}" + end + end +end diff --git a/ee/app/services/ee/merge_requests/create_ref_service.rb b/ee/app/services/ee/merge_requests/create_ref_service.rb new file mode 100644 index 000000000000..ee218aef0b36 --- /dev/null +++ b/ee/app/services/ee/merge_requests/create_ref_service.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module EE + module MergeRequests + module CreateRefService + extend ::Gitlab::Utils::Override + + private + + override :merge_commit_message + def merge_commit_message + legacy_commit_message || super + end + + def legacy_commit_message + return if ::Feature.enabled?(:standard_merge_train_ref_merge_commit, target_project) + + ::MergeTrains::MergeCommitMessage.legacy_value(merge_request, first_parent_ref) + end + end + end +end diff --git a/ee/app/services/merge_trains/create_pipeline_service.rb b/ee/app/services/merge_trains/create_pipeline_service.rb index 37be611bbd56..182c0913f394 100644 --- a/ee/app/services/merge_trains/create_pipeline_service.rb +++ b/ee/app/services/merge_trains/create_pipeline_service.rb @@ -23,16 +23,13 @@ def validate(merge_request) def create_train_ref(merge_request, previous_ref) return error('previous ref is not specified') unless previous_ref - commit_message = commit_message(merge_request, previous_ref) - if Feature.enabled?(:merge_trains_create_ref_service, merge_request.target_project) ::MergeRequests::CreateRefService.new( current_user: merge_request.merge_user, merge_request: merge_request, target_ref: merge_request.train_ref_path, source_sha: merge_request.diff_head_sha, - first_parent_ref: previous_ref, - merge_commit_message: commit_message + first_parent_ref: previous_ref ).execute.to_h.transform_keys do |key| # TODO: Remove this transformation with FF merge_trains_create_ref_service case key @@ -49,17 +46,12 @@ def create_train_ref(merge_request, previous_ref) params: { target_ref: merge_request.train_ref_path, first_parent_ref: previous_ref, - commit_message: commit_message + commit_message: MergeTrains::MergeCommitMessage.legacy_value(merge_request, previous_ref) } ).execute(merge_request) end end - def commit_message(merge_request, previous_ref) - "Merge branch #{merge_request.source_branch} with #{previous_ref} " \ - "into #{merge_request.train_ref_path}" - end - def create_pipeline(merge_request, merge_status) response = ::Ci::CreatePipelineService.new(merge_request.target_project, merge_request.merge_user, ref: merge_request.train_ref_path, diff --git a/ee/spec/services/ee/merge_requests/create_ref_service_spec.rb b/ee/spec/services/ee/merge_requests/create_ref_service_spec.rb new file mode 100644 index 000000000000..26bc1df5b2da --- /dev/null +++ b/ee/spec/services/ee/merge_requests/create_ref_service_spec.rb @@ -0,0 +1,311 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::CreateRefService, feature_category: :merge_trains do + using RSpec::Parameterized::TableSyntax + + describe '#execute' do + let_it_be(:project) { create(:project, :empty_repo) } + let_it_be(:user) { project.creator } + let_it_be(:first_parent_ref) { project.default_branch_or_main } + let_it_be(:source_branch) { 'branch' } + let(:target_ref) { "refs/merge-requests/#{merge_request.iid}/train" } + let(:source_sha) { project.commit(source_branch).sha } + let(:squash) { false } + let(:legacy_commit_message) { MergeTrains::MergeCommitMessage.legacy_value(merge_request, first_parent_ref) } + let(:default_commit_message) { merge_request.default_merge_commit_message(user: user) } + + let(:merge_request) do + create( + :merge_request, + title: 'Merge request ref test', + author: user, + source_project: project, + target_project: project, + source_branch: source_branch, + target_branch: first_parent_ref, + squash: squash + ) + end + + subject(:result) do + described_class.new( + current_user: user, + merge_request: merge_request, + target_ref: target_ref, + source_sha: source_sha, + first_parent_ref: first_parent_ref + ).execute + end + + context 'when there is a user-caused gitaly error' do + let(:source_sha) { '123' } + + it 'returns an error response' do + expect(result[:status]).to eq :error + end + end + + context 'with valid inputs' do + before_all do + # ensure first_parent_ref is created before source_sha + project.repository.create_file( + user, + 'README.md', + '', + message: 'Base parent commit 1', + branch_name: first_parent_ref + ) + project.repository.create_branch(source_branch, first_parent_ref) + + # create two commits source_branch to test squashing + project.repository.create_file( + user, + '.gitlab-ci.yml', + '', + message: 'Feature branch commit 1', + branch_name: source_branch + ) + + project.repository.create_file( + user, + '.gitignore', + '', + message: 'Feature branch commit 2', + branch_name: source_branch + ) + + # create an extra commit not present on source_branch + project.repository.create_file( + user, + 'EXTRA', + '', + message: 'Base parent commit 2', + branch_name: first_parent_ref + ) + end + + shared_examples_for 'writing with a merge commit' do + it 'merges with a merge commit', :aggregate_failures do + expect(result[:status]).to eq :success + expect(result[:commit_sha]).to eq(project.repository.commit(target_ref).sha) + expect(result[:source_sha]).to eq(project.repository.commit(target_ref).parents[1].sha) + expect(result[:target_sha]).to eq(project.repository.commit(first_parent_ref).sha) + expect(project.repository.commits(target_ref, limit: 10, order: 'topo').map(&:message)).to( + match( + [ + expected_merge_commit, + 'Feature branch commit 2', + 'Feature branch commit 1', + 'Base parent commit 2', + 'Base parent commit 1' + ] + ) + ) + end + end + + shared_examples_for 'writing with a squash and merge commit' do + it 'writes the squashed result', :aggregate_failures do + expect(result[:status]).to eq :success + expect(result[:commit_sha]).to eq(project.repository.commit(target_ref).sha) + expect(result[:source_sha]).to eq(project.repository.commit(target_ref).parents[1].sha) + expect(result[:target_sha]).to eq(project.repository.commit(first_parent_ref).sha) + expect(project.repository.commits(target_ref, limit: 10, order: 'topo').map(&:message)).to( + match( + [ + expected_merge_commit, + "#{merge_request.title}\n", + 'Base parent commit 2', + 'Base parent commit 1' + ] + ) + ) + end + end + + shared_examples_for 'writing with a squash and no merge commit' do + it 'writes the squashed result without a merge commit', :aggregate_failures do + expect(result[:status]).to eq :success + expect(result[:commit_sha]).to eq(project.repository.commit(target_ref).sha) + expect(result[:source_sha]).to eq(project.repository.commit(target_ref).sha) + expect(result[:target_sha]).to eq(project.repository.commit(first_parent_ref).sha) + expect(project.repository.commits(target_ref, limit: 10, order: 'topo').map(&:message)).to( + match( + [ + "#{merge_request.title}\n", + 'Base parent commit 2', + 'Base parent commit 1' + ] + ) + ) + end + end + + shared_examples_for 'writing without a merge commit' do + it 'writes the rebased merged result', :aggregate_failures do + expect(result[:status]).to eq :success + expect(result[:commit_sha]).to eq(project.repository.commit(target_ref).sha) + expect(result[:source_sha]).to eq(project.repository.commit(target_ref).sha) + expect(result[:target_sha]).to eq(project.repository.commit(first_parent_ref).sha) + expect(project.repository.commits(target_ref, limit: 10, order: 'topo').map(&:message)).to( + eq( + [ + 'Feature branch commit 2', + 'Feature branch commit 1', + 'Base parent commit 2', + 'Base parent commit 1' + ] + ) + ) + end + end + + shared_examples 'merge commits without squash' do + context 'when standard commits are on' do + context 'with a custom template' do + let(:expected_merge_commit) { 'This is the merge commit' } # could also be default_commit_message + + before do + project.project_setting.update!(merge_commit_template: expected_merge_commit) + end + + it_behaves_like 'writing with a merge commit' + end + + context 'with no custom template' do + let(:expected_merge_commit) { default_commit_message } + + it_behaves_like 'writing with a merge commit' + end + end + + context 'when standard commits are off' do + let(:expected_merge_commit) { legacy_commit_message } + + before do + stub_feature_flags(standard_merge_train_ref_merge_commit: false) + end + + it_behaves_like 'writing with a merge commit' + end + end + + shared_examples 'merge commits with squash' do + context 'when squash is requested' do + let(:squash) { true } + let(:expected_merge_commit) { merge_request.default_merge_commit_message(user: user) } + + context 'when standard commits are on' do + let(:expected_merge_commit) { merge_request.default_merge_commit_message(user: user) } + + before do + project.project_setting.update!(merge_commit_template: 'This is the merge commit') + end + + it_behaves_like 'writing with a squash and merge commit' + end + + context 'when standard commits are off' do + let(:expected_merge_commit) { legacy_commit_message } + + before do + stub_feature_flags(standard_merge_train_ref_merge_commit: false) + end + + it_behaves_like 'writing with a squash and merge commit' + end + end + end + + context 'when the merge commit message is provided at time of merge' do + let(:custom_commit) { 'something custom' } + let(:expected_merge_commit) { custom_commit } + + before do + merge_request.merge_params['commit_message'] = custom_commit + end + + it 'writes the custom commit message', :aggregate_failures do + expect(result[:status]).to eq :success + expect(project.repository.commits(target_ref, limit: 1, order: 'topo').map(&:message)).to( + match([expected_merge_commit]) + ) + end + + context 'when squash set' do + let(:squash) { true } + + it_behaves_like 'writing with a squash and merge commit' + end + + context 'with standard_merge_train_ref_merge_commit disabled' do + before do + stub_feature_flags(standard_merge_train_ref_merge_commit: false) + end + + it 'writes the legacy commit message', :aggregate_failures do + expect(result[:status]).to eq :success + expect(project.repository.commits(target_ref, limit: 1, order: 'topo').map(&:message)).to( + match([legacy_commit_message]) + ) + end + + context 'when squash set' do + let(:squash) { true } + let(:expected_merge_commit) { legacy_commit_message } + + it_behaves_like 'writing with a squash and merge commit' + end + end + end + + context 'when merged commit strategy' do + include_examples 'merge commits without squash' + include_examples 'merge commits with squash' + end + + context 'when semi-linear merge strategy' do + before do + project.merge_method = :rebase_merge + project.save! + end + + include_examples 'merge commits without squash' + include_examples 'merge commits with squash' + end + + context 'when fast-forward merge strategy' do + before do + project.merge_method = :ff + project.save! + end + + context 'when standard commits are on' do + it_behaves_like 'writing without a merge commit' + + context 'when squash set' do + let(:squash) { true } + + it_behaves_like 'writing with a squash and no merge commit' + end + end + + context 'when standard commits are off' do + before do + stub_feature_flags(standard_merge_train_ref_merge_commit: false) + end + + it_behaves_like 'writing without a merge commit' + + context 'when squash set' do + let(:squash) { true } + + it_behaves_like 'writing with a squash and no merge commit' + end + end + end + end + end +end diff --git a/ee/spec/services/merge_trains/create_pipeline_service_spec.rb b/ee/spec/services/merge_trains/create_pipeline_service_spec.rb index b83ea0a4b5f8..7eacf883a292 100644 --- a/ee/spec/services/merge_trains/create_pipeline_service_spec.rb +++ b/ee/spec/services/merge_trains/create_pipeline_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe MergeTrains::CreatePipelineService, feature_category: :continuous_integration do - shared_examples_for 'MergeTrains::CreatePipelineService' do |ref_creation_class| + shared_examples_for 'MergeTrains::CreatePipelineService' do let_it_be(:project) { create(:project, :repository, :auto_devops, merge_pipelines_enabled: true, merge_trains_enabled: true) } let_it_be(:maintainer) { create(:user) } @@ -85,13 +85,13 @@ end it 'creates train ref' do + expect(expected_ref_creation_service).to receive(:new).with(hash_including( + expected_ref_creation_service_args + )).and_call_original + expect { subject } .to change { merge_request.project.repository.ref_exists?(merge_request.train_ref_path) } .from(false).to(true) - - expect(project.repository.commit(merge_request.train_ref_path).message) - .to eq("Merge branch #{merge_request.source_branch} with #{previous_ref} " \ - "into #{merge_request.train_ref_path}") end it 'calls Ci::CreatePipelineService for creating pipeline on train ref' do @@ -168,7 +168,7 @@ context 'when failed to prepare merge ref' do before do check_service = double - allow(ref_creation_class).to receive(:new) { check_service } + allow(expected_ref_creation_service).to receive(:new) { check_service } allow(check_service).to receive(:execute) { { status: :error, message: 'Merge ref was not found' } } end @@ -179,13 +179,35 @@ end end - it_behaves_like 'MergeTrains::CreatePipelineService', MergeRequests::CreateRefService + context 'when ff :merge_trains_create_ref_service is enabled' do + let(:expected_ref_creation_service) { MergeRequests::CreateRefService } + let(:expected_ref_creation_service_args) do + { + current_user: merge_request.merge_user, + merge_request: merge_request, + target_ref: merge_request.train_ref_path, + source_sha: merge_request.diff_head_sha, + first_parent_ref: previous_ref + } + end + + it_behaves_like 'MergeTrains::CreatePipelineService' + end context 'when ff :merge_trains_create_ref_service is disabled' do + let(:expected_ref_creation_service) { MergeRequests::MergeToRefService } + let(:expected_ref_creation_service_args) do + { + params: hash_including( + commit_message: MergeTrains::MergeCommitMessage.legacy_value(merge_request, previous_ref) + ) + } + end + before do stub_feature_flags(merge_trains_create_ref_service: false) end - it_behaves_like 'MergeTrains::CreatePipelineService', MergeRequests::MergeToRefService + it_behaves_like 'MergeTrains::CreatePipelineService' end end diff --git a/spec/services/merge_requests/create_ref_service_spec.rb b/spec/services/merge_requests/create_ref_service_spec.rb index 85ac651c1fa1..ea3ad61a30de 100644 --- a/spec/services/merge_requests/create_ref_service_spec.rb +++ b/spec/services/merge_requests/create_ref_service_spec.rb @@ -13,6 +13,7 @@ let(:target_ref) { "refs/merge-requests/#{merge_request.iid}/train" } let(:source_sha) { project.commit(source_branch).sha } let(:squash) { false } + let(:default_commit_message) { merge_request.default_merge_commit_message(user: user) } let(:merge_request) do create( @@ -84,27 +85,27 @@ ) end - it 'writes the merged result into target_ref', :aggregate_failures do - expect(result[:status]).to eq :success - expect(result[:commit_sha]).to eq(project.repository.commit(target_ref).sha) - expect(result[:source_sha]).to eq(project.repository.commit(target_ref).parents[1].sha) - expect(result[:target_sha]).to eq(project.repository.commit(first_parent_ref).sha) - expect(project.repository.commits(target_ref, limit: 10, order: 'topo').map(&:message)).to( - match( - [ - a_string_matching(/Merge branch '#{source_branch}' into '#{first_parent_ref}'/), - 'Feature branch commit 2', - 'Feature branch commit 1', - 'Base parent commit 2', - 'Base parent commit 1' - ] + shared_examples_for 'writing with a merge commit' do + it 'merges with a merge commit', :aggregate_failures do + expect(result[:status]).to eq :success + expect(result[:commit_sha]).to eq(project.repository.commit(target_ref).sha) + expect(result[:source_sha]).to eq(project.repository.commit(target_ref).parents[1].sha) + expect(result[:target_sha]).to eq(project.repository.commit(first_parent_ref).sha) + expect(project.repository.commits(target_ref, limit: 10, order: 'topo').map(&:message)).to( + match( + [ + expected_merge_commit, + 'Feature branch commit 2', + 'Feature branch commit 1', + 'Base parent commit 2', + 'Base parent commit 1' + ] + ) ) - ) + end end - context 'when squash is requested' do - let(:squash) { true } - + shared_examples_for 'writing with a squash and merge commit' do it 'writes the squashed result', :aggregate_failures do expect(result[:status]).to eq :success expect(result[:commit_sha]).to eq(project.repository.commit(target_ref).sha) @@ -113,7 +114,7 @@ expect(project.repository.commits(target_ref, limit: 10, order: 'topo').map(&:message)).to( match( [ - a_string_matching(/Merge branch '#{source_branch}' into '#{first_parent_ref}'/), + expected_merge_commit, "#{merge_request.title}\n", 'Base parent commit 2', 'Base parent commit 1' @@ -123,23 +124,16 @@ end end - context 'when semi-linear merges are enabled' do - before do - project.merge_method = :rebase_merge - project.save! - end - - it 'writes the semi-linear merged result', :aggregate_failures do + shared_examples_for 'writing with a squash and no merge commit' do + it 'writes the squashed result without a merge commit', :aggregate_failures do expect(result[:status]).to eq :success expect(result[:commit_sha]).to eq(project.repository.commit(target_ref).sha) - expect(result[:source_sha]).to eq(project.repository.commit(target_ref).parents[1].sha) + expect(result[:source_sha]).to eq(project.repository.commit(target_ref).sha) expect(result[:target_sha]).to eq(project.repository.commit(first_parent_ref).sha) expect(project.repository.commits(target_ref, limit: 10, order: 'topo').map(&:message)).to( match( [ - a_string_matching(/Merge branch '#{source_branch}' into '#{first_parent_ref}'/), - 'Feature branch commit 2', - 'Feature branch commit 1', + "#{merge_request.title}\n", 'Base parent commit 2', 'Base parent commit 1' ] @@ -148,12 +142,7 @@ end end - context 'when fast-forward merges are enabled' do - before do - project.merge_method = :ff - project.save! - end - + shared_examples_for 'writing without a merge commit' do it 'writes the rebased merged result', :aggregate_failures do expect(result[:status]).to eq :success expect(result[:commit_sha]).to eq(project.repository.commit(target_ref).sha) @@ -171,6 +160,88 @@ ) end end + + shared_examples 'merge commits without squash' do + context 'with a custom template' do + let(:expected_merge_commit) { 'This is the merge commit' } # could also be default_commit_message + + before do + project.project_setting.update!(merge_commit_template: expected_merge_commit) + end + + it_behaves_like 'writing with a merge commit' + end + + context 'with no custom template' do + let(:expected_merge_commit) { default_commit_message } + + it_behaves_like 'writing with a merge commit' + end + end + + shared_examples 'merge commits with squash' do + context 'when squash set' do + let(:squash) { true } + let(:expected_merge_commit) { merge_request.default_merge_commit_message(user: user) } + + before do + project.project_setting.update!(merge_commit_template: 'This is the merge commit') + end + + it_behaves_like 'writing with a squash and merge commit' + end + end + + context 'when the merge commit message is provided at time of merge' do + let(:expected_merge_commit) { 'something custom' } + + before do + merge_request.merge_params['commit_message'] = expected_merge_commit + end + + it 'writes the merged result', :aggregate_failures do + expect(result[:status]).to eq :success + expect(project.repository.commits(target_ref, limit: 1, order: 'topo').map(&:message)).to( + match([expected_merge_commit]) + ) + end + + context 'when squash set' do + let(:squash) { true } + + it_behaves_like 'writing with a squash and merge commit' + end + end + + context 'when merged commit strategy' do + include_examples 'merge commits without squash' + include_examples 'merge commits with squash' + end + + context 'when semi-linear merge strategy' do + before do + project.merge_method = :rebase_merge + project.save! + end + + include_examples 'merge commits without squash' + include_examples 'merge commits with squash' + end + + context 'when fast-forward merge strategy' do + before do + project.merge_method = :ff + project.save! + end + + it_behaves_like 'writing without a merge commit' + + context 'when squash set' do + let(:squash) { true } + + it_behaves_like 'writing with a squash and no merge commit' + end + end end end end -- GitLab