From 6fc129fe62d12cf702397ae9bc55778ac3abd997 Mon Sep 17 00:00:00 2001 From: James Lakin Date: Fri, 2 Oct 2020 09:37:46 +0000 Subject: [PATCH] Fix repository create/delete event webhooks (#13008) This small PR changes the webhook trigger behaviour to be more in line with what's expected. (When 'repository' events are enabled, of course) In other words: For system-wide or default webhooks, repository events will now trigger said webhook. Previously it had to be under an organization for create events to be visible - a tad unexpected! Deleting a repository will now fire its own defined webhooks, not just organisational and system ones. In order to enable the latter the webhook has to now be triggered before the actual repo undergoes deletion. I'm willing to tweak this to try and 'grab' the webhook model beforehand and trigger the webhook notifier directly afterwards, but this may make the code more complex for little benefit. Closes #11766, #9180. --- modules/notification/webhook/webhook.go | 32 +++++++++++-------------- services/repository/repository.go | 8 +++---- 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/modules/notification/webhook/webhook.go b/modules/notification/webhook/webhook.go index 625cf119a..c8b6bae72 100644 --- a/modules/notification/webhook/webhook.go +++ b/modules/notification/webhook/webhook.go @@ -99,30 +99,26 @@ func (m *webhookNotifier) NotifyForkRepository(doer *models.User, oldRepo, repo func (m *webhookNotifier) NotifyCreateRepository(doer *models.User, u *models.User, repo *models.Repository) { // Add to hook queue for created repo after session commit. - if u.IsOrganization() { - if err := webhook_module.PrepareWebhooks(repo, models.HookEventRepository, &api.RepositoryPayload{ - Action: api.HookRepoCreated, - Repository: repo.APIFormat(models.AccessModeOwner), - Organization: u.APIFormat(), - Sender: doer.APIFormat(), - }); err != nil { - log.Error("PrepareWebhooks [repo_id: %d]: %v", repo.ID, err) - } + if err := webhook_module.PrepareWebhooks(repo, models.HookEventRepository, &api.RepositoryPayload{ + Action: api.HookRepoCreated, + Repository: repo.APIFormat(models.AccessModeOwner), + Organization: u.APIFormat(), + Sender: doer.APIFormat(), + }); err != nil { + log.Error("PrepareWebhooks [repo_id: %d]: %v", repo.ID, err) } } func (m *webhookNotifier) NotifyDeleteRepository(doer *models.User, repo *models.Repository) { u := repo.MustOwner() - if u.IsOrganization() { - if err := webhook_module.PrepareWebhooks(repo, models.HookEventRepository, &api.RepositoryPayload{ - Action: api.HookRepoDeleted, - Repository: repo.APIFormat(models.AccessModeOwner), - Organization: u.APIFormat(), - Sender: doer.APIFormat(), - }); err != nil { - log.Error("PrepareWebhooks [repo_id: %d]: %v", repo.ID, err) - } + if err := webhook_module.PrepareWebhooks(repo, models.HookEventRepository, &api.RepositoryPayload{ + Action: api.HookRepoDeleted, + Repository: repo.APIFormat(models.AccessModeOwner), + Organization: u.APIFormat(), + Sender: doer.APIFormat(), + }); err != nil { + log.Error("PrepareWebhooks [repo_id: %d]: %v", repo.ID, err) } } diff --git a/services/repository/repository.go b/services/repository/repository.go index 88b162393..20d630424 100644 --- a/services/repository/repository.go +++ b/services/repository/repository.go @@ -64,13 +64,11 @@ func DeleteRepository(doer *models.User, repo *models.Repository) error { log.Error("CloseRepoBranchesPulls failed: %v", err) } - if err := models.DeleteRepository(doer, repo.OwnerID, repo.ID); err != nil { - return err - } - + // If the repo itself has webhooks, we need to trigger them before deleting it... notification.NotifyDeleteRepository(doer, repo) - return nil + err := models.DeleteRepository(doer, repo.OwnerID, repo.ID) + return err } // PushCreateRepo creates a repository when a new repository is pushed to an appropriate namespace