diff --git a/changelogs/unreleased/325691-add-sync_code_onwers_approval_rules_worker.yml b/changelogs/unreleased/325691-add-sync_code_onwers_approval_rules_worker.yml new file mode 100644 index 0000000000000000000000000000000000000000..48f6211f02cf5daa624da3204659d408ad7652f8 --- /dev/null +++ b/changelogs/unreleased/325691-add-sync_code_onwers_approval_rules_worker.yml @@ -0,0 +1,5 @@ +--- +title: Add new MergeRequests::SyncCodeOwnerApprovalRulesWorker +merge_request: 58512 +author: +type: performance diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 7febb3770d1234d9f7d5506b3c1cba91633b0e4a..c0aab89fd4639ce2fc4c7ddae22299784f12bad0 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -220,6 +220,8 @@ - 1 - - merge_requests_resolve_todos - 1 +- - merge_requests_sync_code_owner_approval_rules + - 1 - - metrics_dashboard_prune_old_annotations - 1 - - metrics_dashboard_sync_dashboards diff --git a/ee/app/services/ee/merge_requests/update_service.rb b/ee/app/services/ee/merge_requests/update_service.rb index e8f8d3af1964be7027c3ce112b842d9b07d31021..636b7b624eb02bc926d15e198ffe3a58936a7e58 100644 --- a/ee/app/services/ee/merge_requests/update_service.rb +++ b/ee/app/services/ee/merge_requests/update_service.rb @@ -39,7 +39,9 @@ def execute(merge_request) def after_update(merge_request) super - ::MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute + merge_request.run_after_commit do + ::MergeRequests::SyncCodeOwnerApprovalRulesWorker.perform_async(merge_request) + end end override :create_branch_change_note diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index dbe7694f337931de5cab6e4fffe97f0212b6d2a4..7d08829262cc16e73d3a4956f9f9f0ca3fbc1fb2 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -867,6 +867,14 @@ :weight: 1 :idempotent: :tags: [] +- :name: merge_requests_sync_code_owner_approval_rules + :feature_category: :source_code_management + :has_external_dependencies: + :urgency: :high + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: new_epic :feature_category: :epics :has_external_dependencies: diff --git a/ee/app/workers/merge_requests/sync_code_owner_approval_rules_worker.rb b/ee/app/workers/merge_requests/sync_code_owner_approval_rules_worker.rb new file mode 100644 index 0000000000000000000000000000000000000000..2ea4ae21ad1165513b95819bafa7bedbdc706366 --- /dev/null +++ b/ee/app/workers/merge_requests/sync_code_owner_approval_rules_worker.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module MergeRequests + class SyncCodeOwnerApprovalRulesWorker + include ApplicationWorker + + feature_category :source_code_management + urgency :high + deduplicate :until_executed + idempotent! + + def perform(merge_request_id) + merge_request = MergeRequest.find_by_id(merge_request_id) + return unless merge_request + + ::MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute + end + end +end diff --git a/ee/spec/services/ee/merge_requests/update_service_spec.rb b/ee/spec/services/ee/merge_requests/update_service_spec.rb index 44f079fc72078f498401eaf2a4962aa41a7b2338..9665aa888a5b18d5457a13e4f5bf3a7c72a5fbec 100644 --- a/ee/spec/services/ee/merge_requests/update_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/update_service_spec.rb @@ -302,12 +302,13 @@ def update_merge_request(opts) end end - it 'updates code owner approval rules' do - expect_next_instance_of(::MergeRequests::SyncCodeOwnerApprovalRules) do |instance| - expect(instance).to receive(:execute) - end + context 'when called inside an ActiveRecord transaction' do + it 'does not attempt to update code owner approval rules' do + allow(ActiveRecord::Base.connection).to receive(:transaction_open?).and_return(true) + expect(::MergeRequests::SyncCodeOwnerApprovalRulesWorker).not_to receive(:perform_async) - update_merge_request(title: 'Title') + update_merge_request(title: 'Title') + end end context 'updating reviewers_ids' do diff --git a/ee/spec/services/milestones/destroy_service_spec.rb b/ee/spec/services/milestones/destroy_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..35872d1c16d00383d5d1d3ccf568602b313129b9 --- /dev/null +++ b/ee/spec/services/milestones/destroy_service_spec.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Milestones::DestroyService do + let(:user) { create(:user) } + let(:project) { create(:project, :repository) } + let(:milestone) { create(:milestone, title: 'Milestone v1.0', project: project) } + + before do + project.add_maintainer(user) + end + + def service + described_class.new(project, user, {}) + end + + describe '#execute' do + context 'with an existing merge request' do + let!(:issue) { create(:issue, project: project, milestone: milestone) } + let!(:merge_request) { create(:merge_request, source_project: project, milestone: milestone) } + + it 'manually queues MergeRequests::SyncCodeOwnerApprovalRulesWorker jobs' do + expect(::MergeRequests::SyncCodeOwnerApprovalRulesWorker).to receive(:perform_async) + + service.execute(milestone) + end + end + end +end diff --git a/ee/spec/workers/merge_requests/sync_code_owner_approval_rules_worker_spec.rb b/ee/spec/workers/merge_requests/sync_code_owner_approval_rules_worker_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..2d6a6c365d8f2e9ff20e28352325fa4330607309 --- /dev/null +++ b/ee/spec/workers/merge_requests/sync_code_owner_approval_rules_worker_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe MergeRequests::SyncCodeOwnerApprovalRulesWorker do + let_it_be(:merge_request) { create(:merge_request) } + + subject { described_class.new } + + describe "#perform" do + it_behaves_like 'an idempotent worker' do + let(:job_args) { [merge_request.id] } + end + + context "when merge request is not found" do + it "returns without attempting to sync code owner rules" do + expect(MergeRequests::SyncCodeOwnerApprovalRules).not_to receive(:new) + + subject.perform(non_existing_record_id) + end + end + + context "when merge request is found" do + it "attempts to sync code owner rules" do + expect_next_instance_of(::MergeRequests::SyncCodeOwnerApprovalRules) do |instance| + expect(instance).to receive(:execute) + end + + subject.perform(merge_request.id) + end + end + end +end diff --git a/spec/services/milestones/destroy_service_spec.rb b/spec/services/milestones/destroy_service_spec.rb index dd68471d9274e93b17446e66b2d8c69c955eca69..6c08b7db43a0d05d053f76e7f72f703bed8dd5b8 100644 --- a/spec/services/milestones/destroy_service_spec.rb +++ b/spec/services/milestones/destroy_service_spec.rb @@ -22,14 +22,16 @@ def service expect { milestone.reload }.to raise_error ActiveRecord::RecordNotFound end - it 'deletes milestone id from issuables' do - issue = create(:issue, project: project, milestone: milestone) - merge_request = create(:merge_request, source_project: project, milestone: milestone) + context 'with an existing merge request' do + let!(:issue) { create(:issue, project: project, milestone: milestone) } + let!(:merge_request) { create(:merge_request, source_project: project, milestone: milestone) } - service.execute(milestone) + it 'deletes milestone id from issuables' do + service.execute(milestone) - expect(issue.reload.milestone).to be_nil - expect(merge_request.reload.milestone).to be_nil + expect(issue.reload.milestone).to be_nil + expect(merge_request.reload.milestone).to be_nil + end end it 'logs destroy event' do