From 42073c2b6cf8f304e41201ec01c07335bf7166da Mon Sep 17 00:00:00 2001 From: Brad Albright Date: Sat, 26 Sep 2020 15:36:17 -0500 Subject: [PATCH 01/18] sketching out hook_task cleanup code --- custom/conf/app.example.ini | 15 +++++ .../doc/advanced/config-cheat-sheet.en-us.md | 9 +++ models/webhook.go | 62 +++++++++++++++++++ models/webhook_test.go | 58 +++++++++++++++++ modules/cron/setting.go | 8 +++ modules/cron/tasks_basic.go | 17 +++++ 6 files changed, 169 insertions(+) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index ffc4e406780e7..615c72bfb7da9 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -928,6 +928,21 @@ UPDATE_EXISTING = true ; Interval as a duration between each synchronization. (default every 24h) SCHEDULE = @every 24h +; Cleanup hook_task table +[cron.cleanup_hook_task_table] +; Whether to enable the job +ENABLED = true +; Whether to always run at start up time (if ENABLED) +RUN_AT_START = false +; Time interval for job to run +SCHEDULE = @every 24h +; AGE or NUMBER_TO_KEEP. How the records are removed, either by age (i.e. how old hook_task record is) or by the number to keep per webhook (i.e. keep most recent x deliveries per webhook). +CLEANUP_TYPE = AGE +; If CLEANUP_TYPE is set to AGE, then any delivered hook_task records older than this number of days will be deleted. +AGE_DAYS = 30 +; If CLEANUP_TYPE is set to NUMBER_TO_KEEP, this is number of hook_task records to keep for a webhook (i.e. keep the most recent x deliveries). +NUMBER_TO_KEEP = 10 + [git] ; The path of git executable. If empty, Gitea searches through the PATH environment. PATH = diff --git a/docs/content/doc/advanced/config-cheat-sheet.en-us.md b/docs/content/doc/advanced/config-cheat-sheet.en-us.md index 1e48ee2597f76..0db447c85e1dd 100644 --- a/docs/content/doc/advanced/config-cheat-sheet.en-us.md +++ b/docs/content/doc/advanced/config-cheat-sheet.en-us.md @@ -594,6 +594,15 @@ NB: You must `REDIRECT_MACARON_LOG` and have `DISABLE_ROUTER_LOG` set to `false` - `RUN_AT_START`: **true**: Run repository statistics check at start time. - `SCHEDULE`: **@every 24h**: Cron syntax for scheduling repository statistics check. +### Cron - Cleanup hook_task Table (`cron.cleanup_hook_task_table`) + +- `ENABLED`: **true**: Enable cleanup hook_task job. +- `RUN_AT_START`: **false**: Run cleanup hook_task at start time (if ENABLED). +- `SCHEDULE`: **@every 24h**: Cron syntax for cleaning hook_task table. +- `CLEANUP_TYPE` **AGE** AGE or NUMBER_TO_KEEP Method to cleanup hook_task, either by age (i.e. how old hook_task record is) or by the number to keep per webhook (i.e. keep most recent x deliveries per webhook). +- `AGE_DAYS`: **30**: If CLEANUP_TYPE is set to AGE, then any delivered hook_task records older than this number of days will be deleted. +- `NUMBER_TO_KEEP`: **10**: If CLEANUP_TYPE is set to NUMBER_TO_KEEP, this is number of hook_task records to keep for a webhook (i.e. keep the most recent x deliveries). + ### Cron - Update Migration Poster ID (`cron.update_migration_poster_id`) - `SCHEDULE`: **@every 24h** : Interval as a duration between each synchronization, it will always attempt synchronization when the instance starts. diff --git a/models/webhook.go b/models/webhook.go index 54cd9b6565841..14c6163ccc85e 100644 --- a/models/webhook.go +++ b/models/webhook.go @@ -6,10 +6,13 @@ package models import ( + "context" "encoding/json" "fmt" "time" + //"xorm.io/builder" + //"code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" @@ -798,3 +801,62 @@ func FindRepoUndeliveredHookTasks(repoID int64) ([]*HookTask, error) { } return tasks, nil } + + +// CleanupHookTaskTable deletes rows from hook_task as needed. +func CleanupHookTaskTable(ctx context.Context, cleanupType string, ageDays int, numberToKeep int) error { + log.Trace("Doing: CleanupHookTaskTable") + + if cleanupType == "AGE" && ageDays > 0 { + deleteOlderThan := time.Now().AddDate(0, 0, ageDays).UnixNano(); + deletes, err := x. + Where("is_delivered = ? and delivered < ?", true, deleteOlderThan). + Delete(new(HookTask)) + if err != nil { + return err + } + log.Trace("Deleted %d rows from hook_task older than %d days", deletes, ageDays) + + } else if cleanupType == "NUMBER_TO_KEEP" && numberToKeep > 0 { + hookIDs := make([]int64, 0, 10) + err := x.Table("webhook"). + Where("id > 0"). + Cols("id"). + Find(&hookIDs) + if err != nil { + return err + } + for _, hookID := range hookIDs { + deleteDeliveredHookTasksByWebhook(hookID, numberToKeep) + } + } + + log.Trace("Finished: CleanupHookTaskTable") + return nil +} + +func deleteDeliveredHookTasksByWebhook(hookID int64, numberDeliveriesToKeep int) error { + var deliveryDates = make([]int64, 0, 10) + err := x.Table("hook_task"). + Where("hook_task.hook_id = ? AND hook_task.is_delivered = ?", hookID, true). + Cols("hook_task.delivered"). + Join("INNER", "webhook", "hook_task.hook_id = webhook.id"). + OrderBy("hook_task.delivered desc"). + Limit(1, int(numberDeliveriesToKeep)). + Find(&deliveryDates) + if err != nil { + return err + } + + if len(deliveryDates) > 0 { + deletes, err := x. + Where("hook_id = ? and is_delivered = ? and delivered <= ?", hookID, true, deliveryDates[0]). + Delete(new(HookTask)) + if err != nil { + return err + } + log.Trace("Deleted %d hook_task rows for webhook %d", deletes, hookID) + } + + return nil +} diff --git a/models/webhook_test.go b/models/webhook_test.go index 5ee7f9159bd50..fb2c87ffd4d07 100644 --- a/models/webhook_test.go +++ b/models/webhook_test.go @@ -7,6 +7,7 @@ package models import ( "encoding/json" "testing" + "time" api "code.gitea.io/gitea/modules/structs" @@ -245,3 +246,60 @@ func TestUpdateHookTask(t *testing.T) { assert.NoError(t, UpdateHookTask(hook)) AssertExistsAndLoadBean(t, hook) } + +func TestDeleteDeliveredHookTasks_DeletesDelivered(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + hookTask := &HookTask{ + RepoID: 3, + HookID: 3, + Type: GITEA, + URL: "http://www.example.com/unit_test", + Payloader: &api.PushPayload{}, + IsDelivered: true, + } + AssertNotExistsBean(t, hookTask) + assert.NoError(t, CreateHookTask(hookTask)) + AssertExistsAndLoadBean(t, hookTask) + + assert.NoError(t, DeleteDeliveredHookTasks(3, 0)) + AssertNotExistsBean(t, hookTask) +} + +func TestDeleteDeliveredHookTasks_LeavesUndelivered(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + hookTask := &HookTask{ + RepoID: 2, + HookID: 4, + Type: GITEA, + URL: "http://www.example.com/unit_test", + Payloader: &api.PushPayload{}, + IsDelivered: false, + } + AssertNotExistsBean(t, hookTask) + assert.NoError(t, CreateHookTask(hookTask)) + AssertExistsAndLoadBean(t, hookTask) + + assert.NoError(t, DeleteDeliveredHookTasks(3, 0)) + AssertExistsAndLoadBean(t, hookTask) +} + +func TestDeleteDeliveredHookTasks_LeavesMostRecentTask(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + hookTask := &HookTask{ + RepoID: 2, + HookID: 4, + Type: GITEA, + URL: "http://www.example.com/unit_test", + Payloader: &api.PushPayload{}, + IsDelivered: true, + Delivered: time.Now().UnixNano(), + } + AssertNotExistsBean(t, hookTask) + assert.NoError(t, CreateHookTask(hookTask)) + AssertExistsAndLoadBean(t, hookTask) + + assert.NoError(t, DeleteDeliveredHookTasks(3, 1)) + AssertExistsAndLoadBean(t, hookTask) +} + +//TODO need unit tests for AGE type of cleanup... diff --git a/modules/cron/setting.go b/modules/cron/setting.go index 5fe9ca6e14d82..c0baf3b0d7989 100644 --- a/modules/cron/setting.go +++ b/modules/cron/setting.go @@ -40,6 +40,14 @@ type UpdateExistingConfig struct { UpdateExisting bool } +// CleanupHookTaskConfig represents a cron task with settings to cleanup hook_task +type CleanupHookTaskConfig struct { + BaseConfig + CleanupType string + AgeDays int + NumberToKeep int +} + // GetSchedule returns the schedule for the base config func (b *BaseConfig) GetSchedule() string { return b.Schedule diff --git a/modules/cron/tasks_basic.go b/modules/cron/tasks_basic.go index 4da21fc7d9ddd..aea218f96e6ce 100644 --- a/modules/cron/tasks_basic.go +++ b/modules/cron/tasks_basic.go @@ -108,6 +108,22 @@ func registerUpdateMigrationPosterID() { }) } +func registerCleanupHookTaskTable() { + RegisterTaskFatal("cleanup_hook_task_table", &CleanupHookTaskConfig{ + BaseConfig: BaseConfig{ + Enabled: true, + RunAtStart: false, + Schedule: "@every 24h", + }, + CleanupType: "AGE", + AgeDays: 30, + NumberToKeep: 10, + }, func(ctx context.Context, _ *models.User, config Config) error { + realConfig := config.(*CleanupHookTaskConfig) + return models.CleanupHookTaskTable(ctx, realConfig.CleanupType, realConfig.AgeDays, realConfig.NumberToKeep) + }) +} + func initBasicTasks() { registerUpdateMirrorTask() registerRepoHealthCheck() @@ -116,4 +132,5 @@ func initBasicTasks() { registerSyncExternalUsers() registerDeletedBranchesCleanup() registerUpdateMigrationPosterID() + registerCleanupHookTaskTable() } From e505ceda09950d9fccc69e482d48170b69d9baa8 Mon Sep 17 00:00:00 2001 From: Brad Albright Date: Mon, 28 Sep 2020 22:05:30 -0500 Subject: [PATCH 02/18] continued work on cleanup hook_task --- custom/conf/app.example.ini | 8 ++--- .../doc/advanced/config-cheat-sheet.en-us.md | 6 ++-- models/webhook.go | 30 ++++++++++++++----- models/webhook_test.go | 12 ++++---- modules/cron/tasks_basic.go | 2 +- 5 files changed, 37 insertions(+), 21 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 615c72bfb7da9..b336749ea6b10 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -936,11 +936,11 @@ ENABLED = true RUN_AT_START = false ; Time interval for job to run SCHEDULE = @every 24h -; AGE or NUMBER_TO_KEEP. How the records are removed, either by age (i.e. how old hook_task record is) or by the number to keep per webhook (i.e. keep most recent x deliveries per webhook). -CLEANUP_TYPE = AGE -; If CLEANUP_TYPE is set to AGE, then any delivered hook_task records older than this number of days will be deleted. +; Age or PerWebhook. How the records are removed, either by age (i.e. how old hook_task record is) or by the number to keep per webhook (i.e. keep most recent x deliveries per webhook). +CLEANUP_TYPE = Age +; If CLEANUP_TYPE is set to Age, then any delivered hook_task records older than this number of days will be deleted. AGE_DAYS = 30 -; If CLEANUP_TYPE is set to NUMBER_TO_KEEP, this is number of hook_task records to keep for a webhook (i.e. keep the most recent x deliveries). +; If CLEANUP_TYPE is set to PerWebhook, this is number of hook_task records to keep for a webhook (i.e. keep the most recent x deliveries). NUMBER_TO_KEEP = 10 [git] diff --git a/docs/content/doc/advanced/config-cheat-sheet.en-us.md b/docs/content/doc/advanced/config-cheat-sheet.en-us.md index 0db447c85e1dd..cf52d5969750e 100644 --- a/docs/content/doc/advanced/config-cheat-sheet.en-us.md +++ b/docs/content/doc/advanced/config-cheat-sheet.en-us.md @@ -599,9 +599,9 @@ NB: You must `REDIRECT_MACARON_LOG` and have `DISABLE_ROUTER_LOG` set to `false` - `ENABLED`: **true**: Enable cleanup hook_task job. - `RUN_AT_START`: **false**: Run cleanup hook_task at start time (if ENABLED). - `SCHEDULE`: **@every 24h**: Cron syntax for cleaning hook_task table. -- `CLEANUP_TYPE` **AGE** AGE or NUMBER_TO_KEEP Method to cleanup hook_task, either by age (i.e. how old hook_task record is) or by the number to keep per webhook (i.e. keep most recent x deliveries per webhook). -- `AGE_DAYS`: **30**: If CLEANUP_TYPE is set to AGE, then any delivered hook_task records older than this number of days will be deleted. -- `NUMBER_TO_KEEP`: **10**: If CLEANUP_TYPE is set to NUMBER_TO_KEEP, this is number of hook_task records to keep for a webhook (i.e. keep the most recent x deliveries). +- `CLEANUP_TYPE` **Age** Age or PerWebhook Method to cleanup hook_task, either by age (i.e. how old hook_task record is) or by the number to keep per webhook (i.e. keep most recent x deliveries per webhook). +- `AGE_DAYS`: **30**: If CLEANUP_TYPE is set to Age, then any delivered hook_task records older than this number of days will be deleted. +- `NUMBER_TO_KEEP`: **10**: If CLEANUP_TYPE is set to PerWebhook, this is number of hook_task records to keep for a webhook (i.e. keep the most recent x deliveries). ### Cron - Update Migration Poster ID (`cron.update_migration_poster_id`) diff --git a/models/webhook.go b/models/webhook.go index 14c6163ccc85e..305a6b701cc6a 100644 --- a/models/webhook.go +++ b/models/webhook.go @@ -6,13 +6,10 @@ package models import ( - "context" "encoding/json" "fmt" "time" - //"xorm.io/builder" - //"code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" @@ -41,6 +38,26 @@ func ToHookContentType(name string) HookContentType { return hookContentTypes[name] } +// HookTaskCleanupType is the type of cleanup to perform on hook_task +type HookTaskCleanupType int + +const ( + // Age hook_task rows will be cleaned up by the age of the row + Age HookTaskCleanupType = iota + // PerWebhook hook_task rows will be cleaned up by leaving the most recent deliveries for each webhook + PerWebhook +) + +var hookTaskCleanupTypes = map[string]HookTaskCleanupType{ + "Age": Age, + "PerWebhook": PerWebhook, +} + +// ToHookTaskCleanupType returns HookTaskCleanupType by given name. +func ToHookTaskCleanupType(name string) HookTaskCleanupType { + return hookTaskCleanupTypes[name] +} + // Name returns the name of a given web hook's content type func (t HookContentType) Name() string { switch t { @@ -802,12 +819,11 @@ func FindRepoUndeliveredHookTasks(repoID int64) ([]*HookTask, error) { return tasks, nil } - // CleanupHookTaskTable deletes rows from hook_task as needed. -func CleanupHookTaskTable(ctx context.Context, cleanupType string, ageDays int, numberToKeep int) error { +func CleanupHookTaskTable(cleanupType HookTaskCleanupType, ageDays int, numberToKeep int) error { log.Trace("Doing: CleanupHookTaskTable") - if cleanupType == "AGE" && ageDays > 0 { + if cleanupType == Age && ageDays > 0 { deleteOlderThan := time.Now().AddDate(0, 0, ageDays).UnixNano(); deletes, err := x. Where("is_delivered = ? and delivered < ?", true, deleteOlderThan). @@ -817,7 +833,7 @@ func CleanupHookTaskTable(ctx context.Context, cleanupType string, ageDays int, } log.Trace("Deleted %d rows from hook_task older than %d days", deletes, ageDays) - } else if cleanupType == "NUMBER_TO_KEEP" && numberToKeep > 0 { + } else if cleanupType == PerWebhook { hookIDs := make([]int64, 0, 10) err := x.Table("webhook"). Where("id > 0"). diff --git a/models/webhook_test.go b/models/webhook_test.go index fb2c87ffd4d07..1363a0df0dbfa 100644 --- a/models/webhook_test.go +++ b/models/webhook_test.go @@ -247,7 +247,7 @@ func TestUpdateHookTask(t *testing.T) { AssertExistsAndLoadBean(t, hook) } -func TestDeleteDeliveredHookTasks_DeletesDelivered(t *testing.T) { +func TestCleanupHookTaskTable_NumberToKeep_DeletesDelivered(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) hookTask := &HookTask{ RepoID: 3, @@ -261,11 +261,11 @@ func TestDeleteDeliveredHookTasks_DeletesDelivered(t *testing.T) { assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, DeleteDeliveredHookTasks(3, 0)) + assert.NoError(t, CleanupHookTaskTable(PerWebhook, 0, 0)) AssertNotExistsBean(t, hookTask) } -func TestDeleteDeliveredHookTasks_LeavesUndelivered(t *testing.T) { +func TestCleanupHookTaskTable_NumberToKeep_LeavesUndelivered(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) hookTask := &HookTask{ RepoID: 2, @@ -279,11 +279,11 @@ func TestDeleteDeliveredHookTasks_LeavesUndelivered(t *testing.T) { assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, DeleteDeliveredHookTasks(3, 0)) + assert.NoError(t, CleanupHookTaskTable(PerWebhook, 0, 0)) AssertExistsAndLoadBean(t, hookTask) } -func TestDeleteDeliveredHookTasks_LeavesMostRecentTask(t *testing.T) { +func TestCleanupHookTaskTable_NumberToKeep_LeavesMostRecentTask(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) hookTask := &HookTask{ RepoID: 2, @@ -298,7 +298,7 @@ func TestDeleteDeliveredHookTasks_LeavesMostRecentTask(t *testing.T) { assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, DeleteDeliveredHookTasks(3, 1)) + assert.NoError(t, CleanupHookTaskTable(PerWebhook, 0, 1)) AssertExistsAndLoadBean(t, hookTask) } diff --git a/modules/cron/tasks_basic.go b/modules/cron/tasks_basic.go index aea218f96e6ce..65e132530a97c 100644 --- a/modules/cron/tasks_basic.go +++ b/modules/cron/tasks_basic.go @@ -120,7 +120,7 @@ func registerCleanupHookTaskTable() { NumberToKeep: 10, }, func(ctx context.Context, _ *models.User, config Config) error { realConfig := config.(*CleanupHookTaskConfig) - return models.CleanupHookTaskTable(ctx, realConfig.CleanupType, realConfig.AgeDays, realConfig.NumberToKeep) + return models.CleanupHookTaskTable(models.ToHookTaskCleanupType(realConfig.CleanupType), realConfig.AgeDays, realConfig.NumberToKeep) }) } From 6278f52f6a00c99008975ed3fe521e4265c3ffec Mon Sep 17 00:00:00 2001 From: Brad Albright Date: Mon, 28 Sep 2020 22:13:04 -0500 Subject: [PATCH 03/18] added tests for age based hook_task cleanup --- models/webhook_test.go | 63 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 59 insertions(+), 4 deletions(-) diff --git a/models/webhook_test.go b/models/webhook_test.go index 1363a0df0dbfa..dab7a6c0f7c3c 100644 --- a/models/webhook_test.go +++ b/models/webhook_test.go @@ -247,7 +247,7 @@ func TestUpdateHookTask(t *testing.T) { AssertExistsAndLoadBean(t, hook) } -func TestCleanupHookTaskTable_NumberToKeep_DeletesDelivered(t *testing.T) { +func TestCleanupHookTaskTable_PerWebhook_DeletesDelivered(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) hookTask := &HookTask{ RepoID: 3, @@ -265,7 +265,7 @@ func TestCleanupHookTaskTable_NumberToKeep_DeletesDelivered(t *testing.T) { AssertNotExistsBean(t, hookTask) } -func TestCleanupHookTaskTable_NumberToKeep_LeavesUndelivered(t *testing.T) { +func TestCleanupHookTaskTable_PerWebhook_LeavesUndelivered(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) hookTask := &HookTask{ RepoID: 2, @@ -283,7 +283,7 @@ func TestCleanupHookTaskTable_NumberToKeep_LeavesUndelivered(t *testing.T) { AssertExistsAndLoadBean(t, hookTask) } -func TestCleanupHookTaskTable_NumberToKeep_LeavesMostRecentTask(t *testing.T) { +func TestCleanupHookTaskTable_PerWebhook_LeavesMostRecentTask(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) hookTask := &HookTask{ RepoID: 2, @@ -302,4 +302,59 @@ func TestCleanupHookTaskTable_NumberToKeep_LeavesMostRecentTask(t *testing.T) { AssertExistsAndLoadBean(t, hookTask) } -//TODO need unit tests for AGE type of cleanup... +func TestCleanupHookTaskTable_Age_DeletesDelivered(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + hookTask := &HookTask{ + RepoID: 3, + HookID: 3, + Type: GITEA, + URL: "http://www.example.com/unit_test", + Payloader: &api.PushPayload{}, + IsDelivered: true, + Delivered: time.Now().AddDate(0, 0, 31).UnixNano(), + } + AssertNotExistsBean(t, hookTask) + assert.NoError(t, CreateHookTask(hookTask)) + AssertExistsAndLoadBean(t, hookTask) + + assert.NoError(t, CleanupHookTaskTable(Age, 30, 0)) + AssertNotExistsBean(t, hookTask) +} + +func TestCleanupHookTaskTable_Age_LeavesUndelivered(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + hookTask := &HookTask{ + RepoID: 2, + HookID: 4, + Type: GITEA, + URL: "http://www.example.com/unit_test", + Payloader: &api.PushPayload{}, + IsDelivered: false, + Delivered: time.Now().AddDate(0, 0, 31).UnixNano(), + } + AssertNotExistsBean(t, hookTask) + assert.NoError(t, CreateHookTask(hookTask)) + AssertExistsAndLoadBean(t, hookTask) + + assert.NoError(t, CleanupHookTaskTable(Age, 30, 0)) + AssertExistsAndLoadBean(t, hookTask) +} + +func TestCleanupHookTaskTable_Age_LeavesTaskEarlierThanAgeToDelete(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + hookTask := &HookTask{ + RepoID: 2, + HookID: 4, + Type: GITEA, + URL: "http://www.example.com/unit_test", + Payloader: &api.PushPayload{}, + IsDelivered: true, + Delivered: time.Now().AddDate(0, 0, 29).UnixNano(), + } + AssertNotExistsBean(t, hookTask) + assert.NoError(t, CreateHookTask(hookTask)) + AssertExistsAndLoadBean(t, hookTask) + + assert.NoError(t, CleanupHookTaskTable(Age, 30, 0)) + AssertExistsAndLoadBean(t, hookTask) +} From 65eea98f19ff037532a2df851a4e8d3e30be2bcf Mon Sep 17 00:00:00 2001 From: Brad Albright Date: Wed, 7 Oct 2020 09:56:30 -0500 Subject: [PATCH 04/18] mostly local debug --- models/webhook.go | 24 ++++++++++++++++-------- modules/cron/tasks_basic.go | 15 ++++++++++++--- options/locale/locale_en-US.ini | 3 ++- 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/models/webhook.go b/models/webhook.go index 305a6b701cc6a..e53803a08bd3a 100644 --- a/models/webhook.go +++ b/models/webhook.go @@ -6,6 +6,7 @@ package models import ( + "context" "encoding/json" "fmt" "time" @@ -49,7 +50,7 @@ const ( ) var hookTaskCleanupTypes = map[string]HookTaskCleanupType{ - "Age": Age, + "Age": Age, "PerWebhook": PerWebhook, } @@ -820,18 +821,18 @@ func FindRepoUndeliveredHookTasks(repoID int64) ([]*HookTask, error) { } // CleanupHookTaskTable deletes rows from hook_task as needed. -func CleanupHookTaskTable(cleanupType HookTaskCleanupType, ageDays int, numberToKeep int) error { - log.Trace("Doing: CleanupHookTaskTable") +func CleanupHookTaskTable(ctx context.Context, cleanupType HookTaskCleanupType, ageDays int, numberToKeep int) error { + log.Error("Doing: CleanupHookTaskTable") if cleanupType == Age && ageDays > 0 { - deleteOlderThan := time.Now().AddDate(0, 0, ageDays).UnixNano(); + deleteOlderThan := time.Now().AddDate(0, 0, -ageDays).UnixNano() deletes, err := x. Where("is_delivered = ? and delivered < ?", true, deleteOlderThan). Delete(new(HookTask)) if err != nil { return err } - log.Trace("Deleted %d rows from hook_task older than %d days", deletes, ageDays) + log.Error("Deleted %d rows from hook_task older than %d days", deletes, ageDays) } else if cleanupType == PerWebhook { hookIDs := make([]int64, 0, 10) @@ -843,11 +844,18 @@ func CleanupHookTaskTable(cleanupType HookTaskCleanupType, ageDays int, numberTo return err } for _, hookID := range hookIDs { - deleteDeliveredHookTasksByWebhook(hookID, numberToKeep) + select { + case <-ctx.Done(): + return ErrCancelledf("Before deleting hook_task records for hook id %d", hookID) + default: + } + if err = deleteDeliveredHookTasksByWebhook(hookID, numberToKeep); err != nil { + return err + } } } - log.Trace("Finished: CleanupHookTaskTable") + log.Error("Finished: CleanupHookTaskTable") return nil } @@ -871,7 +879,7 @@ func deleteDeliveredHookTasksByWebhook(hookID int64, numberDeliveriesToKeep int) if err != nil { return err } - log.Trace("Deleted %d hook_task rows for webhook %d", deletes, hookID) + log.Error("Deleted %d hook_task rows for webhook %d", deletes, hookID) } return nil diff --git a/modules/cron/tasks_basic.go b/modules/cron/tasks_basic.go index 65e132530a97c..de45da635302b 100644 --- a/modules/cron/tasks_basic.go +++ b/modules/cron/tasks_basic.go @@ -8,6 +8,7 @@ import ( "context" "time" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/migrations" repository_service "code.gitea.io/gitea/modules/repository" @@ -112,18 +113,26 @@ func registerCleanupHookTaskTable() { RegisterTaskFatal("cleanup_hook_task_table", &CleanupHookTaskConfig{ BaseConfig: BaseConfig{ Enabled: true, - RunAtStart: false, + RunAtStart: true, Schedule: "@every 24h", }, - CleanupType: "AGE", + CleanupType: "Age", AgeDays: 30, NumberToKeep: 10, }, func(ctx context.Context, _ *models.User, config Config) error { realConfig := config.(*CleanupHookTaskConfig) - return models.CleanupHookTaskTable(models.ToHookTaskCleanupType(realConfig.CleanupType), realConfig.AgeDays, realConfig.NumberToKeep) + //TODO log what is in here! + log.Error("num %d", realConfig.NumberToKeep) + log.Error("age %d", realConfig.AgeDays) + log.Error("type %s", realConfig.CleanupType) + log.Error("enabled %s", realConfig.Enabled) + log.Error("run!!! %s", realConfig.RunAtStart) + log.Error("sched %s", realConfig.Schedule) + return models.CleanupHookTaskTable(ctx, models.ToHookTaskCleanupType(realConfig.CleanupType), realConfig.AgeDays, realConfig.NumberToKeep) }) } + func initBasicTasks() { registerUpdateMirrorTask() registerRepoHealthCheck() diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index ea76d4cb6a7c5..6ac7202a5ff29 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -941,7 +941,7 @@ issues.new.no_reviewers = No reviewers issues.new.add_reviewer_title = Request review issues.choose.get_started = Get Started issues.choose.blank = Default -issues.choose.blank_about = Create an issue from default template. +issues.choose.blank_about = Create an issue from default template. issues.no_ref = No Branch/Tag Specified issues.create = Create Issue issues.new_label = New Label @@ -1982,6 +1982,7 @@ dashboard.resync_all_sshkeys.desc = (Not needed for the built-in SSH server.) dashboard.resync_all_hooks = Resynchronize pre-receive, update and post-receive hooks of all repositories. dashboard.reinit_missing_repos = Reinitialize all missing Git repositories for which records exist dashboard.sync_external_users = Synchronize external user data +dashboard.cleanup_hook_task_table = Cleanup hook_task table dashboard.server_uptime = Server Uptime dashboard.current_goroutine = Current Goroutines dashboard.current_memory_usage = Current Memory Usage From f541590975fc8a970c58b8b843466c1617b81d4d Mon Sep 17 00:00:00 2001 From: Brad Albright Date: Wed, 7 Oct 2020 12:36:12 -0500 Subject: [PATCH 05/18] changed to use a time duration for the default cleanup --- custom/conf/app.example.ini | 8 ++++---- .../doc/advanced/config-cheat-sheet.en-us.md | 4 ++-- models/webhook.go | 17 ++++++++++------- modules/cron/setting.go | 2 +- modules/cron/tasks_basic.go | 17 ++++------------- 5 files changed, 21 insertions(+), 27 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 2034d7ef2dab4..e77e3dfa37b61 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -990,10 +990,10 @@ ENABLED = true RUN_AT_START = false ; Time interval for job to run SCHEDULE = @every 24h -; Age or PerWebhook. How the records are removed, either by age (i.e. how old hook_task record is) or by the number to keep per webhook (i.e. keep most recent x deliveries per webhook). -CLEANUP_TYPE = Age -; If CLEANUP_TYPE is set to Age, then any delivered hook_task records older than this number of days will be deleted. -AGE_DAYS = 30 +; OlderThan or PerWebhook. How the records are removed, either by age (i.e. how long ago hook_task record was delivered) or by the number to keep per webhook (i.e. keep most recent x deliveries per webhook). +CLEANUP_TYPE = OlderThan +; If CLEANUP_TYPE is set to OlderThan, then any delivered hook_task records older than this expression will be deleted. +OLDER_THAN = 30d ; If CLEANUP_TYPE is set to PerWebhook, this is number of hook_task records to keep for a webhook (i.e. keep the most recent x deliveries). NUMBER_TO_KEEP = 10 diff --git a/docs/content/doc/advanced/config-cheat-sheet.en-us.md b/docs/content/doc/advanced/config-cheat-sheet.en-us.md index 448436b688229..ba4a7980b9135 100644 --- a/docs/content/doc/advanced/config-cheat-sheet.en-us.md +++ b/docs/content/doc/advanced/config-cheat-sheet.en-us.md @@ -690,8 +690,8 @@ NB: You must `REDIRECT_MACARON_LOG` and have `DISABLE_ROUTER_LOG` set to `false` - `ENABLED`: **true**: Enable cleanup hook_task job. - `RUN_AT_START`: **false**: Run cleanup hook_task at start time (if ENABLED). - `SCHEDULE`: **@every 24h**: Cron syntax for cleaning hook_task table. -- `CLEANUP_TYPE` **Age** Age or PerWebhook Method to cleanup hook_task, either by age (i.e. how old hook_task record is) or by the number to keep per webhook (i.e. keep most recent x deliveries per webhook). -- `AGE_DAYS`: **30**: If CLEANUP_TYPE is set to Age, then any delivered hook_task records older than this number of days will be deleted. +- `CLEANUP_TYPE` **OlderThan** OlderThan or PerWebhook Method to cleanup hook_task, either by age (i.e. how long ago hook_task record was delivered) or by the number to keep per webhook (i.e. keep most recent x deliveries per webhook). +- `OLDER_THAN`: **30d**: If CLEANUP_TYPE is set to OlderThan, then any delivered hook_task records older than this expression will be deleted. - `NUMBER_TO_KEEP`: **10**: If CLEANUP_TYPE is set to PerWebhook, this is number of hook_task records to keep for a webhook (i.e. keep the most recent x deliveries). ### Cron - Update Migration Poster ID (`cron.update_migration_poster_id`) diff --git a/models/webhook.go b/models/webhook.go index e53803a08bd3a..dce04c3b4c935 100644 --- a/models/webhook.go +++ b/models/webhook.go @@ -43,14 +43,14 @@ func ToHookContentType(name string) HookContentType { type HookTaskCleanupType int const ( - // Age hook_task rows will be cleaned up by the age of the row - Age HookTaskCleanupType = iota + // OlderThan hook_task rows will be cleaned up by the age of the row + OlderThan HookTaskCleanupType = iota // PerWebhook hook_task rows will be cleaned up by leaving the most recent deliveries for each webhook PerWebhook ) var hookTaskCleanupTypes = map[string]HookTaskCleanupType{ - "Age": Age, + "OlderThan": OlderThan, "PerWebhook": PerWebhook, } @@ -821,18 +821,18 @@ func FindRepoUndeliveredHookTasks(repoID int64) ([]*HookTask, error) { } // CleanupHookTaskTable deletes rows from hook_task as needed. -func CleanupHookTaskTable(ctx context.Context, cleanupType HookTaskCleanupType, ageDays int, numberToKeep int) error { +func CleanupHookTaskTable(ctx context.Context, cleanupType HookTaskCleanupType, olderThan time.Duration, numberToKeep int) error { log.Error("Doing: CleanupHookTaskTable") - if cleanupType == Age && ageDays > 0 { - deleteOlderThan := time.Now().AddDate(0, 0, -ageDays).UnixNano() + if cleanupType == OlderThan { + deleteOlderThan := time.Now().Add(-olderThan).UnixNano() deletes, err := x. Where("is_delivered = ? and delivered < ?", true, deleteOlderThan). Delete(new(HookTask)) if err != nil { return err } - log.Error("Deleted %d rows from hook_task older than %d days", deletes, ageDays) + log.Error("Deleted %d rows from hook_task older than %d", deletes, olderThan) } else if cleanupType == PerWebhook { hookIDs := make([]int64, 0, 10) @@ -860,6 +860,7 @@ func CleanupHookTaskTable(ctx context.Context, cleanupType HookTaskCleanupType, } func deleteDeliveredHookTasksByWebhook(hookID int64, numberDeliveriesToKeep int) error { + log.Error("Deleting hook_task rows for webhook %d, keeping the most recent %d deliveries", hookID, numberDeliveriesToKeep) var deliveryDates = make([]int64, 0, 10) err := x.Table("hook_task"). Where("hook_task.hook_id = ? AND hook_task.is_delivered = ?", hookID, true). @@ -880,6 +881,8 @@ func deleteDeliveredHookTasksByWebhook(hookID int64, numberDeliveriesToKeep int) return err } log.Error("Deleted %d hook_task rows for webhook %d", deletes, hookID) + } else { + log.Error("No hook_task rows to delete for webhook %d", hookID) } return nil diff --git a/modules/cron/setting.go b/modules/cron/setting.go index c0baf3b0d7989..d55e5b60ad69e 100644 --- a/modules/cron/setting.go +++ b/modules/cron/setting.go @@ -44,7 +44,7 @@ type UpdateExistingConfig struct { type CleanupHookTaskConfig struct { BaseConfig CleanupType string - AgeDays int + OlderThan time.Duration NumberToKeep int } diff --git a/modules/cron/tasks_basic.go b/modules/cron/tasks_basic.go index de45da635302b..b38855078a157 100644 --- a/modules/cron/tasks_basic.go +++ b/modules/cron/tasks_basic.go @@ -8,7 +8,6 @@ import ( "context" "time" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/migrations" repository_service "code.gitea.io/gitea/modules/repository" @@ -113,26 +112,18 @@ func registerCleanupHookTaskTable() { RegisterTaskFatal("cleanup_hook_task_table", &CleanupHookTaskConfig{ BaseConfig: BaseConfig{ Enabled: true, - RunAtStart: true, + RunAtStart: false, Schedule: "@every 24h", }, - CleanupType: "Age", - AgeDays: 30, + CleanupType: "OlderThan", + OlderThan: 24 * time.Hour * 10, NumberToKeep: 10, }, func(ctx context.Context, _ *models.User, config Config) error { realConfig := config.(*CleanupHookTaskConfig) - //TODO log what is in here! - log.Error("num %d", realConfig.NumberToKeep) - log.Error("age %d", realConfig.AgeDays) - log.Error("type %s", realConfig.CleanupType) - log.Error("enabled %s", realConfig.Enabled) - log.Error("run!!! %s", realConfig.RunAtStart) - log.Error("sched %s", realConfig.Schedule) - return models.CleanupHookTaskTable(ctx, models.ToHookTaskCleanupType(realConfig.CleanupType), realConfig.AgeDays, realConfig.NumberToKeep) + return models.CleanupHookTaskTable(ctx, models.ToHookTaskCleanupType(realConfig.CleanupType), realConfig.OlderThan, realConfig.NumberToKeep) }) } - func initBasicTasks() { registerUpdateMirrorTask() registerRepoHealthCheck() From 225335437f50669a1706639792fa10df19125fe8 Mon Sep 17 00:00:00 2001 From: Brad Albright Date: Wed, 7 Oct 2020 12:47:27 -0500 Subject: [PATCH 06/18] fixed oldenthan default and unit tests --- models/webhook.go | 2 -- models/webhook_test.go | 13 +++++++------ modules/cron/tasks_basic.go | 2 +- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/models/webhook.go b/models/webhook.go index dce04c3b4c935..5a9571c7260bb 100644 --- a/models/webhook.go +++ b/models/webhook.go @@ -833,7 +833,6 @@ func CleanupHookTaskTable(ctx context.Context, cleanupType HookTaskCleanupType, return err } log.Error("Deleted %d rows from hook_task older than %d", deletes, olderThan) - } else if cleanupType == PerWebhook { hookIDs := make([]int64, 0, 10) err := x.Table("webhook"). @@ -854,7 +853,6 @@ func CleanupHookTaskTable(ctx context.Context, cleanupType HookTaskCleanupType, } } } - log.Error("Finished: CleanupHookTaskTable") return nil } diff --git a/models/webhook_test.go b/models/webhook_test.go index dab7a6c0f7c3c..224d06c98659d 100644 --- a/models/webhook_test.go +++ b/models/webhook_test.go @@ -5,6 +5,7 @@ package models import ( + "context" "encoding/json" "testing" "time" @@ -261,7 +262,7 @@ func TestCleanupHookTaskTable_PerWebhook_DeletesDelivered(t *testing.T) { assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, CleanupHookTaskTable(PerWebhook, 0, 0)) + assert.NoError(t, CleanupHookTaskTable(context.Background(), PerWebhook, 0, 0)) AssertNotExistsBean(t, hookTask) } @@ -279,7 +280,7 @@ func TestCleanupHookTaskTable_PerWebhook_LeavesUndelivered(t *testing.T) { assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, CleanupHookTaskTable(PerWebhook, 0, 0)) + assert.NoError(t, CleanupHookTaskTable(context.Background(), PerWebhook, 0, 0)) AssertExistsAndLoadBean(t, hookTask) } @@ -298,7 +299,7 @@ func TestCleanupHookTaskTable_PerWebhook_LeavesMostRecentTask(t *testing.T) { assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, CleanupHookTaskTable(PerWebhook, 0, 1)) + assert.NoError(t, CleanupHookTaskTable(context.Background(), PerWebhook, 0, 1)) AssertExistsAndLoadBean(t, hookTask) } @@ -317,7 +318,7 @@ func TestCleanupHookTaskTable_Age_DeletesDelivered(t *testing.T) { assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, CleanupHookTaskTable(Age, 30, 0)) + assert.NoError(t, CleanupHookTaskTable(context.Background(), OlderThan, 24 * time.Hour * 30, 0)) AssertNotExistsBean(t, hookTask) } @@ -336,7 +337,7 @@ func TestCleanupHookTaskTable_Age_LeavesUndelivered(t *testing.T) { assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, CleanupHookTaskTable(Age, 30, 0)) + assert.NoError(t, CleanupHookTaskTable(context.Background(), OlderThan, 24 * time.Hour * 30, 0)) AssertExistsAndLoadBean(t, hookTask) } @@ -355,6 +356,6 @@ func TestCleanupHookTaskTable_Age_LeavesTaskEarlierThanAgeToDelete(t *testing.T) assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, CleanupHookTaskTable(Age, 30, 0)) + assert.NoError(t, CleanupHookTaskTable(context.Background(), OlderThan, 24 * time.Hour * 30, 0)) AssertExistsAndLoadBean(t, hookTask) } diff --git a/modules/cron/tasks_basic.go b/modules/cron/tasks_basic.go index b38855078a157..ffccbb44d93c1 100644 --- a/modules/cron/tasks_basic.go +++ b/modules/cron/tasks_basic.go @@ -116,7 +116,7 @@ func registerCleanupHookTaskTable() { Schedule: "@every 24h", }, CleanupType: "OlderThan", - OlderThan: 24 * time.Hour * 10, + OlderThan: 24 * time.Hour * 30, NumberToKeep: 10, }, func(ctx context.Context, _ *models.User, config Config) error { realConfig := config.(*CleanupHookTaskConfig) From 49304b176e9eb3af9bf0862e45501b3a04b9ee96 Mon Sep 17 00:00:00 2001 From: Brad Albright Date: Thu, 8 Oct 2020 14:47:02 -0500 Subject: [PATCH 07/18] fixed defaults --- custom/conf/app.example.ini | 2 +- .../content/doc/advanced/config-cheat-sheet.en-us.md | 2 +- models/webhook.go | 12 ++++++------ models/webhook_test.go | 12 ++++++------ modules/cron/tasks_basic.go | 3 ++- 5 files changed, 16 insertions(+), 15 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index e77e3dfa37b61..227766ccf9e59 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -993,7 +993,7 @@ SCHEDULE = @every 24h ; OlderThan or PerWebhook. How the records are removed, either by age (i.e. how long ago hook_task record was delivered) or by the number to keep per webhook (i.e. keep most recent x deliveries per webhook). CLEANUP_TYPE = OlderThan ; If CLEANUP_TYPE is set to OlderThan, then any delivered hook_task records older than this expression will be deleted. -OLDER_THAN = 30d +OLDER_THAN = 168h ; If CLEANUP_TYPE is set to PerWebhook, this is number of hook_task records to keep for a webhook (i.e. keep the most recent x deliveries). NUMBER_TO_KEEP = 10 diff --git a/docs/content/doc/advanced/config-cheat-sheet.en-us.md b/docs/content/doc/advanced/config-cheat-sheet.en-us.md index ba4a7980b9135..90e5ef355e309 100644 --- a/docs/content/doc/advanced/config-cheat-sheet.en-us.md +++ b/docs/content/doc/advanced/config-cheat-sheet.en-us.md @@ -691,7 +691,7 @@ NB: You must `REDIRECT_MACARON_LOG` and have `DISABLE_ROUTER_LOG` set to `false` - `RUN_AT_START`: **false**: Run cleanup hook_task at start time (if ENABLED). - `SCHEDULE`: **@every 24h**: Cron syntax for cleaning hook_task table. - `CLEANUP_TYPE` **OlderThan** OlderThan or PerWebhook Method to cleanup hook_task, either by age (i.e. how long ago hook_task record was delivered) or by the number to keep per webhook (i.e. keep most recent x deliveries per webhook). -- `OLDER_THAN`: **30d**: If CLEANUP_TYPE is set to OlderThan, then any delivered hook_task records older than this expression will be deleted. +- `OLDER_THAN`: **168h**: If CLEANUP_TYPE is set to OlderThan, then any delivered hook_task records older than this expression will be deleted. - `NUMBER_TO_KEEP`: **10**: If CLEANUP_TYPE is set to PerWebhook, this is number of hook_task records to keep for a webhook (i.e. keep the most recent x deliveries). ### Cron - Update Migration Poster ID (`cron.update_migration_poster_id`) diff --git a/models/webhook.go b/models/webhook.go index 5a9571c7260bb..172b0e6891973 100644 --- a/models/webhook.go +++ b/models/webhook.go @@ -822,7 +822,7 @@ func FindRepoUndeliveredHookTasks(repoID int64) ([]*HookTask, error) { // CleanupHookTaskTable deletes rows from hook_task as needed. func CleanupHookTaskTable(ctx context.Context, cleanupType HookTaskCleanupType, olderThan time.Duration, numberToKeep int) error { - log.Error("Doing: CleanupHookTaskTable") + log.Trace("Doing: CleanupHookTaskTable") if cleanupType == OlderThan { deleteOlderThan := time.Now().Add(-olderThan).UnixNano() @@ -832,7 +832,7 @@ func CleanupHookTaskTable(ctx context.Context, cleanupType HookTaskCleanupType, if err != nil { return err } - log.Error("Deleted %d rows from hook_task older than %d", deletes, olderThan) + log.Trace("Deleted %d rows from hook_task", deletes) } else if cleanupType == PerWebhook { hookIDs := make([]int64, 0, 10) err := x.Table("webhook"). @@ -853,12 +853,12 @@ func CleanupHookTaskTable(ctx context.Context, cleanupType HookTaskCleanupType, } } } - log.Error("Finished: CleanupHookTaskTable") + log.Trace("Finished: CleanupHookTaskTable") return nil } func deleteDeliveredHookTasksByWebhook(hookID int64, numberDeliveriesToKeep int) error { - log.Error("Deleting hook_task rows for webhook %d, keeping the most recent %d deliveries", hookID, numberDeliveriesToKeep) + log.Trace("Deleting hook_task rows for webhook %d, keeping the most recent %d deliveries", hookID, numberDeliveriesToKeep) var deliveryDates = make([]int64, 0, 10) err := x.Table("hook_task"). Where("hook_task.hook_id = ? AND hook_task.is_delivered = ?", hookID, true). @@ -878,9 +878,9 @@ func deleteDeliveredHookTasksByWebhook(hookID int64, numberDeliveriesToKeep int) if err != nil { return err } - log.Error("Deleted %d hook_task rows for webhook %d", deletes, hookID) + log.Trace("Deleted %d hook_task rows for webhook %d", deletes, hookID) } else { - log.Error("No hook_task rows to delete for webhook %d", hookID) + log.Trace("No hook_task rows to delete for webhook %d", hookID) } return nil diff --git a/models/webhook_test.go b/models/webhook_test.go index 224d06c98659d..50c604d7efcdc 100644 --- a/models/webhook_test.go +++ b/models/webhook_test.go @@ -312,13 +312,13 @@ func TestCleanupHookTaskTable_Age_DeletesDelivered(t *testing.T) { URL: "http://www.example.com/unit_test", Payloader: &api.PushPayload{}, IsDelivered: true, - Delivered: time.Now().AddDate(0, 0, 31).UnixNano(), + Delivered: time.Now().AddDate(0, 0, 8).UnixNano(), } AssertNotExistsBean(t, hookTask) assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, CleanupHookTaskTable(context.Background(), OlderThan, 24 * time.Hour * 30, 0)) + assert.NoError(t, CleanupHookTaskTable(context.Background(), OlderThan, 168 * time.Hour, 0)) AssertNotExistsBean(t, hookTask) } @@ -331,13 +331,13 @@ func TestCleanupHookTaskTable_Age_LeavesUndelivered(t *testing.T) { URL: "http://www.example.com/unit_test", Payloader: &api.PushPayload{}, IsDelivered: false, - Delivered: time.Now().AddDate(0, 0, 31).UnixNano(), + Delivered: time.Now().AddDate(0, 0, 8).UnixNano(), } AssertNotExistsBean(t, hookTask) assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, CleanupHookTaskTable(context.Background(), OlderThan, 24 * time.Hour * 30, 0)) + assert.NoError(t, CleanupHookTaskTable(context.Background(), OlderThan, 168 * time.Hour, 0)) AssertExistsAndLoadBean(t, hookTask) } @@ -350,12 +350,12 @@ func TestCleanupHookTaskTable_Age_LeavesTaskEarlierThanAgeToDelete(t *testing.T) URL: "http://www.example.com/unit_test", Payloader: &api.PushPayload{}, IsDelivered: true, - Delivered: time.Now().AddDate(0, 0, 29).UnixNano(), + Delivered: time.Now().AddDate(0, 0, 6).UnixNano(), } AssertNotExistsBean(t, hookTask) assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, CleanupHookTaskTable(context.Background(), OlderThan, 24 * time.Hour * 30, 0)) + assert.NoError(t, CleanupHookTaskTable(context.Background(), OlderThan, 168 * time.Hour, 0)) AssertExistsAndLoadBean(t, hookTask) } diff --git a/modules/cron/tasks_basic.go b/modules/cron/tasks_basic.go index ffccbb44d93c1..efd36ae58d51f 100644 --- a/modules/cron/tasks_basic.go +++ b/modules/cron/tasks_basic.go @@ -116,7 +116,7 @@ func registerCleanupHookTaskTable() { Schedule: "@every 24h", }, CleanupType: "OlderThan", - OlderThan: 24 * time.Hour * 30, + OlderThan: 168 * time.Hour, NumberToKeep: 10, }, func(ctx context.Context, _ *models.User, config Config) error { realConfig := config.(*CleanupHookTaskConfig) @@ -124,6 +124,7 @@ func registerCleanupHookTaskTable() { }) } + func initBasicTasks() { registerUpdateMirrorTask() registerRepoHealthCheck() From 693a6bdae0c3a3df393ec1db333529b6577c99b4 Mon Sep 17 00:00:00 2001 From: Brad Albright Date: Thu, 8 Oct 2020 14:50:49 -0500 Subject: [PATCH 08/18] gofmt --- models/webhook_test.go | 6 +++--- modules/cron/tasks_basic.go | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/models/webhook_test.go b/models/webhook_test.go index 50c604d7efcdc..724372f2487c6 100644 --- a/models/webhook_test.go +++ b/models/webhook_test.go @@ -318,7 +318,7 @@ func TestCleanupHookTaskTable_Age_DeletesDelivered(t *testing.T) { assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, CleanupHookTaskTable(context.Background(), OlderThan, 168 * time.Hour, 0)) + assert.NoError(t, CleanupHookTaskTable(context.Background(), OlderThan, 168*time.Hour, 0)) AssertNotExistsBean(t, hookTask) } @@ -337,7 +337,7 @@ func TestCleanupHookTaskTable_Age_LeavesUndelivered(t *testing.T) { assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, CleanupHookTaskTable(context.Background(), OlderThan, 168 * time.Hour, 0)) + assert.NoError(t, CleanupHookTaskTable(context.Background(), OlderThan, 168*time.Hour, 0)) AssertExistsAndLoadBean(t, hookTask) } @@ -356,6 +356,6 @@ func TestCleanupHookTaskTable_Age_LeavesTaskEarlierThanAgeToDelete(t *testing.T) assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, CleanupHookTaskTable(context.Background(), OlderThan, 168 * time.Hour, 0)) + assert.NoError(t, CleanupHookTaskTable(context.Background(), OlderThan, 168*time.Hour, 0)) AssertExistsAndLoadBean(t, hookTask) } diff --git a/modules/cron/tasks_basic.go b/modules/cron/tasks_basic.go index efd36ae58d51f..439f839ecb6a3 100644 --- a/modules/cron/tasks_basic.go +++ b/modules/cron/tasks_basic.go @@ -124,7 +124,6 @@ func registerCleanupHookTaskTable() { }) } - func initBasicTasks() { registerUpdateMirrorTask() registerRepoHealthCheck() From d1fe028a8860e604d03b90bc0b259dad3ba0e472 Mon Sep 17 00:00:00 2001 From: Brad Albright Date: Thu, 8 Oct 2020 22:00:28 -0500 Subject: [PATCH 09/18] fixed method names on cleanup hook task unit tests --- models/webhook_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/models/webhook_test.go b/models/webhook_test.go index 724372f2487c6..b7232dc296dc3 100644 --- a/models/webhook_test.go +++ b/models/webhook_test.go @@ -303,7 +303,7 @@ func TestCleanupHookTaskTable_PerWebhook_LeavesMostRecentTask(t *testing.T) { AssertExistsAndLoadBean(t, hookTask) } -func TestCleanupHookTaskTable_Age_DeletesDelivered(t *testing.T) { +func TestCleanupHookTaskTable_OlderThan_DeletesDelivered(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) hookTask := &HookTask{ RepoID: 3, @@ -322,7 +322,7 @@ func TestCleanupHookTaskTable_Age_DeletesDelivered(t *testing.T) { AssertNotExistsBean(t, hookTask) } -func TestCleanupHookTaskTable_Age_LeavesUndelivered(t *testing.T) { +func TestCleanupHookTaskTable_OlderThan_LeavesUndelivered(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) hookTask := &HookTask{ RepoID: 2, @@ -341,7 +341,7 @@ func TestCleanupHookTaskTable_Age_LeavesUndelivered(t *testing.T) { AssertExistsAndLoadBean(t, hookTask) } -func TestCleanupHookTaskTable_Age_LeavesTaskEarlierThanAgeToDelete(t *testing.T) { +func TestCleanupHookTaskTable_OlderThan_LeavesTaskEarlierThanAgeToDelete(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) hookTask := &HookTask{ RepoID: 2, From cfc2984cfe4556a0d190888204a4b9e590c34d93 Mon Sep 17 00:00:00 2001 From: Brad Albright Date: Fri, 9 Oct 2020 12:13:45 -0500 Subject: [PATCH 10/18] fixed unit test --- models/webhook_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/models/webhook_test.go b/models/webhook_test.go index b7232dc296dc3..f21a79f2916e1 100644 --- a/models/webhook_test.go +++ b/models/webhook_test.go @@ -257,6 +257,7 @@ func TestCleanupHookTaskTable_PerWebhook_DeletesDelivered(t *testing.T) { URL: "http://www.example.com/unit_test", Payloader: &api.PushPayload{}, IsDelivered: true, + Delivered: time.Now().UnixNano(), } AssertNotExistsBean(t, hookTask) assert.NoError(t, CreateHookTask(hookTask)) @@ -275,6 +276,7 @@ func TestCleanupHookTaskTable_PerWebhook_LeavesUndelivered(t *testing.T) { URL: "http://www.example.com/unit_test", Payloader: &api.PushPayload{}, IsDelivered: false, + Delivered: time.Now().UnixNano(), } AssertNotExistsBean(t, hookTask) assert.NoError(t, CreateHookTask(hookTask)) From b49384e72d35b7b09790d4783cf431ab442bdbc9 Mon Sep 17 00:00:00 2001 From: Brad Albright Date: Fri, 9 Oct 2020 12:49:58 -0500 Subject: [PATCH 11/18] attempt to fix unit test --- models/webhook_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/models/webhook_test.go b/models/webhook_test.go index f21a79f2916e1..97280b661f239 100644 --- a/models/webhook_test.go +++ b/models/webhook_test.go @@ -263,7 +263,7 @@ func TestCleanupHookTaskTable_PerWebhook_DeletesDelivered(t *testing.T) { assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, CleanupHookTaskTable(context.Background(), PerWebhook, 0, 0)) + assert.NoError(t, CleanupHookTaskTable(context.Background(), PerWebhook, 168 * time.Hour, 0)) AssertNotExistsBean(t, hookTask) } @@ -282,7 +282,7 @@ func TestCleanupHookTaskTable_PerWebhook_LeavesUndelivered(t *testing.T) { assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, CleanupHookTaskTable(context.Background(), PerWebhook, 0, 0)) + assert.NoError(t, CleanupHookTaskTable(context.Background(), PerWebhook, 168 * time.Hour, 0)) AssertExistsAndLoadBean(t, hookTask) } @@ -301,7 +301,7 @@ func TestCleanupHookTaskTable_PerWebhook_LeavesMostRecentTask(t *testing.T) { assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, CleanupHookTaskTable(context.Background(), PerWebhook, 0, 1)) + assert.NoError(t, CleanupHookTaskTable(context.Background(), PerWebhook, 168 * time.Hour, 1)) AssertExistsAndLoadBean(t, hookTask) } From 9699c77844f839a892034d0c66305e7697a9e2e7 Mon Sep 17 00:00:00 2001 From: Brad Albright Date: Fri, 9 Oct 2020 14:02:35 -0500 Subject: [PATCH 12/18] gofmt --- models/webhook_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/models/webhook_test.go b/models/webhook_test.go index 97280b661f239..1a57aabf64e70 100644 --- a/models/webhook_test.go +++ b/models/webhook_test.go @@ -263,7 +263,7 @@ func TestCleanupHookTaskTable_PerWebhook_DeletesDelivered(t *testing.T) { assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, CleanupHookTaskTable(context.Background(), PerWebhook, 168 * time.Hour, 0)) + assert.NoError(t, CleanupHookTaskTable(context.Background(), PerWebhook, 168*time.Hour, 0)) AssertNotExistsBean(t, hookTask) } @@ -282,7 +282,7 @@ func TestCleanupHookTaskTable_PerWebhook_LeavesUndelivered(t *testing.T) { assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, CleanupHookTaskTable(context.Background(), PerWebhook, 168 * time.Hour, 0)) + assert.NoError(t, CleanupHookTaskTable(context.Background(), PerWebhook, 168*time.Hour, 0)) AssertExistsAndLoadBean(t, hookTask) } @@ -301,7 +301,7 @@ func TestCleanupHookTaskTable_PerWebhook_LeavesMostRecentTask(t *testing.T) { assert.NoError(t, CreateHookTask(hookTask)) AssertExistsAndLoadBean(t, hookTask) - assert.NoError(t, CleanupHookTaskTable(context.Background(), PerWebhook, 168 * time.Hour, 1)) + assert.NoError(t, CleanupHookTaskTable(context.Background(), PerWebhook, 168*time.Hour, 1)) AssertExistsAndLoadBean(t, hookTask) } From cbe3d89d33e30768f23650fc7c31ee34cfdf06b4 Mon Sep 17 00:00:00 2001 From: Brad Albright Date: Fri, 9 Oct 2020 18:52:11 -0500 Subject: [PATCH 13/18] working on unit test --- models/webhook_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/models/webhook_test.go b/models/webhook_test.go index 1a57aabf64e70..87837d6e527d1 100644 --- a/models/webhook_test.go +++ b/models/webhook_test.go @@ -276,7 +276,6 @@ func TestCleanupHookTaskTable_PerWebhook_LeavesUndelivered(t *testing.T) { URL: "http://www.example.com/unit_test", Payloader: &api.PushPayload{}, IsDelivered: false, - Delivered: time.Now().UnixNano(), } AssertNotExistsBean(t, hookTask) assert.NoError(t, CreateHookTask(hookTask)) @@ -314,7 +313,7 @@ func TestCleanupHookTaskTable_OlderThan_DeletesDelivered(t *testing.T) { URL: "http://www.example.com/unit_test", Payloader: &api.PushPayload{}, IsDelivered: true, - Delivered: time.Now().AddDate(0, 0, 8).UnixNano(), + Delivered: time.Now().AddDate(0, 0, -8).UnixNano(), } AssertNotExistsBean(t, hookTask) assert.NoError(t, CreateHookTask(hookTask)) @@ -333,7 +332,6 @@ func TestCleanupHookTaskTable_OlderThan_LeavesUndelivered(t *testing.T) { URL: "http://www.example.com/unit_test", Payloader: &api.PushPayload{}, IsDelivered: false, - Delivered: time.Now().AddDate(0, 0, 8).UnixNano(), } AssertNotExistsBean(t, hookTask) assert.NoError(t, CreateHookTask(hookTask)) @@ -352,7 +350,7 @@ func TestCleanupHookTaskTable_OlderThan_LeavesTaskEarlierThanAgeToDelete(t *test URL: "http://www.example.com/unit_test", Payloader: &api.PushPayload{}, IsDelivered: true, - Delivered: time.Now().AddDate(0, 0, 6).UnixNano(), + Delivered: time.Now().AddDate(0, 0, -6).UnixNano(), } AssertNotExistsBean(t, hookTask) assert.NoError(t, CreateHookTask(hookTask)) From 809f333cba80ebc996306b470d28d905cef7f678 Mon Sep 17 00:00:00 2001 From: Brad Albright Date: Fri, 9 Oct 2020 19:03:42 -0500 Subject: [PATCH 14/18] added delivered date to hook task test that updates delivered to true --- models/webhook_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/models/webhook_test.go b/models/webhook_test.go index 87837d6e527d1..b2fb24de3abc1 100644 --- a/models/webhook_test.go +++ b/models/webhook_test.go @@ -243,6 +243,7 @@ func TestUpdateHookTask(t *testing.T) { hook.PayloadContent = "new payload content" hook.DeliveredString = "new delivered string" hook.IsDelivered = true + hook.Delivered = time.Now().UnixNano() AssertNotExistsBean(t, hook) assert.NoError(t, UpdateHookTask(hook)) AssertExistsAndLoadBean(t, hook) From 89df8c389454d957c1680b694ea815f6130b9dec Mon Sep 17 00:00:00 2001 From: Brad Albright Date: Fri, 9 Oct 2020 22:59:54 -0500 Subject: [PATCH 15/18] gofmt --- models/webhook_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/webhook_test.go b/models/webhook_test.go index b2fb24de3abc1..c9f39ccb53d5f 100644 --- a/models/webhook_test.go +++ b/models/webhook_test.go @@ -243,7 +243,7 @@ func TestUpdateHookTask(t *testing.T) { hook.PayloadContent = "new payload content" hook.DeliveredString = "new delivered string" hook.IsDelivered = true - hook.Delivered = time.Now().UnixNano() + hook.Delivered = time.Now().UnixNano() AssertNotExistsBean(t, hook) assert.NoError(t, UpdateHookTask(hook)) AssertExistsAndLoadBean(t, hook) From 04faa7c5837737a3e5a09f3ae824b47f3708f71c Mon Sep 17 00:00:00 2001 From: Brad Albright Date: Sat, 10 Oct 2020 18:41:39 -0500 Subject: [PATCH 16/18] added null check --- models/webhook.go | 2 +- models/webhook_test.go | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/models/webhook.go b/models/webhook.go index 172b0e6891973..9537232f04ee2 100644 --- a/models/webhook.go +++ b/models/webhook.go @@ -861,7 +861,7 @@ func deleteDeliveredHookTasksByWebhook(hookID int64, numberDeliveriesToKeep int) log.Trace("Deleting hook_task rows for webhook %d, keeping the most recent %d deliveries", hookID, numberDeliveriesToKeep) var deliveryDates = make([]int64, 0, 10) err := x.Table("hook_task"). - Where("hook_task.hook_id = ? AND hook_task.is_delivered = ?", hookID, true). + Where("hook_task.hook_id = ? AND hook_task.is_delivered = ? AND hook_task.delivered is not null", hookID, true). Cols("hook_task.delivered"). Join("INNER", "webhook", "hook_task.hook_id = webhook.id"). OrderBy("hook_task.delivered desc"). diff --git a/models/webhook_test.go b/models/webhook_test.go index c9f39ccb53d5f..87837d6e527d1 100644 --- a/models/webhook_test.go +++ b/models/webhook_test.go @@ -243,7 +243,6 @@ func TestUpdateHookTask(t *testing.T) { hook.PayloadContent = "new payload content" hook.DeliveredString = "new delivered string" hook.IsDelivered = true - hook.Delivered = time.Now().UnixNano() AssertNotExistsBean(t, hook) assert.NoError(t, UpdateHookTask(hook)) AssertExistsAndLoadBean(t, hook) From b8afeb27366b59b2e33658ed875806f6a388dbe8 Mon Sep 17 00:00:00 2001 From: Brad Albright Date: Tue, 19 Jan 2021 22:01:36 -0600 Subject: [PATCH 17/18] fix lint errors --- models/webhook_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/models/webhook_test.go b/models/webhook_test.go index 8544e19eeae98..1baf6ef44bfa5 100644 --- a/models/webhook_test.go +++ b/models/webhook_test.go @@ -231,7 +231,7 @@ func TestCleanupHookTaskTable_PerWebhook_DeletesDelivered(t *testing.T) { hookTask := &HookTask{ RepoID: 3, HookID: 3, - Type: GITEA, + Typ: GITEA, URL: "http://www.example.com/unit_test", Payloader: &api.PushPayload{}, IsDelivered: true, @@ -250,7 +250,7 @@ func TestCleanupHookTaskTable_PerWebhook_LeavesUndelivered(t *testing.T) { hookTask := &HookTask{ RepoID: 2, HookID: 4, - Type: GITEA, + Typ: GITEA, URL: "http://www.example.com/unit_test", Payloader: &api.PushPayload{}, IsDelivered: false, @@ -268,7 +268,7 @@ func TestCleanupHookTaskTable_PerWebhook_LeavesMostRecentTask(t *testing.T) { hookTask := &HookTask{ RepoID: 2, HookID: 4, - Type: GITEA, + Typ: GITEA, URL: "http://www.example.com/unit_test", Payloader: &api.PushPayload{}, IsDelivered: true, @@ -287,7 +287,7 @@ func TestCleanupHookTaskTable_OlderThan_DeletesDelivered(t *testing.T) { hookTask := &HookTask{ RepoID: 3, HookID: 3, - Type: GITEA, + Typ: GITEA, URL: "http://www.example.com/unit_test", Payloader: &api.PushPayload{}, IsDelivered: true, @@ -306,7 +306,7 @@ func TestCleanupHookTaskTable_OlderThan_LeavesUndelivered(t *testing.T) { hookTask := &HookTask{ RepoID: 2, HookID: 4, - Type: GITEA, + Typ: GITEA, URL: "http://www.example.com/unit_test", Payloader: &api.PushPayload{}, IsDelivered: false, @@ -324,7 +324,7 @@ func TestCleanupHookTaskTable_OlderThan_LeavesTaskEarlierThanAgeToDelete(t *test hookTask := &HookTask{ RepoID: 2, HookID: 4, - Type: GITEA, + Typ: GITEA, URL: "http://www.example.com/unit_test", Payloader: &api.PushPayload{}, IsDelivered: true, From 2e4710e417a23b68d70f885af625d3aef8131a39 Mon Sep 17 00:00:00 2001 From: Brad Albright Date: Tue, 19 Jan 2021 22:07:02 -0600 Subject: [PATCH 18/18] ran gofmt --- modules/cron/tasks_basic.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/cron/tasks_basic.go b/modules/cron/tasks_basic.go index da32064baf605..391cda0f891f5 100644 --- a/modules/cron/tasks_basic.go +++ b/modules/cron/tasks_basic.go @@ -135,5 +135,5 @@ func initBasicTasks() { if !setting.Repository.DisableMigrations { registerUpdateMigrationPosterID() } - registerCleanupHookTaskTable() + registerCleanupHookTaskTable() }