Seperate responsibilities between (Branch|Tag)PushService and and Git::(Branch|Tag)HooksService
In gitlab-foss!31653 (comment 202209839) @DouweM identified that the line separating the responsibilities of the (Branch|Tag)PushService
and the Git::(Branch|Tag)HooksService
is quite blurry.
We should probably clean that up a little bit.
After gitlab-foss!31653 (merged) and gitlab-foss!31641 (merged) are merged, I believe the tree would look roughly like this:
-
PostReceive#process_project_changes
-
post_received.project.repository.expire_branches_cache if post_received.includes_branches?
(cache expiry) -
post_received.project.repository.expire_caches_for_tags if post_received.includes_tags?
(cache expiry) -
changes.each
-
BranchPushService#execute
-
enqueue_update_mrs
(async side effect) -
enqueue_detect_repository_languages
(async side effect` -
execute_related_hooks
(Git::BranchHooksService#execute
)-
execute_branch_hooks
-
Repository#after_push_commit(branch_name)
(cache expiry, event tracking) -
branch_(create|update|change|remove)_hooks
(cache expiry: some no-op, event tracking, side effects) -
Git::BaseHooksService#execute
-
repository.after_create
(cache expiry) -
create_events
(side effect) -
create_pipelines
(side effect) -
execute_project_hooks
(side effect) -
enqueue_invalidate_cache
(cache invalidation, but requires list of commits) -
update_remote_mirrors
(side effect)
-
-
-
-
-
TagPushService#execute
-
Repository#before_tag_push
(event tracking) -
Git::TagHooksService#execute
->Git::BaseHooksService
-
repository.after_create
(cache expiry) -
create_events
(side effect) -
create_pipelines
(side effect) -
execute_project_hooks
(side effect) -
enqueue_invalidate_cache
(cache invalidation, but requires list of commits) -
update_remote_mirrors
(side effect)
-
-
-
PostReceive#after_project_changes_hooks
-
I think we should add all cache expiry calls that relate to 1 tag or branch into the PushService
, while all side effects that require loading commits and so on into the hooks.
The following discussion from gitlab-foss!31653 (merged) should be addressed:
-
@reprazent started a discussion: (+6 comments)
Even though we're not invalidating caches in the
BranchPushService
anymore, I think we should call theBranchPushService#execute_related_hooks
before enqueuing anything else.We know now that there's nothing else in the
Repository#after_create_branch
andRepository#after_remove_branch
, but that could change, and we're also handling theRepository#after_create
in the hooks. So I think we have to assume that everything scheduled expects the hooks to have run.