Skip to content

Delete only HookTasks of the repository #17981

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Delete only HookTasks of the repository #17981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions models/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -766,7 +766,6 @@ func DeleteRepository(doer *user_model.User, uid, repoID int64) error {
&Comment{RefRepoID: repoID},
&CommitStatus{RepoID: repoID},
&DeletedBranch{RepoID: repoID},
&webhook.HookTask{RepoID: repoID},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But a HookTask belongs to this repository which produced because of an org webhook should also be deleted? Otherwise we will have dirty records?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A HookTask belongs to a Webhook. If a system or organization webhook generates a HookTask I would be surprised if this history entry of the webhook gets deleted just because the repository was deleted. The entry should only be deleted if the system/org webhook gets deleted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hook_task should belong to webhook but also repository. That means, deleting webhook will delete all webhook generated hook tasks. Deleting repository will delete all hook tasks for the repository.

For an individual repository, a webhook will generate hooktask for only one repository. So there is no problem whatever delete repository or delete webhook, we could delete all hooktask according webhook id. But for organization repository, the webhook maybe created and stay a long time(maybe forever? :)). But some repositories created and some repositories deleted. (btw: a possible bug maybe when create a new repository, the system webhook and organization webhook didn't apply into them, I have to check.) So maybe you have never chance to delete all these hook tasks.

Copy link
Member Author

@KN4CK3R KN4CK3R Dec 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hook_task should belong to webhook but also repository. That means, deleting webhook will delete all webhook generated hook tasks. Deleting repository will delete all hook tasks for the repository.

Same is true for an issue comment but you do not delete the comment just because you delete the user. The comment is related to the issue like the hooktask to the webhook. The history entry of a webhook should not get deleted just because an unused reference is deleted. You don't remove the system notices of a repository just because the repository isn't there anymore.

But for organization repository, the webhook maybe created and stay a long time(maybe forever? :)).

Webhooks could stay a long time. The hook tasks of a webhook live up to 7 days. There is a cronjob which deletes them.

&LFSLock{RepoID: repoID},
&repo_model.LanguageStat{RepoID: repoID},
&Milestone{RepoID: repoID},
Expand All @@ -783,11 +782,21 @@ func DeleteRepository(doer *user_model.User, uid, repoID int64) error {
&repo_model.Star{RepoID: repoID},
&Task{RepoID: repoID},
&repo_model.Watch{RepoID: repoID},
&webhook.Webhook{RepoID: repoID},
); err != nil {
return fmt.Errorf("deleteBeans: %v", err)
}

// Delete Webhooks and related objects
hooks, err := webhook.ListWebhooksByOptsCtx(ctx, &webhook.ListWebhookOptions{RepoID: repoID})
if err != nil {
return err
}
for _, hook := range hooks {
if err := webhook.DeleteWebhook(ctx, hook); err != nil {
return err
}
}

// Delete Labels and related objects
if err := deleteLabelsByRepoID(sess, repoID); err != nil {
return err
Expand Down
41 changes: 19 additions & 22 deletions models/webhook/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,8 +429,10 @@ func (opts *ListWebhookOptions) toCond() builder.Cond {
return cond
}

func listWebhooksByOpts(e db.Engine, opts *ListWebhookOptions) ([]*Webhook, error) {
sess := e.Where(opts.toCond())
// ListWebhooksByOptsCtx gets webhooks based on options
func ListWebhooksByOptsCtx(ctx context.Context, opts *ListWebhookOptions) ([]*Webhook, error) {
sess := db.GetEngine(ctx).
Where(opts.toCond())

if opts.Page != 0 {
sess = db.SetSessionPagination(sess, opts)
Expand All @@ -440,13 +442,12 @@ func listWebhooksByOpts(e db.Engine, opts *ListWebhookOptions) ([]*Webhook, erro
}

webhooks := make([]*Webhook, 0, 10)
err := sess.Find(&webhooks)
return webhooks, err
return webhooks, sess.Find(&webhooks)
}

// ListWebhooksByOpts return webhooks based on options
func ListWebhooksByOpts(opts *ListWebhookOptions) ([]*Webhook, error) {
return listWebhooksByOpts(db.GetEngine(db.DefaultContext), opts)
return ListWebhooksByOptsCtx(db.DefaultContext, opts)
}

// CountWebhooksByOpts count webhooks based on options and ignore pagination
Expand Down Expand Up @@ -504,39 +505,35 @@ func UpdateWebhookLastStatus(w *Webhook) error {
return err
}

// deleteWebhook uses argument bean as query condition,
// ID must be specified and do not assign unnecessary fields.
func deleteWebhook(bean *Webhook) (err error) {
ctx, committer, err := db.TxContext()
if err != nil {
return err
}
defer committer.Close()

// DeleteWebhook deletes a webhook
func DeleteWebhook(ctx context.Context, bean *Webhook) error {
if count, err := db.DeleteByBean(ctx, bean); err != nil {
return err
} else if count == 0 {
return ErrWebhookNotExist{ID: bean.ID}
} else if _, err = db.DeleteByBean(ctx, &HookTask{HookID: bean.ID}); err != nil {
return err
}

return committer.Commit()
return nil
}

// DeleteWebhookByRepoID deletes webhook of repository by given ID.
func DeleteWebhookByRepoID(repoID, id int64) error {
return deleteWebhook(&Webhook{
ID: id,
RepoID: repoID,
return db.WithTx(func(ctx context.Context) error {
return DeleteWebhook(ctx, &Webhook{
ID: id,
RepoID: repoID,
})
})
}

// DeleteWebhookByOrgID deletes webhook of organization by given ID.
func DeleteWebhookByOrgID(orgID, id int64) error {
return deleteWebhook(&Webhook{
ID: id,
OrgID: orgID,
return db.WithTx(func(ctx context.Context) error {
return DeleteWebhook(ctx, &Webhook{
ID: id,
OrgID: orgID,
})
})
}

Expand Down