diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb
index 3320c13e87bc5c0bf50b8eaf681c95e44dc88f0f..7e538238cbd133f348fac8bd922d703207015d73 100644
--- a/app/models/hooks/web_hook.rb
+++ b/app/models/hooks/web_hook.rb
@@ -50,8 +50,9 @@ def permanently_disabled?
end
# rubocop: disable CodeReuse/ServiceClass
- def execute(data, hook_name)
- WebHookService.new(self, data, hook_name).execute if executable?
+ def execute(data, hook_name, force: false)
+ # hook.executable? is checked in WebHookService#execute
+ WebHookService.new(self, data, hook_name, force: force).execute
end
# rubocop: enable CodeReuse/ServiceClass
diff --git a/app/services/test_hooks/base_service.rb b/app/services/test_hooks/base_service.rb
index 0fda6fb1ed074567c74d8aca808f7edb9d2d54da..b41a9959c138a202a1c947fe5e20353fde2a1112 100644
--- a/app/services/test_hooks/base_service.rb
+++ b/app/services/test_hooks/base_service.rb
@@ -18,7 +18,7 @@ def execute
return error('Testing not available for this hook') if trigger_key.nil? || data.blank?
return error(data[:error]) if data[:error].present?
- hook.execute(data, trigger_key)
+ hook.execute(data, trigger_key, force: true)
end
end
end
diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb
index a4342aa5b90436edd0800d4c2dc49079bf1d7692..b1d8872aa5e56dc50d6fe7f0182e305ead661753 100644
--- a/app/services/web_hook_service.rb
+++ b/app/services/web_hook_service.rb
@@ -34,11 +34,12 @@ def self.hook_to_event(hook_name)
hook_name.to_s.singularize.titleize
end
- def initialize(hook, data, hook_name, uniqueness_token = nil)
+ def initialize(hook, data, hook_name, uniqueness_token = nil, force: false)
@hook = hook
@data = data
@hook_name = hook_name.to_s
@uniqueness_token = uniqueness_token
+ @force = force
@request_options = {
timeout: Gitlab.config.gitlab.webhook_timeout,
use_read_total_timeout: true,
@@ -46,8 +47,12 @@ def initialize(hook, data, hook_name, uniqueness_token = nil)
}
end
+ def disabled?
+ !@force && !hook.executable?
+ end
+
def execute
- return { status: :error, message: 'Hook disabled' } unless hook.executable?
+ return { status: :error, message: 'Hook disabled' } if disabled?
if recursion_blocked?
log_recursion_blocked
@@ -148,8 +153,12 @@ def log_execution(trigger:, url:, request_data:, response:, execution_duration:,
internal_error_message: error_message
}
- ::WebHooks::LogExecutionWorker
- .perform_async(hook.id, log_data, category, uniqueness_token)
+ if @force # executed as part of test - run log-execution inline.
+ ::WebHooks::LogExecutionService.new(hook: hook, log_data: log_data, response_category: category).execute
+ else
+ ::WebHooks::LogExecutionWorker
+ .perform_async(hook.id, log_data, category, uniqueness_token)
+ end
end
def response_category(response)
diff --git a/spec/models/hooks/service_hook_spec.rb b/spec/models/hooks/service_hook_spec.rb
index 85f433f5f814b72169832ca78be0842e13b7fddb..0d65fe302e184c99aa21952c72d389171bd521fd 100644
--- a/spec/models/hooks/service_hook_spec.rb
+++ b/spec/models/hooks/service_hook_spec.rb
@@ -16,7 +16,7 @@
let(:data) { { key: 'value' } }
it '#execute' do
- expect(WebHookService).to receive(:new).with(hook, data, 'service_hook').and_call_original
+ expect(WebHookService).to receive(:new).with(hook, data, 'service_hook', force: false).and_call_original
expect_any_instance_of(WebHookService).to receive(:execute)
hook.execute(data)
diff --git a/spec/models/hooks/system_hook_spec.rb b/spec/models/hooks/system_hook_spec.rb
index 89bfb742f5d1a345cb7838e8faf1853504e58be6..a3d36058b743b8f5f2383dc5581b543cf47369a6 100644
--- a/spec/models/hooks/system_hook_spec.rb
+++ b/spec/models/hooks/system_hook_spec.rb
@@ -168,17 +168,17 @@
let(:data) { { key: 'value' } }
let(:hook_name) { 'system_hook' }
- before do
- expect(WebHookService).to receive(:new).with(hook, data, hook_name).and_call_original
- end
-
it '#execute' do
+ expect(WebHookService).to receive(:new).with(hook, data, hook_name, force: false).and_call_original
+
expect_any_instance_of(WebHookService).to receive(:execute)
hook.execute(data, hook_name)
end
it '#async_execute' do
+ expect(WebHookService).to receive(:new).with(hook, data, hook_name).and_call_original
+
expect_any_instance_of(WebHookService).to receive(:async_execute)
hook.async_execute(data, hook_name)
diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb
index c292e78b32d87c9592dce41503fdc4db0c766c4e..482e372543cd37cb2e11fb55f16de88c6f81b0dc 100644
--- a/spec/models/hooks/web_hook_spec.rb
+++ b/spec/models/hooks/web_hook_spec.rb
@@ -100,12 +100,18 @@
hook.execute(data, hook_name)
end
- it 'does not execute non-executable hooks' do
- hook.update!(disabled_until: 1.day.from_now)
+ it 'passes force: false to the web hook service by default' do
+ expect(WebHookService)
+ .to receive(:new).with(hook, data, hook_name, force: false).and_return(double(execute: :done))
- expect(WebHookService).not_to receive(:new)
+ expect(hook.execute(data, hook_name)).to eq :done
+ end
- hook.execute(data, hook_name)
+ it 'passes force: true to the web hook service if required' do
+ expect(WebHookService)
+ .to receive(:new).with(hook, data, hook_name, force: true).and_return(double(execute: :forced))
+
+ expect(hook.execute(data, hook_name, force: true)).to eq :forced
end
it '#async_execute' do
diff --git a/spec/services/test_hooks/project_service_spec.rb b/spec/services/test_hooks/project_service_spec.rb
index cd6284b4a87d38da84d48da4bbca93e32dc67e45..d97a6f15270b88e93935cdd9791780714c4d2234 100644
--- a/spec/services/test_hooks/project_service_spec.rb
+++ b/spec/services/test_hooks/project_service_spec.rb
@@ -37,7 +37,7 @@
it 'executes hook' do
allow(Gitlab::DataBuilder::Push).to receive(:build_sample).and_return(sample_data)
- expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+ expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
expect(service.execute).to include(success_result)
end
end
@@ -49,7 +49,7 @@
it 'executes hook' do
allow(Gitlab::DataBuilder::Push).to receive(:build_sample).and_return(sample_data)
- expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+ expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
expect(service.execute).to include(success_result)
end
end
@@ -69,7 +69,7 @@
allow(Gitlab::DataBuilder::Note).to receive(:build).and_return(sample_data)
allow_next(NotesFinder).to receive(:execute).and_return(Note.all)
- expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+ expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
expect(service.execute).to include(success_result)
end
end
@@ -86,7 +86,7 @@
allow(issue).to receive(:to_hook_data).and_return(sample_data)
allow_next(IssuesFinder).to receive(:execute).and_return([issue])
- expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+ expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
expect(service.execute).to include(success_result)
end
end
@@ -119,7 +119,7 @@
allow(merge_request).to receive(:to_hook_data).and_return(sample_data)
allow_next(MergeRequestsFinder).to receive(:execute).and_return([merge_request])
- expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+ expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
expect(service.execute).to include(success_result)
end
end
@@ -138,7 +138,7 @@
allow(Gitlab::DataBuilder::Build).to receive(:build).and_return(sample_data)
allow_next(Ci::JobsFinder).to receive(:execute).and_return([ci_job])
- expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+ expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
expect(service.execute).to include(success_result)
end
end
@@ -157,7 +157,7 @@
allow(Gitlab::DataBuilder::Pipeline).to receive(:build).and_return(sample_data)
allow_next(Ci::PipelinesFinder).to receive(:execute).and_return([pipeline])
- expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+ expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
expect(service.execute).to include(success_result)
end
end
@@ -184,7 +184,7 @@
create(:wiki_page, wiki: project.wiki)
allow(Gitlab::DataBuilder::WikiPage).to receive(:build).and_return(sample_data)
- expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+ expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
expect(service.execute).to include(success_result)
end
end
@@ -203,7 +203,7 @@
allow(release).to receive(:to_hook_data).and_return(sample_data)
allow_next(ReleasesFinder).to receive(:execute).and_return([release])
- expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+ expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
expect(service.execute).to include(success_result)
end
end
diff --git a/spec/services/test_hooks/system_service_spec.rb b/spec/services/test_hooks/system_service_spec.rb
index 48c8c24212ad1b1c175e5bd5513cb819a6018c5a..66a1218d123e1702281af1c6af0c592b5c04744a 100644
--- a/spec/services/test_hooks/system_service_spec.rb
+++ b/spec/services/test_hooks/system_service_spec.rb
@@ -32,7 +32,7 @@
it 'executes hook' do
expect(Gitlab::DataBuilder::Push).to receive(:sample_data).and_call_original
- expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Push::SAMPLE_DATA, trigger_key).and_return(success_result)
+ expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Push::SAMPLE_DATA, trigger_key, force: true).and_return(success_result)
expect(service.execute).to include(success_result)
end
end
@@ -45,7 +45,7 @@
allow(project.repository).to receive(:tags).and_return(['tag'])
expect(Gitlab::DataBuilder::Push).to receive(:sample_data).and_call_original
- expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Push::SAMPLE_DATA, trigger_key).and_return(success_result)
+ expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Push::SAMPLE_DATA, trigger_key, force: true).and_return(success_result)
expect(service.execute).to include(success_result)
end
end
@@ -57,7 +57,7 @@
it 'executes hook' do
expect(Gitlab::DataBuilder::Repository).to receive(:sample_data).and_call_original
- expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Repository::SAMPLE_DATA, trigger_key).and_return(success_result)
+ expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Repository::SAMPLE_DATA, trigger_key, force: true).and_return(success_result)
expect(service.execute).to include(success_result)
end
end
@@ -76,7 +76,7 @@
it 'executes hook' do
expect(MergeRequest).to receive(:of_projects).and_return([merge_request])
expect(merge_request).to receive(:to_hook_data).and_return(sample_data)
- expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+ expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result)
expect(service.execute).to include(success_result)
end
end
diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb
index 448d00076c1efc0bbfa8533e01222d710e1923d5..64371f979086ecb495963936663f38fde5876b3b 100644
--- a/spec/services/web_hook_service_spec.rb
+++ b/spec/services/web_hook_service_spec.rb
@@ -52,6 +52,25 @@
end
end
+ describe '#disabled?' do
+ using RSpec::Parameterized::TableSyntax
+
+ subject { described_class.new(hook, data, :push_hooks, force: forced) }
+
+ let(:hook) { double(executable?: executable, allow_local_requests?: false) }
+
+ where(:forced, :executable, :disabled) do
+ false | true | false
+ false | false | true
+ true | true | false
+ true | false | false
+ end
+
+ with_them do
+ it { is_expected.to have_attributes(disabled?: disabled) }
+ end
+ end
+
describe '#execute' do
let!(:uuid) { SecureRandom.uuid }
let(:headers) do
@@ -129,7 +148,7 @@
end
it 'does not execute disabled hooks' do
- project_hook.update!(recent_failures: 4)
+ allow(service_instance).to receive(:disabled?).and_return(true)
expect(service_instance.execute).to eq({ status: :error, message: 'Hook disabled' })
end
@@ -251,6 +270,20 @@ def run_service
stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: 'Success')
end
+ context 'when forced' do
+ let(:service_instance) { described_class.new(project_hook, data, :push_hooks, force: true) }
+
+ it 'logs execution inline' do
+ expect(::WebHooks::LogExecutionWorker).not_to receive(:perform_async)
+ expect(::WebHooks::LogExecutionService)
+ .to receive(:new)
+ .with(hook: project_hook, log_data: Hash, response_category: :ok)
+ .and_return(double(execute: nil))
+
+ run_service
+ end
+ end
+
it 'log successful execution' do
run_service