diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index 6be974a1a0437c2854b71f2111d64f0d945b914e..3e838f2ca3d7576889f9fc0d907bcfafb1951715 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -45,9 +45,6 @@ def refresh_merge_requests! notify_about_push(mr) mark_mr_as_draft_from_commits(mr) - # Call merge request webhook with update branches - execute_mr_web_hooks(mr) if Feature.disabled?(:split_refresh_worker_web_hooks, @current_user) - if Feature.disabled?(:split_refresh_worker_pipeline, @current_user) && !@push.branch_removed? # Run at the end of the loop to avoid any potential contention on the MR object refresh_pipelines_on_merge_requests(mr) @@ -359,15 +356,13 @@ def schedule_duo_code_review(merge_request) end def execute_async_workers - if Feature.enabled?(:split_refresh_worker_web_hooks, @current_user) - MergeRequests::Refresh::WebHooksWorker.perform_async( - @project.id, - @current_user.id, - @push.oldrev, - @push.newrev, - @push.ref - ) - end + MergeRequests::Refresh::WebHooksWorker.perform_async( + @project.id, + @current_user.id, + @push.oldrev, + @push.newrev, + @push.ref + ) if Feature.enabled?(:split_refresh_worker_pipeline, @current_user) MergeRequests::Refresh::PipelineWorker.perform_async( diff --git a/config/feature_flags/beta/split_refresh_worker_web_hooks.yml b/config/feature_flags/beta/split_refresh_worker_web_hooks.yml deleted file mode 100644 index d26dac26d8198c442e426ee2923f2e9fce91e51d..0000000000000000000000000000000000000000 --- a/config/feature_flags/beta/split_refresh_worker_web_hooks.yml +++ /dev/null @@ -1,10 +0,0 @@ ---- -name: split_refresh_worker_web_hooks -description: Split web hooks execution from MergeRequests::RefreshService into separate worker -feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/554081 -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/203215 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/569485 -milestone: '18.4' -group: group::code review -type: beta -default_enabled: true diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index d993abc26899633bcb7f405ca62a1b6624f11a02..4d06bd53648486566c96611a1e716781fdf706cc 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -10,13 +10,11 @@ let(:user) { create(:user) } let(:service) { described_class } - let(:webhook_ff) { false } let(:pipeline_ff) { false } describe '#execute' do before do stub_feature_flags( - split_refresh_worker_web_hooks: webhook_ff, split_refresh_worker_pipeline: pipeline_ff ) @@ -86,7 +84,6 @@ let(:notification_service) { spy('notification_service') } before do - allow(refresh_service).to receive(:execute_hooks) allow(NotificationService).to receive(:new) { notification_service } end @@ -105,13 +102,10 @@ end end - it 'executes hooks with update action' do + it 'refreshes the merge requests' do refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') reload_mrs - expect(refresh_service).to have_received(:execute_hooks) - .with(@merge_request, 'update', old_rev: @oldrev) - expect(notification_service).to have_received(:push_to_merge_request) .with(@merge_request, @user, new_commits: anything, existing_commits: anything) expect(notification_service).to have_received(:push_to_merge_request) @@ -127,27 +121,25 @@ expect(@fork_build_failed_todo).to be_done end - context 'when split_refresh_worker_web_hooks is on' do - let(:webhook_ff) { true } + it 'calls the web hooks worker async' do + expect(MergeRequests::Refresh::WebHooksWorker).to receive(:perform_async) + .with( + @project.id, + @user.id, + @oldrev, + @newrev, + 'refs/heads/master' + ) - it 'calls the web hooks worker async' do - expect(MergeRequests::Refresh::WebHooksWorker).to receive(:perform_async) - .with( - @project.id, - @user.id, - @oldrev, - @newrev, - 'refs/heads/master' - ) + refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') + end - refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') - end + it 'does not execute the web hooks' do + allow(refresh_service).to receive(:execute_hooks) - it 'does not execute the web hooks' do - refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') + refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') - expect(refresh_service).not_to have_received(:execute_hooks) - end + expect(refresh_service).not_to have_received(:execute_hooks) end it 'triggers mergeRequestMergeStatusUpdated GraphQL subscription conditionally' do @@ -509,15 +501,12 @@ let(:notification_service) { spy('notification_service') } before do - allow(refresh_service).to receive(:execute_hooks) allow(NotificationService).to receive(:new) { notification_service } refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') reload_mrs end - it 'executes hooks with update action' do - expect(refresh_service).to have_received(:execute_hooks) - .with(@merge_request, 'update', old_rev: @oldrev) + it 'refreshes the merge requests' do expect(notification_service).to have_received(:push_to_merge_request) .with(@merge_request, @user, new_commits: anything, existing_commits: anything) expect(notification_service).to have_received(:push_to_merge_request) @@ -657,7 +646,6 @@ let(:refresh_service) { service.new(project: @fork_project, current_user: @user) } def refresh - allow(refresh_service).to receive(:execute_hooks) refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') reload_mrs end @@ -671,12 +659,9 @@ def refresh refresh end - it 'executes hooks with update action' do + it 'refreshes the merge requests' do refresh - expect(refresh_service).to have_received(:execute_hooks) - .with(@fork_merge_request, 'update', old_rev: @oldrev) - expect(@merge_request.notes).to be_empty expect(@merge_request).to be_open expect(@fork_merge_request.notes.last.note).to include('added 28 commits') @@ -699,12 +684,6 @@ def refresh @fork_merge_request.close! end - it 'do not execute hooks with update action' do - refresh - - expect(refresh_service).not_to have_received(:execute_hooks) - end - it 'updates merge request to closed state' do refresh @@ -843,8 +822,6 @@ def refresh let(:refresh_service) { service.new(project: @fork_project, current_user: @user) } it 'refreshes the merge request' do - expect(refresh_service).to receive(:execute_hooks) - .with(@fork_merge_request, 'update', old_rev: Gitlab::Git::SHA1_BLANK_SHA) allow_any_instance_of(Repository).to receive(:merge_base).and_return(@oldrev) refresh_service.execute(Gitlab::Git::SHA1_BLANK_SHA, @newrev, 'refs/heads/master') @@ -892,7 +869,6 @@ def refresh ) refresh_service = service.new(project: project, current_user: user) - allow(refresh_service).to receive(:execute_hooks) refresh_service.execute(@oldrev, @newrev, 'refs/heads/close-by-commit') expect(MergeRequestsClosingIssues.where(merge_request: merge_request)).to contain_exactly( @@ -918,7 +894,6 @@ def refresh ) refresh_service = service.new(project: forked_project, current_user: user) - allow(refresh_service).to receive(:execute_hooks) refresh_service.execute(@oldrev, @newrev, 'refs/heads/close-by-commit') expect(MergeRequestsClosingIssues.where(merge_request: merge_request)).to contain_exactly( @@ -931,10 +906,6 @@ def refresh context 'marking the merge request as draft' do let(:refresh_service) { service.new(project: @project, current_user: @user) } - before do - allow(refresh_service).to receive(:execute_hooks) - end - it 'marks the merge request as draft from fixup commits' do fixup_merge_request = create( :merge_request,