Prune hook_task table #11416

bhalbright · 2020-05-15T03:38:59Z

Ref issue: #10741

At a high level, added code to do regular pruning of delivered webhooks in the hook_task table. It can be turned on/off and the schedule controlled globally via app.ini. Also can be controlled per repository via UI. By default webhooks will be on and delete all but the most recent 10 deliveres.

Added a db migration to add 2 fields to repository table: is_hook_task_purge_enabled and number_webhook_deliveries_to_keep. Defaults to true and 10.
Added fields to repository settings page so that the behavior can be controlled per repository:
Added a cron job to perform the deletions on the hook_task table. Very similar to the setup of other cron jobs, it can be turned off and the schedule controlled via app.ini. Defaults to enabled, not run on start, and every 24hours.

…e by repository

…-hook-task # Conflicts: # models/migrations/migrations.go # models/migrations/v136.go

…-hook-task

…-hook-task # Conflicts: # models/migrations/migrations.go # models/migrations/v139.go

…_purge_enabled flag

…-hook-task

…-hook-task # Conflicts: # modules/cron/cron.go # modules/setting/cron.go

bhalbright · 2020-05-17T03:52:57Z

This is ready to review

…-hook-task

lunny · 2020-09-02T12:18:38Z

Please resolve the conflicts.

lunny · 2020-09-02T14:40:31Z

Please resolve the conflicts.

Signed-off-by: Andrew Thornton <[email protected]>

zeripath · 2020-09-02T19:33:19Z

I've resolved the conflicts.

bhalbright · 2020-09-02T19:39:37Z

I've resolved the conflicts.

Thanks @zeripath

codecov-commenter · 2020-09-02T20:01:11Z

Codecov Report

Merging #11416 into master will decrease coverage by 0.00%.
The diff coverage is 32.87%.

@@            Coverage Diff             @@
##           master   #11416      +/-   ##
==========================================
- Coverage   43.08%   43.07%   -0.01%     
==========================================
  Files         658      660       +2     
  Lines       72448    72519      +71     
==========================================
+ Hits        31211    31237      +26     
- Misses      36183    36220      +37     
- Partials     5054     5062       +8

Impacted Files	Coverage Δ
models/migrations/migrations.go	`2.46% <ø> (ø)`
models/migrations/v153.go	`0.00% <0.00%> (ø)`
models/repo.go	`55.61% <ø> (+0.22%)`	⬆️
modules/auth/repo_form.go	`42.34% <ø> (ø)`
modules/repository/prune_hook_task.go	`0.00% <0.00%> (ø)`
modules/setting/repository.go	`56.36% <ø> (ø)`
routers/repo/setting.go	`14.86% <0.00%> (-0.10%)`	⬇️
services/repository/push.go	`57.92% <0.00%> (-0.65%)`	⬇️
modules/cron/tasks_basic.go	`85.41% <66.66%> (-1.94%)`	⬇️
models/webhook.go	`68.78% <80.00%> (+0.62%)`	⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7ffc67...cffae2e. Read the comment docs.

zeripath · 2020-09-02T20:40:01Z

no problem @bhalbright - sorry this has taken so long to merge.

models/webhook.go

lunny · 2020-09-09T09:39:18Z

modules/repository/prune_hook_task.go

+			repo := bean.(*models.Repository)
+			repoPath := repo.RepoPath()
+			log.Trace("Running prune hook_task table on repository %s", repoPath)
+			if err := models.DeleteDeliveredHookTasks(repo.ID, repo.NumberWebhookDeliveriesToKeep); err != nil {


This may fail on sqlite. I think use a Find but not Iterate is better.

I'm not sure I totally understand, can you elaborate? I saw Iterate usage other places like for example (if I'm understanding):
https://github.com/go-gitea/gitea/blob/master/modules/repository/check.go#L25
https://github.com/go-gitea/gitea/blob/master/modules/repository/hooks.go#L159

But I'm happy to refactor it if it is a problem.

When did some database operations on an iterate maybe affect a lock on sqlite. If other places also have the problems, they also needs to be changed.

…-hook-task # Conflicts: # models/migrations/migrations.go # models/migrations/v148.go

6543 · 2020-09-12T16:07:53Z

models/webhook_test.go

@@ -245,3 +245,39 @@ func TestUpdateHookTask(t *testing.T) {
 	assert.NoError(t, UpdateHookTask(hook))
 	AssertExistsAndLoadBean(t, hook)
 }
+
+func TestDeleteDeliveredHookTasks_DeletesDelivered(t *testing.T) {
+	assert.NoError(t, PrepareTestDatabase())


can you insert 2 and limit to 1
and check if only the right one has been deleted?

sure, I added a test where I created a new webhook that was delievered "now" and then called the delete method where 1 is kept...and then verified that the newly created webhook is still present.

6543 · 2020-09-12T16:09:53Z

modules/repository/create.go

@@ -39,6 +39,8 @@ func CreateRepository(doer, u *models.User, opts models.CreateRepoOptions) (_ *m
 		IsPrivate:                       opts.IsPrivate,
 		IsFsckEnabled:                   !opts.IsMirror,
 		CloseIssuesViaCommitInAnyBranch: setting.Repository.DefaultCloseIssuesViaCommitsInAnyBranch,
+		IsHookTaskPurgeEnabled:          setting.Repository.DefaultIsHookTaskPurgeEnabled,


do we real have to set it on repo creation?
instead of use global default until user change it?

I guess I was implementing the way "CloseIssuesViaCommitInAnyBranch" is done...but I think you are suggesting that the columns added to repository would be null by default, and would only have values if the user overrides the settings?

web_src/js/index.js

…-hook-task # Conflicts: # models/migrations/migrations.go # models/migrations/v150.go

… will leave the most recent based if 1 delivery is to be kept

6543 · 2020-09-15T23:55:36Z

@bhalbright pleace resolve conflicts :)

…-hook-task # Conflicts: # models/migrations/migrations.go # models/migrations/v151.go

bhalbright · 2020-09-16T02:56:41Z

@bhalbright pleace resolve conflicts :)

sure, done. I started working on the changes you suggested (the columns added to repository would be null by default, and would only have values if the user overrides the settings), but I don't know that I'll have enough free time to finish it this week.

6543 · 2020-09-16T03:04:14Z

@bhalbright thanks!!!

…-hook-task # Conflicts: # models/migrations/migrations.go # models/migrations/v152.go # modules/repository/generate.go

6543 · 2020-09-23T06:29:20Z

@bhalbright I think bhalbright#1 should it be

bhalbright · 2020-09-24T03:21:59Z

@6543 my apologies, I'm going to close this PR. I realized after chatting with @jag3773 that I misinterpreted the original ask, I should be deleting per webhook, not per repo. Also, I think I made this too complicated, the ability to override the number to keep per repo was likely overkill. I'm going to re-submit a PR that will allow you to set the values via the ini file and you will be able to cleanup via a simple age of the record or by delete all but a certain # per webhook. Also I think I'll put in an option to delete orphaned records in hook_task.

I'm sorry for the time everyone spent reviewing.

bhalbright added 18 commits April 13, 2020 22:39

sketching out some of the code to add ability to prune hook_task tabl…

e1c9664

…e by repository

stubbed out more code for prune hook_task

ee83fe7

work on prune hook_task

c4502dc

re-arranged on the settings page

739dfe7

got the form working for editing saving purging of webhook deliveries

2cdecc1

got the deletes running for prune hook_task

dd5a870

Merge branch 'master' of https://github.com/go-gitea/gitea into prune…

dfb2505

…-hook-task # Conflicts: # models/migrations/migrations.go # models/migrations/v136.go

added a couple unit tests; cleaned up delete routine a little

1e800f5

Merge branch 'master' of https://github.com/go-gitea/gitea into prune…

2ae404b

…-hook-task

added ENABLED setting in the ini

c016b7c

defaulting run at start to false on prune hook_task

eea3a74

Merge branch 'master' of https://github.com/go-gitea/gitea into prune…

1a56206

…-hook-task # Conflicts: # models/migrations/migrations.go # models/migrations/v139.go

added a condition to check is_hook_task_purge_enabled before deleting

66c0d8b

removing prior mistake

fde6937

fixed error in webhook tests

dd0f4a1

changed query in DeleteDeliveredHookTasks to respect the is_hook_task…

bc14884

…_purge_enabled flag

Merge branch 'master' of https://github.com/go-gitea/gitea into prune…

8a66680

…-hook-task

fixed misspelling in comment

125dc99

techknowlogick added this to the 1.13.0 milestone May 16, 2020

bhalbright added 4 commits May 16, 2020 08:47

formatting

d1db4f2

changed var declaration

fd13812

fix unit tests

a28df99

unit tests

4651030

bhalbright changed the title ~~[WIP] Prune hook_task table~~ Prune hook_task table May 17, 2020

bhalbright marked this pull request as ready for review May 17, 2020 00:05

bhalbright changed the title ~~Prune hook_task table~~ WIP Prune hook_task table May 17, 2020

bhalbright added 2 commits May 16, 2020 22:26

Merge branch 'master' of https://github.com/go-gitea/gitea into prune…

aec78da

…-hook-task # Conflicts: # modules/cron/cron.go # modules/setting/cron.go

adjusted to cron refactor; added missing string for cron page

38f6d30

bhalbright changed the title ~~WIP Prune hook_task table~~ Prune hook_task table May 17, 2020

bhalbright added 2 commits August 5, 2020 22:11

added additional information about the prune hook_task process

49ea492

Merge branch 'master' of https://github.com/go-gitea/gitea into prune…

deaac3e

…-hook-task

zeripath added 2 commits September 2, 2020 20:30

move migration out of the way

1709bc9

Signed-off-by: Andrew Thornton <[email protected]>

Merge remote-tracking branch 'origin/master' into prune-hook-task

7ebc11c

lunny reviewed Sep 4, 2020

View reviewed changes

models/webhook.go Show resolved Hide resolved

lunny reviewed Sep 9, 2020

View reviewed changes

bhalbright added 2 commits September 9, 2020 21:51

Merge branch 'master' of https://github.com/go-gitea/gitea into prune…

157fd96

…-hook-task # Conflicts: # models/migrations/migrations.go # models/migrations/v148.go

changed a couple log statmentes to trace, should not have been error

dacb03b

6543 reviewed Sep 12, 2020

View reviewed changes

web_src/js/index.js Show resolved Hide resolved

bhalbright added 2 commits September 12, 2020 13:59

Merge branch 'master' of https://github.com/go-gitea/gitea into prune…

d1642ac

…-hook-task # Conflicts: # models/migrations/migrations.go # models/migrations/v150.go

created unit test to ensure that deleting delivered hook_task records…

63551dd

… will leave the most recent based if 1 delivery is to be kept

Merge branch 'master' of https://github.com/go-gitea/gitea into prune…

0341fc6

…-hook-task # Conflicts: # models/migrations/migrations.go # models/migrations/v151.go

Merge branch 'master' of https://github.com/go-gitea/gitea into prune…

cffae2e

…-hook-task # Conflicts: # models/migrations/migrations.go # models/migrations/v152.go # modules/repository/generate.go

bhalbright closed this Sep 24, 2020

6543 removed this from the 1.13.0 milestone Sep 24, 2020

bhalbright mentioned this pull request Oct 9, 2020

Cron job to cleanup hook_task table #13080

Merged

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prune hook_task table #11416

Prune hook_task table #11416

bhalbright commented May 15, 2020

bhalbright commented May 17, 2020

lunny commented Sep 2, 2020

lunny commented Sep 2, 2020

zeripath commented Sep 2, 2020

bhalbright commented Sep 2, 2020

codecov-commenter commented Sep 2, 2020 •

edited

Loading

zeripath commented Sep 2, 2020

lunny Sep 9, 2020

bhalbright Sep 10, 2020

lunny Sep 16, 2020

6543 Sep 12, 2020

bhalbright Sep 12, 2020

6543 Sep 12, 2020

bhalbright Sep 12, 2020

6543 Sep 12, 2020

6543 commented Sep 15, 2020

bhalbright commented Sep 16, 2020

6543 commented Sep 16, 2020 •

edited

Loading

6543 commented Sep 23, 2020

bhalbright commented Sep 24, 2020

Prune hook_task table #11416

Prune hook_task table #11416

Conversation

bhalbright commented May 15, 2020

bhalbright commented May 17, 2020

lunny commented Sep 2, 2020

lunny commented Sep 2, 2020

zeripath commented Sep 2, 2020

bhalbright commented Sep 2, 2020

codecov-commenter commented Sep 2, 2020 • edited Loading

Codecov Report

zeripath commented Sep 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

6543 commented Sep 15, 2020

bhalbright commented Sep 16, 2020

6543 commented Sep 16, 2020 • edited Loading

6543 commented Sep 23, 2020

bhalbright commented Sep 24, 2020

codecov-commenter commented Sep 2, 2020 •

edited

Loading

6543 commented Sep 16, 2020 •

edited

Loading