From 377e3670a9aa2ca1419232d75b060cfccc58c7ef Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 21 Aug 2018 13:32:50 -0700 Subject: Eliminate unnecessary and duplicate system hook fires Previously `SystemHookPushWorker` would always be called after a push event, and this would queue a Sidekiq job regardless of whether any system hooks needed that event. Moreover, another call inside `Project#execute_hooks` would also fire system hooks if they existed. This change both removes the duplicate system hook calls. For installations without system hooks for push events, this change also can save significant amount of RAM used by Redis. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/50549 --- app/services/git_push_service.rb | 1 - changelogs/unreleased/sh-conditional-system-hook-push.yml | 5 +++++ spec/services/git_push_service_spec.rb | 10 +++++++--- 3 files changed, 12 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/sh-conditional-system-hook-push.yml diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index 637c1df4ad9..156b4b3c6ec 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -140,7 +140,6 @@ class GitPushService < BaseService EventCreateService.new.push(project, current_user, build_push_data) Ci::CreatePipelineService.new(project, current_user, build_push_data).execute(:push) - SystemHookPushWorker.perform_async(build_push_data.dup, :push_hooks) project.execute_hooks(build_push_data.dup, :push_hooks) project.execute_services(build_push_data.dup, :push_hooks) diff --git a/changelogs/unreleased/sh-conditional-system-hook-push.yml b/changelogs/unreleased/sh-conditional-system-hook-push.yml new file mode 100644 index 00000000000..3a1a1b3d36c --- /dev/null +++ b/changelogs/unreleased/sh-conditional-system-hook-push.yml @@ -0,0 +1,5 @@ +--- +title: Eliminate unnecessary and duplicate system hook fires +merge_request: 21337 +author: +type: performance diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index a3c9a660c2f..ed0088a9500 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -223,9 +223,13 @@ describe GitPushService, services: true do end end - context "Sends System Push data" do - it "when pushing on a branch" do - expect(SystemHookPushWorker).to receive(:perform_async).with(push_data, :push_hooks) + describe 'system hooks' do + let(:system_hook_service) { double() } + + it "sends a system hook after pushing a branch" do + expect(SystemHooksService).to receive(:new).and_return(system_hook_service) + expect(system_hook_service).to receive(:execute_hooks).with(push_data, :push_hooks) + execute_service(project, user, oldrev, newrev, ref) end end -- cgit v1.2.3