Commit fe3d48c8 authored by Mehmet Emin INAC's avatar Mehmet Emin INAC Committed by Mayra Cabrera

Fix race condition issue on approval rules sync job

`Security::SyncReportsToApprovalRulesService` service class depends on
the attribute of `head_pipeline` of merge requests to be set to sync
security report results to approval rules.

But the `head_pipeline` attribute of merge requests are set by Sidekiq
job on background which can be finished before or after the logic of
`Security::SyncReportsToApprovalRulesService`.

For that reason, we have to move the scheduling of sync approval rule
job after we set the `head_pipeline` of merge request.
parent 31805a5a
...@@ -14,3 +14,5 @@ module MergeRequests ...@@ -14,3 +14,5 @@ module MergeRequests
end end
end end
end end
MergeRequests::AfterCreateService.prepend_if_ee('EE::MergeRequests::AfterCreateService')
# frozen_string_literal: true
module EE
module MergeRequests
module AfterCreateService
extend ::Gitlab::Utils::Override
override :execute
def execute(merge_request)
super
schedule_sync_for(merge_request.head_pipeline_id)
end
private
def schedule_sync_for(pipeline_id)
::SyncSecurityReportsToReportApprovalRulesWorker.perform_async(pipeline_id) if pipeline_id
end
end
end
end
...@@ -12,12 +12,6 @@ module EE ...@@ -12,12 +12,6 @@ module EE
::MergeRequests::SyncCodeOwnerApprovalRules.new(issuable).execute ::MergeRequests::SyncCodeOwnerApprovalRules.new(issuable).execute
::MergeRequests::SyncReportApproverApprovalRules.new(issuable).execute ::MergeRequests::SyncReportApproverApprovalRules.new(issuable).execute
# Attempt to sync reports if pipeline has finished before MR has been created
pipeline = issuable.find_actual_head_pipeline
if pipeline
::SyncSecurityReportsToReportApprovalRulesWorker.perform_async(pipeline.id)
end
::MergeRequests::UpdateBlocksService ::MergeRequests::UpdateBlocksService
.new(issuable, current_user, blocking_merge_requests_params) .new(issuable, current_user, blocking_merge_requests_params)
.execute .execute
......
---
title: Fix the `Vulnerability-Check` and `License-Check` approval rules to be synched correctly after creating a merge request.
merge_request: 39587
author:
type: fixed
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe MergeRequests::AfterCreateService do
let_it_be(:merge_request) { create(:merge_request) }
let(:service_object) { described_class.new(merge_request.target_project, merge_request.author) }
describe '#execute' do
subject(:execute) { service_object.execute(merge_request) }
before do
allow(SyncSecurityReportsToReportApprovalRulesWorker).to receive(:perform_async)
end
context 'when the merge request has actual_head_pipeline' do
let(:pipeline_id) { 1881 }
before do
allow(merge_request).to receive(:head_pipeline_id).and_return(pipeline_id)
allow(merge_request).to receive(:update_head_pipeline).and_return(true)
end
it 'schedules a background job to sync security reports to approval rules' do
execute
expect(merge_request).to have_received(:update_head_pipeline).ordered
expect(SyncSecurityReportsToReportApprovalRulesWorker).to have_received(:perform_async).ordered.with(pipeline_id)
end
end
context 'when the merge request does not have actual_head_pipeline' do
it 'does not schedule a background job to sync security reports to approval rules' do
execute
expect(SyncSecurityReportsToReportApprovalRulesWorker).not_to have_received(:perform_async)
end
end
end
end
...@@ -39,9 +39,6 @@ RSpec.describe MergeRequests::CreateService do ...@@ -39,9 +39,6 @@ RSpec.describe MergeRequests::CreateService do
end end
context 'report approvers' do context 'report approvers' do
let(:sha) { project.repository.commits(opts[:source_branch], limit: 1).first.id }
let(:pipeline) { instance_double(Ci::Pipeline, id: 42, project_id: project.id, merge_request?: true) }
it 'refreshes report approvers for the merge request' do it 'refreshes report approvers for the merge request' do
expect_next_instance_of(::MergeRequests::SyncReportApproverApprovalRules) do |service| expect_next_instance_of(::MergeRequests::SyncReportApproverApprovalRules) do |service|
expect(service).to receive(:execute) expect(service).to receive(:execute)
...@@ -49,24 +46,6 @@ RSpec.describe MergeRequests::CreateService do ...@@ -49,24 +46,6 @@ RSpec.describe MergeRequests::CreateService do
service.execute service.execute
end end
it 'enqueues approval rule report syncing when pipeline exists' do
expect_next_instance_of(MergeRequest) do |merge_request|
allow(merge_request).to receive(:find_actual_head_pipeline).and_return(pipeline)
allow(merge_request).to receive(:update_head_pipeline).and_return(true)
end
expect(::SyncSecurityReportsToReportApprovalRulesWorker)
.to receive(:perform_async)
service.execute
end
it 'wont enqueue approval rule report syncing without pipeline' do
expect(::SyncSecurityReportsToReportApprovalRulesWorker)
.not_to receive(:perform_async)
service.execute
end
end end
it_behaves_like 'new issuable with scoped labels' do it_behaves_like 'new issuable with scoped labels' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment