diff --git a/app/services/merge_requests/create_pipeline_service.rb b/app/services/merge_requests/create_pipeline_service.rb index df5bc25cc74a2ceea801b27bfa8cbe9728384753..6851924d651280cd1cd6209b46be6a2df86c46f7 100644 --- a/app/services/merge_requests/create_pipeline_service.rb +++ b/app/services/merge_requests/create_pipeline_service.rb @@ -41,6 +41,16 @@ 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 allow_duplicate + params[:allow_duplicate] + end + + private + def can_create_pipeline_for?(merge_request) ## # UpdateMergeRequestsWorker could be retried by an exception. @@ -51,11 +61,13 @@ def can_create_pipeline_for?(merge_request) true end - def allow_duplicate - params[:allow_duplicate] + def user_can_run_pipeline?(merge_request) + current_user.can?(:create_pipeline, pipeline_project(merge_request)) end - private + 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) diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 27dda6f9b9e18918cf733bb64403c58dd07028b8..c778ba044f5d82bf3f5e9d1f1f3543dd06d0584a 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 0000000000000000000000000000000000000000..a3eadf9dec0e99ca49f8c16e79ee19736967618d --- /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 0000000000000000000000000000000000000000..8b3b9c991b91405b3803321cc359c9f21692a731 --- /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 +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 0000000000000000000000000000000000000000..1ec0d95168b0001bc93da68fc9385facccb31bff --- /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 0000000000000000000000000000000000000000..5d5e8be9c08285680eaf2b8527f1728fe5b2c8af --- /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 8953f28e86af1725b1b41e46f42ee01af626fac9..8d3b3ce1f53af37d61d3f17b6f9122305474978a 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) 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 02f2c8101f7b5af95e5e8075bfae515706ebb820..fdf4894d440ebde96745da94e25441050c998210 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/lib/gitlab/quick_actions/merge_request_actions.rb b/lib/gitlab/quick_actions/merge_request_actions.rb index c9aa047310a3c8ca135d3efa0ed2eb98cb0042d7..a907d96a6329b3de5fc762359525f397bb07705f 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/quick_action_activity_unique_counter.rb b/lib/gitlab/usage_data_counters/quick_action_activity_unique_counter.rb index bec28071bd9cc136f4a4656fe79c55aa1e6e2025..4ea1c08034096a1cdb7f7733c3a29583953417f9 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`. diff --git a/locale/gitlab.pot b/locale/gitlab.pot index d78136373620aa3ab7a930a112cbd20c3b1990da..b3c695e9afbd333c7ac4f97604f15d84fb355406 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/merge_requests/create_pipeline_service_spec.rb b/spec/services/merge_requests/create_pipeline_service_spec.rb index 5b61b0464e8fc45c3684231b0d1d50e1e073e4f4..19d71d8a90d11e5bb3f868afa847705e60a60457 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/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 5fbb5a064cfd29e4af56b9d18a87bee84ac78834..704280e66d42a872b2a7bb25eb3f1d745db226c5 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -3380,6 +3380,51 @@ end end + describe 'ship command' do + let_it_be(:merge_request) do + create(:merge_request, source_project: project) + end + + let(:content) { '/ship' } + + 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) + .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 action is allowed' do + let(:is_allowed) { true } + + 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 4fc61310ca29f19c98e9de98f71cf7578d9a2a5d..d3b0932c4df07ee052b82fd41442439b5970e3b7 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 0000000000000000000000000000000000000000..3917c45b8149acab6eaed284a5f03c08be2ddf27 --- /dev/null +++ b/spec/workers/merge_requests/ship_merge_request_worker_spec.rb @@ -0,0 +1,139 @@ +# 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(:config) do + <<~YAML + workflow: + rules: + - if: $CI_MERGE_REQUEST_ID + rspec: + script: echo + YAML + end + + before do + stub_ci_pipeline_yaml_file(config) + end + + context 'when pipeline creation succeeds' do + 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 triggers GraphQL merge status update + expect(GraphqlTriggers).to receive(:merge_request_merge_status_updated).with(merge_request).twice + + 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 + 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 + let(:config) do + <<~YAML + rspec: + script: echo + YAML + 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) + + expect { perform }.not_to change { ::Ci::Pipeline.count } + expect(perform).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) } + + 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 + let(:is_allowed) { true } + + 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 + + context 'when user cannot create pipeline' do + let(:is_allowed) { false } + + it { is_expected.to be false } + end + end +end