Migrate `WebHookWorker` to use `params` hash arg
About
Workers that accept a params
argument make it much easier to add and remove new arguments to workers. Otherwise, adding or removing arguments is a muti-milestone effort.
!75821 (merged) introduced a params
arg and #347389 (closed) is in the process of using it for the recursion detection UUID value. After that issue is complete, the signature of the .perform
will be:
perform(hook_id, data, hook_name, params = {})
Proposal
We could additionally migrate hook_name
to params
also, making the final signature of .perform
:
perform(hook_id, data, params = {})
With params
containing hook_name
and recursion_detection_uuid
keys.
We need to keep data
as a separate argument, as we use loggable_arguments
to exclude that argument from being logged due to the possibility for sensitive information to be in the webhook payload. We possibly could move hook_id
to params
also - but it seems like a fundamental argument to accept, so it's not being proposed.
Timeframe
The timeframes given in the Deprecate and remove an argument docs for GitLab Sidekiq workers are to do with removing an argument in 3 milestones, but I think we can move an argument in 2:
%"14.12" or earlier
We bridge support for either:
diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb
index a4342aa5b90..e7c219e6b97 100644
--- a/app/services/web_hook_service.rb
+++ b/app/services/web_hook_service.rb
@@ -103,10 +103,11 @@ def async_execute
break log_recursion_blocked if recursion_blocked?
params = {
+ hook_name: hook_name,
recursion_detection_request_uuid: Gitlab::WebHooks::RecursionDetection::UUID.instance.request_uuid
}.compact
- WebHookWorker.perform_async(hook.id, data, hook_name, params)
+ WebHookWorker.perform_async(hook.id, data, nil, params)
end
end
diff --git a/app/workers/web_hook_worker.rb b/app/workers/web_hook_worker.rb
index fdcd22128a3..04b6c26ae86 100644
--- a/app/workers/web_hook_worker.rb
+++ b/app/workers/web_hook_worker.rb
@@ -16,13 +16,15 @@ class WebHookWorker
# Webhook recursion detection properties may be passed through the `data` arg.
# This will be migrated to the `params` arg over the next few releases.
# See https://gitlab.com/gitlab-org/gitlab/-/issues/347389.
- def perform(hook_id, data, hook_name, params = {})
+ def perform(hook_id, data, hook_name = nil, params = {})
hook = WebHook.find_by_id(hook_id)
return unless hook
data = data.with_indifferent_access
params.symbolize_keys!
+ hook_name ||= params.delete(:hook_name)
+
# TODO: Remove in 14.9 https://gitlab.com/gitlab-org/gitlab/-/issues/347389
params[:recursion_detection_request_uuid] ||= data.delete(:_gitlab_recursion_detection_request_uuid)
%15.0
We make a breaking change in a major release (as per our docs):
diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb
index a4342aa5b90..e81fbae982e 100644
--- a/app/services/web_hook_service.rb
+++ b/app/services/web_hook_service.rb
@@ -103,10 +103,11 @@ def async_execute
break log_recursion_blocked if recursion_blocked?
params = {
hook_name: hook_name,
recursion_detection_request_uuid: Gitlab::WebHooks::RecursionDetection::UUID.instance.request_uuid
}.compact
- WebHookWorker.perform_async(hook.id, data, hook_name, params)
+ WebHookWorker.perform_async(hook.id, data, params)
end
end
diff --git a/app/workers/web_hook_worker.rb b/app/workers/web_hook_worker.rb
index fdcd22128a3..439b1573d9b 100644
--- a/app/workers/web_hook_worker.rb
+++ b/app/workers/web_hook_worker.rb
@@ -16,13 +16,15 @@ class WebHookWorker
# Webhook recursion detection properties may be passed through the `data` arg.
# This will be migrated to the `params` arg over the next few releases.
# See https://gitlab.com/gitlab-org/gitlab/-/issues/347389.
- def perform(hook_id, data, hook_name, params = {})
+ def perform(hook_id, data, params = {})
hook = WebHook.find_by_id(hook_id)
return unless hook
data = data.with_indifferent_access
params.symbolize_keys!
- hook_name ||= params.delete(:hook_name)
+ hook_name = params.delete(:hook_name)
# TODO: Remove in 14.9 https://gitlab.com/gitlab-org/gitlab/-/issues/347389
params[:recursion_detection_request_uuid] ||= data.delete(:_gitlab_recursion_detection_request_uuid)