From 755f499bbb3746e54d7cac9c375a7b073f9ca0bf Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Mon, 24 Jan 2022 09:01:32 +0000 Subject: [PATCH 1/6] Add /ship quick action for merge requests Add a quick action that creates a new pipeline, wait for its creation and then set MWCP on the MR. This change is introduced behind a feature flag `ship_mr_quick_action` Changelog: added EE: true --- .../merge_requests/create_pipeline_service.rb | 12 ++ app/workers/all_queues.yml | 10 ++ .../ship_merge_request_worker.rb | 84 ++++++++++++ config/events/i_quickactions_ship.yml | 17 +++ .../ship_mr_quick_action.yml | 9 ++ ...tinct_user_id_from_i_quickactions_ship.yml | 23 ++++ .../merge_requests/create_pipeline_service.rb | 4 + .../quick_actions/merge_request_actions.rb | 17 +++ .../hll_redis_legacy_events.yml | 1 + locale/gitlab.pot | 9 ++ .../quick_actions/interpret_service_spec.rb | 42 ++++++ spec/workers/every_sidekiq_worker_spec.rb | 1 + .../ship_merge_request_worker_spec.rb | 127 ++++++++++++++++++ 13 files changed, 356 insertions(+) create mode 100644 app/workers/merge_requests/ship_merge_request_worker.rb create mode 100644 config/events/i_quickactions_ship.yml create mode 100644 config/feature_flags/gitlab_com_derisk/ship_mr_quick_action.yml create mode 100644 config/metrics/counts_all/count_distinct_user_id_from_i_quickactions_ship.yml create mode 100644 spec/workers/merge_requests/ship_merge_request_worker_spec.rb diff --git a/app/services/merge_requests/create_pipeline_service.rb b/app/services/merge_requests/create_pipeline_service.rb index df5bc25cc74a2c..6fcb30ad546ac1 100644 --- a/app/services/merge_requests/create_pipeline_service.rb +++ b/app/services/merge_requests/create_pipeline_service.rb @@ -41,6 +41,10 @@ def create_merge_request_pipeline(merge_request) pipeline end + def allowed?(merge_request) + can_create_pipeline_for?(merge_request) && user_can_run_pipeline?(merge_request) + end + def can_create_pipeline_for?(merge_request) ## # UpdateMergeRequestsWorker could be retried by an exception. @@ -57,6 +61,14 @@ def allow_duplicate private + def user_can_run_pipeline?(merge_request) + current_user.can?(:create_pipeline, pipeline_project(merge_request)) + end + + def pipeline_project(merge_request) + pipeline_project_and_ref(merge_request).first + end + def pipeline_project_and_ref(merge_request) if can_create_pipeline_in_target_project?(merge_request) [merge_request.target_project, merge_request.ref_path] diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 27dda6f9b9e189..c778ba044f5d82 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -103,6 +103,16 @@ :idempotent: false :tags: [] :queue_namespace: :auto_merge +- :name: auto_merge:merge_requests_ship_merge_request + :worker_name: MergeRequests::ShipMergeRequestWorker + :feature_category: :code_review_workflow + :has_external_dependencies: false + :urgency: :high + :resource_boundary: :cpu + :weight: 3 + :idempotent: true + :tags: [] + :queue_namespace: :auto_merge - :name: batched_background_migrations:database_batched_background_migration_ci_execution :worker_name: Database::BatchedBackgroundMigration::CiExecutionWorker :feature_category: :database diff --git a/app/workers/merge_requests/ship_merge_request_worker.rb b/app/workers/merge_requests/ship_merge_request_worker.rb new file mode 100644 index 00000000000000..a3eadf9dec0e99 --- /dev/null +++ b/app/workers/merge_requests/ship_merge_request_worker.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +module MergeRequests + class ShipMergeRequestWorker + include ApplicationWorker + + data_consistency :sticky + idempotent! + sidekiq_options retry: 3 + deduplicate :until_executed, if_deduplicated: :reason + urgency :high + worker_resource_boundary :cpu + queue_namespace :auto_merge + defer_on_database_health_signal :gitlab_main, [:merge_requests], 1.minute + loggable_arguments 0 + + feature_category :code_review_workflow + + def self.allowed?(merge_request:, current_user:) + return false if Feature.disabled?(:ship_mr_quick_action, merge_request.project) + + create_pipeline_service(merge_request: merge_request, current_user: current_user) + .allowed?(merge_request) + end + + def self.create_pipeline_service(merge_request:, current_user:, params: {}) + ::MergeRequests::CreatePipelineService.new( + project: merge_request.project, + current_user: current_user, + params: { allow_duplicate: true }.merge(params)) + end + + def perform(current_user_id, merge_request_id) + merge_request = MergeRequest.find_by_id(merge_request_id) + return unless merge_request + + current_user = User.find_by_id(current_user_id) + return unless current_user + + response = create_pipeline(merge_request: merge_request, current_user: current_user) + return response unless response.success? + + # Forcing update of merge request head pipeline after pipeline creation. + # Without this line we'll have to wait for MergeRequests::UpdateHeadPipelineWorker + # to react to Ci::PipelineCreatedEvent. + merge_request.update_head_pipeline + + response = set_auto_merge(merge_request: merge_request, current_user: current_user) + + GraphqlTriggers.merge_request_merge_status_updated(merge_request) + + if response == :failed + ServiceResponse.error(message: "Failed to enable Auto-Merge on #{merge_request.to_reference}") + else + ServiceResponse.success(message: "Auto-Merge enabled on #{merge_request.to_reference}") + end + end + + private + + def create_pipeline(merge_request:, current_user:) + pipeline_creation_request = ::Ci::PipelineCreation::Requests.start_for_merge_request(merge_request) + + # We need to update the merge status here because a pipeline has begun creating and MRs that require a + # successful pipeline should not be mergable at this point. + GraphqlTriggers.merge_request_merge_status_updated(merge_request) + + self.class.create_pipeline_service( + merge_request: merge_request, + current_user: current_user, + params: { pipeline_creation_request: pipeline_creation_request }) + .execute(merge_request) + end + + def set_auto_merge(merge_request:, current_user:) + # Use the current diff_head_sha after pipeline creation and head pipeline update + # to ensure the auto-merge SHA matches the actual head of the merge request + auto_merge_params = { sha: merge_request.diff_head_sha } + + ::AutoMergeService.new(merge_request.project, current_user, auto_merge_params) + .execute(merge_request) + end + end +end diff --git a/config/events/i_quickactions_ship.yml b/config/events/i_quickactions_ship.yml new file mode 100644 index 00000000000000..d03dae97254004 --- /dev/null +++ b/config/events/i_quickactions_ship.yml @@ -0,0 +1,17 @@ +--- +description: When the quickaction /ship is executed +internal_events: true +action: i_quickactions_ship +identifiers: +- project +- namespace +- user +product_group: code_review +product_categories: +- code_review_workflow +milestone: '18.6' +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/78998/diffs +tiers: +- free +- premium +- ultimate diff --git a/config/feature_flags/gitlab_com_derisk/ship_mr_quick_action.yml b/config/feature_flags/gitlab_com_derisk/ship_mr_quick_action.yml new file mode 100644 index 00000000000000..1ec0d95168b000 --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/ship_mr_quick_action.yml @@ -0,0 +1,9 @@ +--- +name: ship_mr_quick_action +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/210538 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/78998/diffs +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/513948 +milestone: '18.6' +group: group::pipeline execution +type: gitlab_com_derisk +default_enabled: false diff --git a/config/metrics/counts_all/count_distinct_user_id_from_i_quickactions_ship.yml b/config/metrics/counts_all/count_distinct_user_id_from_i_quickactions_ship.yml new file mode 100644 index 00000000000000..5d5e8be9c08285 --- /dev/null +++ b/config/metrics/counts_all/count_distinct_user_id_from_i_quickactions_ship.yml @@ -0,0 +1,23 @@ +--- +key_path: redis_hll_counters.count_distinct_user_id_from_i_quickactions_ship +description: Count of unique users using /ship quickaction +product_group: code_review +product_categories: +- code_review_workflow +performance_indicator_type: [] +value_type: number +status: active +milestone: '18.6' +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/78998/diffs +time_frame: +- 28d +- 7d +data_source: internal_events +data_category: optional +tiers: +- free +- premium +- ultimate +events: +- name: i_quickactions_ship + unique: user.id 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 8953f28e86af17..c7dacfb36f975e 100644 --- a/ee/app/services/ee/merge_requests/create_pipeline_service.rb +++ b/ee/app/services/ee/merge_requests/create_pipeline_service.rb @@ -38,6 +38,10 @@ def create_merged_result_pipeline_for(merge_request) end end + def allowed?(merge_request) + (can_create_merged_result_pipeline_for?(merge_request) && user_can_run_pipeline?(merge_request)) || super + end + def can_create_merged_result_pipeline_for?(merge_request) return false unless merge_request.project.merge_pipelines_enabled? return false unless can_create_pipeline_in_target_project?(merge_request) diff --git a/lib/gitlab/quick_actions/merge_request_actions.rb b/lib/gitlab/quick_actions/merge_request_actions.rb index c9aa047310a3c8..a907d96a6329b3 100644 --- a/lib/gitlab/quick_actions/merge_request_actions.rb +++ b/lib/gitlab/quick_actions/merge_request_actions.rb @@ -462,6 +462,23 @@ module MergeRequestActions { reviewer_text: 'reviewer'.pluralize(removed_reviewers.size), reviewer_references: removed_reviewers.map(&:to_reference).to_sentence } end end + + desc { _('Ship merge request (run pipeline and set auto-merge)') } + explanation { _('Ship merge request by creating a pipeline and set auto-merge.') } + types MergeRequest + condition do + ::MergeRequests::ShipMergeRequestWorker.allowed?( + merge_request: quick_action_target, + current_user: current_user) + end + command :ship do + ::MergeRequests::ShipMergeRequestWorker.perform_async( + current_user.id, + quick_action_target.id + ) + + @execution_message[:ship] = _('Actions to ship this merge request have been scheduled.') + end end def reviewer_users_sentence(users) diff --git a/lib/gitlab/usage_data_counters/hll_redis_legacy_events.yml b/lib/gitlab/usage_data_counters/hll_redis_legacy_events.yml index fe6bdca0c94a35..2670116d740d97 100644 --- a/lib/gitlab/usage_data_counters/hll_redis_legacy_events.yml +++ b/lib/gitlab/usage_data_counters/hll_redis_legacy_events.yml @@ -284,6 +284,7 @@ - i_quickactions_request_changes - i_quickactions_set_parent - i_quickactions_severity +- i_quickactions_ship - i_quickactions_shrug - i_quickactions_spend_add - i_quickactions_spend_subtract diff --git a/locale/gitlab.pot b/locale/gitlab.pot index d78136373620aa..b3c695e9afbd33 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -4077,6 +4077,9 @@ msgstr "" msgid "Actions for" msgstr "" +msgid "Actions to ship this merge request have been scheduled." +msgstr "" + msgid "Activate Service Desk" msgstr "" @@ -62808,6 +62811,12 @@ msgstr "" msgid "ShellOperations|Maximum number of Git operations per minute" msgstr "" +msgid "Ship merge request (run pipeline and set auto-merge)" +msgstr "" + +msgid "Ship merge request by creating a pipeline and set auto-merge." +msgstr "" + msgid "Short name" msgstr "" diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 5fbb5a064cfd29..a02c2d13955abc 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -3380,6 +3380,48 @@ end end + describe 'ship command' do + let_it_be(:merge_request) do + create(:merge_request, source_project: project) + end + + let(:content) { '/ship' } + + context 'when MR has no commits' do + before do + allow(merge_request).to receive(:has_no_commits?).and_return(true) + end + + it 'does not run the command' do + expect(::MergeRequests::ShipMergeRequestWorker) + .not_to receive(:perform_async) + + result = service.execute(content, merge_request) + expect(result).to eq( + ['', {}, 'Could not apply ship command.', ['ship']] + ) + end + end + + context 'when MR has commits' do + before do + allow(merge_request).to receive(:has_no_commits?).and_return(false) + end + + it 'runs the pipeline async' do + expect(::MergeRequests::ShipMergeRequestWorker) + .to receive(:perform_async) + .with(current_user.id, merge_request.id) + + result = service.execute(content, merge_request) + + expect(result).to eq( + ['', {}, 'Actions to ship this merge request have been scheduled.', ['ship']] + ) + end + end + end + context 'crm_contact commands' do let_it_be(:new_contact) { create(:contact, group: group) } let_it_be(:another_contact) { create(:contact, group: group) } diff --git a/spec/workers/every_sidekiq_worker_spec.rb b/spec/workers/every_sidekiq_worker_spec.rb index 4fc61310ca29f1..d3b0932c4df07e 100644 --- a/spec/workers/every_sidekiq_worker_spec.rb +++ b/spec/workers/every_sidekiq_worker_spec.rb @@ -367,6 +367,7 @@ 'MergeRequests::HandleAssigneesChangeWorker' => 3, 'MergeRequests::MergeabilityCheckBatchWorker' => 3, 'MergeRequests::ResolveTodosWorker' => 3, + 'MergeRequests::ShipMergeRequestWorker' => 3, 'MergeRequests::SyncCodeOwnerApprovalRulesWorker' => 3, 'MergeTrains::RefreshWorker' => 3, 'MergeWorker' => 3, diff --git a/spec/workers/merge_requests/ship_merge_request_worker_spec.rb b/spec/workers/merge_requests/ship_merge_request_worker_spec.rb new file mode 100644 index 00000000000000..82684b68326293 --- /dev/null +++ b/spec/workers/merge_requests/ship_merge_request_worker_spec.rb @@ -0,0 +1,127 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::ShipMergeRequestWorker, feature_category: :code_review_workflow do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + + before_all do + project.add_maintainer(user) + end + + describe '#perform' do + subject(:perform) { described_class.new.perform(user.id, merge_request.id) } + + let(:create_merge_request_pipeline_service) { instace_double(::MergeRequests::CreatePipelineService) } + + before do + allow(MergeRequests::CreatePipelineService).to receive(:new).and_return(create_merge_request_pipeline_service) + end + + context 'when pipeline creation succeeds' do + let(:pipeline) { create(:ci_pipeline, project: project, sha: merge_request.diff_head_sha) } + + before do + allow(create_merge_request_pipeline_service) + .to receive(:execute) + .and_return(ServiceResponse.success(payload: pipeline)) + end + + it 'creates a pipeline and sets auto-merge with correct SHA' do + # it executes the auto-merge + expect_next_instance_of(AutoMergeService) do |service| + expect(service).to receive(:execute).with(merge_request).and_call_original + end + + # It updates the head pipeline + expect(merge_request).to receive(:update_head_pipeline) + + # It triggers GraphQL merge status update + expect(GraphqlTriggers).to receive(:merge_request_merge_status_updated).with(merge_request).twice + + response = perform + expect(response).to be_success + + # Verify that the merge_request has auto_merge enabled with the correct SHA + expect(merge_request.reload.auto_merge_enabled?).to be true + expect(merge_request.merge_params['sha']).to eq(merge_request.diff_head_sha) + end + + context 'when auto-merge fails' do + before do + allow_next_instance_of(::AutoMergeService) do |service| + allow(service).to receive(:execute).and_return(:failed) + end + end + + it 'does not set auto-merge' do + response = perform + expect(response).to be_error + expect(response.message).to eq("Failed to enable Auto-MErge on #{merge_request.to_reference}") + + expect(merge_request.reload.auto_merge_enabled?).to be false + end + end + end + + context 'when pipeline creation fails' do + before do + allow(create_merge_request_pipeline_service) + .to receive(:execute) + .and_return(ServiceResponse.error(message: 'Pipeline creation failed')) + end + + it 'does not set auto-merge' do + expect(AutoMergeService).not_to receive(:new) + + # It does not update the head pipeline + expect(merge_request).not_to receive(:update_head_pipeline) + + response = perform + expect(response).to be_error + + expect(merge_request.reload.auto_merge_enabled?).to be false + end + end + + context 'when merge request does not exist' do + subject(:perform) { described_class.new.perform(user.id, non_existing_record_id) } + + it 'does not raise an error' do + expect { perform }.not_to raise_error + end + end + + context 'when user does not exist' do + subject(:perform) { described_class.new.perform(non_existing_record_id, merge_request.id) } + + it 'does not raise an error' do + expect { perform }.not_to raise_error + end + end + end + + describe '.allowed?' do + subject { described_class.allowed?(merge_request: merge_request, current_user: user) } + + context 'when user can create pipeline' do + before do + allow_next_instance_of(MergeRequests::CreatePipelineService) do |service| + allow(service).to receive(:allowed?).with(merge_request).and_return(true) + end + end + + it { is_expected.to be true } + + context 'when feature flag is disabled' do + before do + stub_feature_flags(ship_mr_quick_action: false) + end + + it { is_expected.to be false } + end + end + end +end -- GitLab From 34d55fcfa2bb9af8a379d143330b99b82fd41f14 Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Tue, 14 Oct 2025 21:26:55 +0100 Subject: [PATCH 2/6] Fix typo --- spec/workers/merge_requests/ship_merge_request_worker_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/workers/merge_requests/ship_merge_request_worker_spec.rb b/spec/workers/merge_requests/ship_merge_request_worker_spec.rb index 82684b68326293..fa485aca77f6cb 100644 --- a/spec/workers/merge_requests/ship_merge_request_worker_spec.rb +++ b/spec/workers/merge_requests/ship_merge_request_worker_spec.rb @@ -14,7 +14,7 @@ describe '#perform' do subject(:perform) { described_class.new.perform(user.id, merge_request.id) } - let(:create_merge_request_pipeline_service) { instace_double(::MergeRequests::CreatePipelineService) } + let(:create_merge_request_pipeline_service) { instance_double(::MergeRequests::CreatePipelineService) } before do allow(MergeRequests::CreatePipelineService).to receive(:new).and_return(create_merge_request_pipeline_service) -- GitLab From 7209d56ab6cfa219f54069808ddd8daf3c9ca297 Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Wed, 15 Oct 2025 09:45:06 +0100 Subject: [PATCH 3/6] Fix failing specs --- .../quick_actions/interpret_service_spec.rb | 19 ++++++++------- .../ship_merge_request_worker_spec.rb | 24 ++++++++++++++----- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index a02c2d13955abc..704280e66d42a8 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -3387,10 +3387,15 @@ let(:content) { '/ship' } - context 'when MR has no commits' do - before do - allow(merge_request).to receive(:has_no_commits?).and_return(true) - end + before do + allow(::MergeRequests::ShipMergeRequestWorker) + .to receive(:allowed?) + .with(merge_request: merge_request, current_user: current_user) + .and_return(is_allowed) + end + + context 'when action is not allowed' do + let(:is_allowed) { false } it 'does not run the command' do expect(::MergeRequests::ShipMergeRequestWorker) @@ -3403,10 +3408,8 @@ end end - context 'when MR has commits' do - before do - allow(merge_request).to receive(:has_no_commits?).and_return(false) - end + context 'when action is allowed' do + let(:is_allowed) { true } it 'runs the pipeline async' do expect(::MergeRequests::ShipMergeRequestWorker) diff --git a/spec/workers/merge_requests/ship_merge_request_worker_spec.rb b/spec/workers/merge_requests/ship_merge_request_worker_spec.rb index fa485aca77f6cb..00dda657b1063a 100644 --- a/spec/workers/merge_requests/ship_merge_request_worker_spec.rb +++ b/spec/workers/merge_requests/ship_merge_request_worker_spec.rb @@ -59,7 +59,7 @@ it 'does not set auto-merge' do response = perform expect(response).to be_error - expect(response.message).to eq("Failed to enable Auto-MErge on #{merge_request.to_reference}") + expect(response.message).to eq("Failed to enable Auto-Merge on #{merge_request.to_reference}") expect(merge_request.reload.auto_merge_enabled?).to be false end @@ -106,12 +106,18 @@ describe '.allowed?' do subject { described_class.allowed?(merge_request: merge_request, current_user: user) } + let(:create_merge_request_pipeline_service) do + instance_double(::MergeRequests::CreatePipelineService, allowed?: is_allowed) + end + + before do + allow(MergeRequests::CreatePipelineService) + .to receive(:new) + .and_return(create_merge_request_pipeline_service) + end + context 'when user can create pipeline' do - before do - allow_next_instance_of(MergeRequests::CreatePipelineService) do |service| - allow(service).to receive(:allowed?).with(merge_request).and_return(true) - end - end + let(:is_allowed) { true } it { is_expected.to be true } @@ -123,5 +129,11 @@ it { is_expected.to be false } end end + + context 'when user cannot create pipeline' do + let(:is_allowed) { false } + + it { is_expected.to be true } + end end end -- GitLab From 8b8003a05ce164740196dc2d9fe66ae0b242c810 Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Mon, 20 Oct 2025 10:29:38 +0100 Subject: [PATCH 4/6] Fix failing specs --- .../merge_requests/create_pipeline_service.rb | 12 ++-- .../merge_requests/create_pipeline_service.rb | 8 +-- .../create_pipeline_service_spec.rb | 71 +++++++++++++++---- .../create_pipeline_service_spec.rb | 41 +++++++++++ .../ship_merge_request_worker_spec.rb | 44 ++++++------ 5 files changed, 129 insertions(+), 47 deletions(-) diff --git a/app/services/merge_requests/create_pipeline_service.rb b/app/services/merge_requests/create_pipeline_service.rb index 6fcb30ad546ac1..6851924d651280 100644 --- a/app/services/merge_requests/create_pipeline_service.rb +++ b/app/services/merge_requests/create_pipeline_service.rb @@ -45,6 +45,12 @@ def allowed?(merge_request) can_create_pipeline_for?(merge_request) && user_can_run_pipeline?(merge_request) end + def allow_duplicate + params[:allow_duplicate] + end + + private + def can_create_pipeline_for?(merge_request) ## # UpdateMergeRequestsWorker could be retried by an exception. @@ -55,12 +61,6 @@ def can_create_pipeline_for?(merge_request) true end - def allow_duplicate - params[:allow_duplicate] - end - - private - def user_can_run_pipeline?(merge_request) current_user.can?(:create_pipeline, pipeline_project(merge_request)) 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 c7dacfb36f975e..8d3b3ce1f53af3 100644 --- a/ee/app/services/ee/merge_requests/create_pipeline_service.rb +++ b/ee/app/services/ee/merge_requests/create_pipeline_service.rb @@ -14,6 +14,10 @@ def execute(merge_request) super end + def allowed?(merge_request) + (can_create_merged_result_pipeline_for?(merge_request) && user_can_run_pipeline?(merge_request)) || super + end + private def create_merged_result_pipeline_for(merge_request) @@ -38,10 +42,6 @@ def create_merged_result_pipeline_for(merge_request) end end - def allowed?(merge_request) - (can_create_merged_result_pipeline_for?(merge_request) && user_can_run_pipeline?(merge_request)) || super - end - def can_create_merged_result_pipeline_for?(merge_request) return false unless merge_request.project.merge_pipelines_enabled? return false unless can_create_pipeline_in_target_project?(merge_request) 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 02f2c8101f7b5a..fdf4894d440ebd 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 @@ -6,27 +6,29 @@ feature_category: :continuous_integration do include ProjectForksHelper + let_it_be(:project, refind: true) { create(:project, :repository) } + + let(:service) { described_class.new(project: source_project, current_user: user, params: params) } + let(:user) { create(:user) } + let(:source_project) { project } + let(:source_branch) { 'feature' } + let(:target_project) { project } + let(:target_branch) { 'master' } + let(:params) { {} } + + let(:merge_request) do + create(:merge_request, + source_project: source_project, source_branch: source_branch, + target_project: target_project, target_branch: target_branch, + merge_status: 'unchecked') + end + describe '#execute' do subject { service.execute(merge_request) } - 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 } - let(:source_project) { project } - let(:source_branch) { 'feature' } - let(:target_project) { project } - let(:target_branch) { 'master' } - - let(:merge_request) do - create(:merge_request, - source_project: source_project, source_branch: source_branch, - target_project: target_project, target_branch: target_branch, - merge_status: 'unchecked') - end let(:ci_yaml) do YAML.dump({ @@ -206,4 +208,43 @@ end end end + + describe '#allowed?' do + using RSpec::Parameterized::TableSyntax + + subject(:allowed) { service.allowed?(merge_request) } + + let(:user_without_permissions) { create(:user) } + + where(:merged_result_pipeline_supported, :detached_mr_pipeline_supported, :user_can_run_pipeline, :result) do + true | true | true | true + true | false | true | true + true | true | false | false + true | false | false | false + false | true | true | true + false | true | false | false + false | false | true | false + false | false | false | false + end + + with_them do + before do + allow(service) + .to receive(:can_create_merged_result_pipeline_for?) + .with(merge_request) + .and_return(merged_result_pipeline_supported) + + allow(service) + .to receive(:can_create_pipeline_for?) + .with(merge_request) + .and_return(detached_mr_pipeline_supported) + + project.add_developer(user) if user_can_run_pipeline + end + + it 'matches the expected result' do + expect(allowed).to eq(result) + end + end + end end diff --git a/spec/services/merge_requests/create_pipeline_service_spec.rb b/spec/services/merge_requests/create_pipeline_service_spec.rb index 5b61b0464e8fc4..19d71d8a90d11e 100644 --- a/spec/services/merge_requests/create_pipeline_service_spec.rb +++ b/spec/services/merge_requests/create_pipeline_service_spec.rb @@ -353,4 +353,45 @@ end end end + + describe '#allowed?' do + subject(:allowed) { service.allowed?(merge_request) } + + context 'when both conditions are met' do + before do + allow(service).to receive(:can_create_pipeline_for?).with(merge_request).and_return(true) + # user is developer of project + end + + it { is_expected.to be_truthy } + end + + context 'when can_create_pipeline_for? returns false' do + before do + allow(service).to receive(:can_create_pipeline_for?).with(merge_request).and_return(false) + end + + it { is_expected.to be_falsey } + end + + context 'when user_can_run_pipeline? returns false' do + let(:actor) { create(:user) } + + before do + allow(service).to receive(:can_create_pipeline_for?).with(merge_request).and_return(true) + end + + it { is_expected.to be_falsey } + end + + context 'when both conditions are false' do + let(:actor) { create(:user) } + + before do + allow(service).to receive(:can_create_pipeline_for?).with(merge_request).and_return(false) + end + + it { is_expected.to be_falsey } + end + end end diff --git a/spec/workers/merge_requests/ship_merge_request_worker_spec.rb b/spec/workers/merge_requests/ship_merge_request_worker_spec.rb index 00dda657b1063a..3917c45b8149ac 100644 --- a/spec/workers/merge_requests/ship_merge_request_worker_spec.rb +++ b/spec/workers/merge_requests/ship_merge_request_worker_spec.rb @@ -14,39 +14,38 @@ describe '#perform' do subject(:perform) { described_class.new.perform(user.id, merge_request.id) } - let(:create_merge_request_pipeline_service) { instance_double(::MergeRequests::CreatePipelineService) } + let(:config) do + <<~YAML + workflow: + rules: + - if: $CI_MERGE_REQUEST_ID + rspec: + script: echo + YAML + end before do - allow(MergeRequests::CreatePipelineService).to receive(:new).and_return(create_merge_request_pipeline_service) + stub_ci_pipeline_yaml_file(config) end context 'when pipeline creation succeeds' do - let(:pipeline) { create(:ci_pipeline, project: project, sha: merge_request.diff_head_sha) } - - before do - allow(create_merge_request_pipeline_service) - .to receive(:execute) - .and_return(ServiceResponse.success(payload: pipeline)) - end - it 'creates a pipeline and sets auto-merge with correct SHA' do # it executes the auto-merge expect_next_instance_of(AutoMergeService) do |service| expect(service).to receive(:execute).with(merge_request).and_call_original end - # It updates the head pipeline - expect(merge_request).to receive(:update_head_pipeline) - # It triggers GraphQL merge status update expect(GraphqlTriggers).to receive(:merge_request_merge_status_updated).with(merge_request).twice - response = perform - expect(response).to be_success + expect { perform }.to change { ::Ci::Pipeline.count }.by(1) + + expect(perform).to be_success # Verify that the merge_request has auto_merge enabled with the correct SHA expect(merge_request.reload.auto_merge_enabled?).to be true expect(merge_request.merge_params['sha']).to eq(merge_request.diff_head_sha) + expect(merge_request.head_pipeline_id).to eq(::Ci::Pipeline.last.id) end context 'when auto-merge fails' do @@ -67,10 +66,11 @@ end context 'when pipeline creation fails' do - before do - allow(create_merge_request_pipeline_service) - .to receive(:execute) - .and_return(ServiceResponse.error(message: 'Pipeline creation failed')) + let(:config) do + <<~YAML + rspec: + script: echo + YAML end it 'does not set auto-merge' do @@ -79,8 +79,8 @@ # It does not update the head pipeline expect(merge_request).not_to receive(:update_head_pipeline) - response = perform - expect(response).to be_error + expect { perform }.not_to change { ::Ci::Pipeline.count } + expect(perform).to be_error expect(merge_request.reload.auto_merge_enabled?).to be false end @@ -133,7 +133,7 @@ context 'when user cannot create pipeline' do let(:is_allowed) { false } - it { is_expected.to be true } + it { is_expected.to be false } end end end -- GitLab From b58598809ade82db8dcd63934b9191412c1e5183 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Wielich?= Date: Tue, 21 Oct 2025 17:48:32 +0200 Subject: [PATCH 5/6] Apply 1 suggestion(s) to 1 file(s) --- config/events/i_quickactions_ship.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/events/i_quickactions_ship.yml b/config/events/i_quickactions_ship.yml index d03dae97254004..8b3b9c991b9140 100644 --- a/config/events/i_quickactions_ship.yml +++ b/config/events/i_quickactions_ship.yml @@ -10,7 +10,7 @@ product_group: code_review product_categories: - code_review_workflow milestone: '18.6' -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/78998/diffs +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/78998 tiers: - free - premium -- GitLab From 182b8f40c7c0eb1d9d7d6503d4328f2bc003053e Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Thu, 23 Oct 2025 13:09:35 +0200 Subject: [PATCH 6/6] Fix analytics tracking --- lib/gitlab/usage_data_counters/hll_redis_legacy_events.yml | 1 - .../usage_data_counters/quick_action_activity_unique_counter.rb | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/usage_data_counters/hll_redis_legacy_events.yml b/lib/gitlab/usage_data_counters/hll_redis_legacy_events.yml index 2670116d740d97..fe6bdca0c94a35 100644 --- a/lib/gitlab/usage_data_counters/hll_redis_legacy_events.yml +++ b/lib/gitlab/usage_data_counters/hll_redis_legacy_events.yml @@ -284,7 +284,6 @@ - i_quickactions_request_changes - i_quickactions_set_parent - i_quickactions_severity -- i_quickactions_ship - i_quickactions_shrug - i_quickactions_spend_add - i_quickactions_spend_subtract diff --git a/lib/gitlab/usage_data_counters/quick_action_activity_unique_counter.rb b/lib/gitlab/usage_data_counters/quick_action_activity_unique_counter.rb index bec28071bd9cc1..4ea1c08034096a 100644 --- a/lib/gitlab/usage_data_counters/quick_action_activity_unique_counter.rb +++ b/lib/gitlab/usage_data_counters/quick_action_activity_unique_counter.rb @@ -14,6 +14,7 @@ class << self remove_email_single q status + ship ].freeze # Tracks the quick action with name `name`. -- GitLab