diff --git a/config/feature_flags/development/update_approval_rules_for_related_mrs.yml b/config/feature_flags/development/update_approval_rules_for_related_mrs.yml new file mode 100644 index 0000000000000000000000000000000000000000..ac233df2fb8a013a9e020b131f71cf5bc38ce543 --- /dev/null +++ b/config/feature_flags/development/update_approval_rules_for_related_mrs.yml @@ -0,0 +1,8 @@ +--- +name: update_approval_rules_for_related_mrs +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142339 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/440185 +milestone: '17.0' +type: development +group: group::pipeline security +default_enabled: false diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index fd57568444fca2a1a5c367148c4f927b49c9e079..992c7abf2a559351d436250a1d58ec9342d2e8e8 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -179,6 +179,8 @@ - 1 - - ci_unlock_pipelines_in_queue - 1 +- - ci_update_approval_rules_for_related_mrs + - 1 - - ci_update_build_names - 1 - - ci_upstream_projects_subscriptions_cleanup diff --git a/ee/app/models/ee/ci/pipeline.rb b/ee/app/models/ee/ci/pipeline.rb index 4478a31737018c4e80c6bfe305ddaa3a9d8b2184..e409eed284dac92aba6586b972ec29ec4562fe8a 100644 --- a/ee/app/models/ee/ci/pipeline.rb +++ b/ee/app/models/ee/ci/pipeline.rb @@ -288,6 +288,11 @@ def opened_merge_requests_with_head_sha all_merge_requests.opened.select { |merge_request| merge_request.diff_head_pipeline?(self) } end + def merge_requests_as_base_pipeline + merge_request_diffs = ::MergeRequestDiff.where(project_id: project_id, base_commit_sha: sha) + project.merge_requests.opened.where(id: merge_request_diffs.select(:merge_request_id)) + end + private def project_has_subscriptions? diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index b1ffcc3dc95430ce90f81e1e75c5ae12f2f0eb8a..4d10b7fabad7877ba4545dc046b2aae70ce66404 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -1065,6 +1065,15 @@ :weight: 1 :idempotent: false :tags: [] +- :name: ci_update_approval_rules_for_related_mrs + :worker_name: Ci::UpdateApprovalRulesForRelatedMrsWorker + :feature_category: :code_review_workflow + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: ci_upstream_projects_subscriptions_cleanup :worker_name: Ci::UpstreamProjectsSubscriptionsCleanupWorker :feature_category: :continuous_integration diff --git a/ee/app/workers/ci/sync_reports_to_report_approval_rules_worker.rb b/ee/app/workers/ci/sync_reports_to_report_approval_rules_worker.rb index 5be0a8742562eec91653d0c80dc9fd64ad64d398..5ff698e7e1651184852addcc367b7bb85b7be3f8 100644 --- a/ee/app/workers/ci/sync_reports_to_report_approval_rules_worker.rb +++ b/ee/app/workers/ci/sync_reports_to_report_approval_rules_worker.rb @@ -18,6 +18,10 @@ def perform(pipeline_id) return unless pipeline ::Ci::SyncReportsToApprovalRulesService.new(pipeline).execute + + return unless ::Feature.enabled?(:update_approval_rules_for_related_mrs, pipeline.project) + + ::Ci::UpdateApprovalRulesForRelatedMrsWorker.perform_async(pipeline.id) end end end diff --git a/ee/app/workers/ci/update_approval_rules_for_related_mrs_worker.rb b/ee/app/workers/ci/update_approval_rules_for_related_mrs_worker.rb new file mode 100644 index 0000000000000000000000000000000000000000..d1ee94c8f1b500fae5fa3d7a118dd3154efb0ac6 --- /dev/null +++ b/ee/app/workers/ci/update_approval_rules_for_related_mrs_worker.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module Ci + class UpdateApprovalRulesForRelatedMrsWorker + include ApplicationWorker + + feature_category :code_review_workflow + urgency :low + data_consistency :sticky + + idempotent! + + EACH_BATCH_COUNT = 100 + MAX_BATCHES_COUNT = 20 + + def perform(pipeline_id) + pipeline = Ci::Pipeline.find_by_id(pipeline_id) + return unless pipeline + + # rubocop: disable CodeReuse/ActiveRecord -- To avoid N+1 queries + merge_requests_as_base_pipeline = pipeline.merge_requests_as_base_pipeline.where.not(head_pipeline_id: nil) + + merge_requests_as_base_pipeline.each_batch(of: EACH_BATCH_COUNT) do |batch, index| + # Limit the number of merge requests we end up updating so this + # worker cannot run indefinitely querying and updating the database + # This limits the amount to 2000 open MRs with a matching SHA in a project. + break if index > MAX_BATCHES_COUNT + + head_pipelines = ::Ci::Pipeline.id_in(batch.pluck(:head_pipeline_id)).complete + head_pipelines.each { |pipeline| ::Ci::SyncReportsToApprovalRulesService.new(pipeline).execute } + end + # rubocop: enable CodeReuse/ActiveRecord + end + end +end diff --git a/ee/spec/models/ci/pipeline_spec.rb b/ee/spec/models/ci/pipeline_spec.rb index 0c44b83a00250b4a014a11e105720afffb4def80..ceac64528ade747edf14f92611027644d9ccbfce 100644 --- a/ee/spec/models/ci/pipeline_spec.rb +++ b/ee/spec/models/ci/pipeline_spec.rb @@ -1252,4 +1252,20 @@ expect(parent_pipeline.self_and_descendant_security_scans).to match_array([parent_scan, scan_1, scan_2]) end end + + describe "#merge_requests_as_base_pipeline" do + let_it_be_with_reload(:pipeline) { create(:ci_empty_pipeline, project: project, status: 'created', ref: 'master', sha: 'a288a022a53a5a944fae87bcec6efc87b7061808') } + let(:merge_request) { create(:merge_request, source_project: project) } + let(:base_pipeline) { create(:ee_ci_pipeline, :success, project: project, ref: merge_request.target_branch, sha: merge_request.diff_base_sha) } + let(:head_pipeline) { create(:ee_ci_pipeline, :success, project: project, ref: merge_request.source_branch) } + + it "returns merge requests whose `diff_base_sha` matches the pipeline's SHA" do + project.reload + expect(base_pipeline.merge_requests_as_base_pipeline).to eq([merge_request]) + end + + it "doesn't return merge requests whose `diff_base_sha` doesn't match the pipeline's SHA" do + expect(head_pipeline.merge_requests_as_base_pipeline).to be_empty + end + end end diff --git a/ee/spec/workers/ci/sync_reports_to_report_approval_rules_worker_spec.rb b/ee/spec/workers/ci/sync_reports_to_report_approval_rules_worker_spec.rb index a6d46cdec8b2fb08b9600c48a368de5fa52a3d2e..a14b9bde76bd51a99608803c16fb021a1e8b9618 100644 --- a/ee/spec/workers/ci/sync_reports_to_report_approval_rules_worker_spec.rb +++ b/ee/spec/workers/ci/sync_reports_to_report_approval_rules_worker_spec.rb @@ -4,8 +4,9 @@ RSpec.describe Ci::SyncReportsToReportApprovalRulesWorker, feature_category: :source_code_management do describe '#perform' do - let(:pipeline) { double(:pipeline, id: 42) } + let(:pipeline) { double(:pipeline, id: 42, project: project) } let(:sync_service) { double(:service, execute: true) } + let_it_be(:project) { create(:project) } context 'when pipeline exists' do before do @@ -18,6 +19,30 @@ described_class.new.perform(pipeline.id) end + + it "enqueues UpdateApprovalRulesForRelatedMrsWorker" do + expect(Ci::SyncReportsToApprovalRulesService).to receive(:new) + .with(pipeline).once.and_return(sync_service) + + expect(Ci::UpdateApprovalRulesForRelatedMrsWorker).to receive(:perform_async).with(pipeline.id) + + described_class.new.perform(pipeline.id) + end + + context "when feature flag is disabled" do + before do + stub_feature_flags(update_approval_rules_for_related_mrs: false) + end + + it "does not enqueue UpdateApprovalRulesForRelatedMrsWorker" do + expect(Ci::SyncReportsToApprovalRulesService).to receive(:new) + .with(pipeline).once.and_return(sync_service) + + expect(Ci::UpdateApprovalRulesForRelatedMrsWorker).not_to receive(:perform_async) + + described_class.new.perform(pipeline.id) + end + end end context 'when pipeline is missing' do diff --git a/ee/spec/workers/ci/update_approval_rules_for_related_mrs_worker_spec.rb b/ee/spec/workers/ci/update_approval_rules_for_related_mrs_worker_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..6910f64516f84b41733a3d556af5f5c6a1759d75 --- /dev/null +++ b/ee/spec/workers/ci/update_approval_rules_for_related_mrs_worker_spec.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ci::UpdateApprovalRulesForRelatedMrsWorker, feature_category: :code_review_workflow do + describe '#perform' do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:merge_request) { create(:merge_request, source_project: project) } + let_it_be(:base_pipeline) do + create(:ee_ci_pipeline, :success, project: project, + ref: merge_request.target_branch, sha: merge_request.diff_base_sha) + end + + let_it_be(:head_pipeline) { create(:ee_ci_pipeline, :success, project: project, ref: merge_request.source_branch) } + let_it_be(:base_pipeline_id) { base_pipeline.id } + + it_behaves_like 'an idempotent worker' do + let(:job_args) { [base_pipeline_id] } + end + + context "when the pipeline is a base pipeline of some merge requests" do + before do + merge_request.update!(head_pipeline_id: head_pipeline.id) + end + + it 'calls SyncReportsToApprovalRulesService for the head pipelines' do + expect(::Ci::SyncReportsToApprovalRulesService).to receive(:new).with(head_pipeline).and_call_original + + described_class.new.perform(base_pipeline_id) + end + + context 'when base pipeline is missing' do + let(:base_pipeline_id) { non_existing_record_id } + + it 'does not call SyncReportsToApprovalRulesService' do + expect(::Ci::SyncReportsToApprovalRulesService).not_to receive(:new) + + described_class.new.perform(base_pipeline_id) + end + end + + context 'when head_pipeline is not completed' do + before do + head_pipeline.update!(status: 'running') + end + + it 'does not call SyncReportsToApprovalRulesService for the head pipeline' do + expect(::Ci::SyncReportsToApprovalRulesService).not_to receive(:new).with(head_pipeline) + + described_class.new.perform(base_pipeline_id) + end + end + + context 'when there are too many merge requests' do + it 'limits the number of merge requests to update' do + stub_const("#{described_class}::MAX_BATCHES_COUNT", 1) + stub_const("#{described_class}::EACH_BATCH_COUNT", 2) + + 3.times { create_merge_request_with_completed_pipeline } + + expect(::Ci::SyncReportsToApprovalRulesService).to receive(:new).twice.and_call_original + described_class.new.perform(base_pipeline_id) + end + end + end + end + + def create_merge_request_with_completed_pipeline + merge_request = create(:merge_request, source_project: project, source_branch: generate(:branch)) + head_pipeline = create(:ee_ci_pipeline, :success, project: project, ref: merge_request.source_branch) + merge_request.update!(head_pipeline_id: head_pipeline.id) + merge_request.merge_request_diff.update!(base_commit_sha: base_pipeline.sha) + end +end