From b99f9f86b11f85a6c6d4f6bd637cd604da3ff3c5 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Wed, 1 Mar 2023 17:50:21 +0800 Subject: [PATCH 1/4] fix: improve GetPushEventPayload --- models/actions/run.go | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/models/actions/run.go b/models/actions/run.go index d5ab45a51958a..954f13565ec5e 100644 --- a/models/actions/run.go +++ b/models/actions/run.go @@ -119,13 +119,29 @@ func (run *ActionRun) Duration() time.Duration { func (run *ActionRun) GetPushEventPayload() (*api.PushPayload, error) { if run.Event == webhook_module.HookEventPush { - var payload api.PushPayload - if err := json.Unmarshal([]byte(run.EventPayload), &payload); err != nil { - return nil, err - } - return &payload, nil + return nil, fmt.Errorf("event %s is not a push event", run.Event) + } + + payload := &api.PushPayload{} + if err := json.Unmarshal([]byte(run.EventPayload), payload); err != nil { + return nil, err + } + + // Since it comes from json unmarshal, we should check if it's broken + if payload.HeadCommit == nil { + return nil, fmt.Errorf("nil HeadCommit") + } + if payload.Repo == nil { + return nil, fmt.Errorf("nil Repo") + } + if payload.Pusher == nil { + return nil, fmt.Errorf("nil Pusher") } - return nil, fmt.Errorf("event %s is not a push event", run.Event) + if payload.Sender == nil { + return nil, fmt.Errorf("nil Sender") + } + + return payload, nil } func updateRepoRunsNumbers(ctx context.Context, repo *repo_model.Repository) error { From e6f01ccac79a7823f4681cdda9e99ce39c21bbe4 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Wed, 1 Mar 2023 17:50:48 +0800 Subject: [PATCH 2/4] Revert "fix: improve GetPushEventPayload" This reverts commit b99f9f86b11f85a6c6d4f6bd637cd604da3ff3c5. --- models/actions/run.go | 28 ++++++---------------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/models/actions/run.go b/models/actions/run.go index 954f13565ec5e..d5ab45a51958a 100644 --- a/models/actions/run.go +++ b/models/actions/run.go @@ -119,29 +119,13 @@ func (run *ActionRun) Duration() time.Duration { func (run *ActionRun) GetPushEventPayload() (*api.PushPayload, error) { if run.Event == webhook_module.HookEventPush { - return nil, fmt.Errorf("event %s is not a push event", run.Event) - } - - payload := &api.PushPayload{} - if err := json.Unmarshal([]byte(run.EventPayload), payload); err != nil { - return nil, err - } - - // Since it comes from json unmarshal, we should check if it's broken - if payload.HeadCommit == nil { - return nil, fmt.Errorf("nil HeadCommit") - } - if payload.Repo == nil { - return nil, fmt.Errorf("nil Repo") - } - if payload.Pusher == nil { - return nil, fmt.Errorf("nil Pusher") - } - if payload.Sender == nil { - return nil, fmt.Errorf("nil Sender") + var payload api.PushPayload + if err := json.Unmarshal([]byte(run.EventPayload), &payload); err != nil { + return nil, err + } + return &payload, nil } - - return payload, nil + return nil, fmt.Errorf("event %s is not a push event", run.Event) } func updateRepoRunsNumbers(ctx context.Context, repo *repo_model.Repository) error { From 3222b78f3b143c73e6d822e3618b4aa996404828 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Wed, 1 Mar 2023 18:09:45 +0800 Subject: [PATCH 3/4] feat: improve CreateCommitStatus --- routers/api/actions/runner/runner.go | 2 +- routers/web/repo/actions/view.go | 22 +++++++++++++++------- services/actions/clear_tasks.go | 22 +++++++++++++++++----- services/actions/commit_status.go | 10 ++++++++++ services/actions/notifier_helper.go | 3 ++- 5 files changed, 45 insertions(+), 14 deletions(-) diff --git a/routers/api/actions/runner/runner.go b/routers/api/actions/runner/runner.go index d0bfb2363e0c5..839f776fe8de7 100644 --- a/routers/api/actions/runner/runner.go +++ b/routers/api/actions/runner/runner.go @@ -150,7 +150,7 @@ func (s *Service) UpdateTask( } if err := actions_service.CreateCommitStatus(ctx, task.Job); err != nil { - log.Error("Update commit status failed: %v", err) + log.Error("Update commit status for job %v failed: %v", task.Job.ID, err) // go on } diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index dd2750f905883..d8f2661b2beb1 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -15,6 +15,7 @@ import ( "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/modules/actions" context_module "code.gitea.io/gitea/modules/context" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" @@ -212,15 +213,18 @@ func Rerun(ctx *context_module.Context) { job.Stopped = 0 if err := db.WithTx(ctx, func(ctx context.Context) error { - if _, err := actions_model.UpdateRunJob(ctx, job, builder.Eq{"status": status}, "task_id", "status", "started", "stopped"); err != nil { - return err - } - return actions_service.CreateCommitStatus(ctx, job) + _, err := actions_model.UpdateRunJob(ctx, job, builder.Eq{"status": status}, "task_id", "status", "started", "stopped") + return err }); err != nil { ctx.Error(http.StatusInternalServerError, err.Error()) return } + if err := actions_service.CreateCommitStatus(ctx, job); err != nil { + log.Error("Update commit status for job %v failed: %v", job.ID, err) + // go on + } + ctx.JSON(http.StatusOK, struct{}{}) } @@ -253,9 +257,6 @@ func Cancel(ctx *context_module.Context) { if err := actions_model.StopTask(ctx, job.TaskID, actions_model.StatusCancelled); err != nil { return err } - if err := actions_service.CreateCommitStatus(ctx, job); err != nil { - return err - } } return nil }); err != nil { @@ -263,6 +264,13 @@ func Cancel(ctx *context_module.Context) { return } + for _, job := range jobs { + if err := actions_service.CreateCommitStatus(ctx, job); err != nil { + log.Error("Update commit status for job %v failed: %v", job.ID, err) + // go on + } + } + ctx.JSON(http.StatusOK, struct{}{}) } diff --git a/services/actions/clear_tasks.go b/services/actions/clear_tasks.go index 583e588de4054..6f8e95218d678 100644 --- a/services/actions/clear_tasks.go +++ b/services/actions/clear_tasks.go @@ -43,6 +43,7 @@ func stopTasks(ctx context.Context, opts actions_model.FindTaskOptions) error { return fmt.Errorf("find tasks: %w", err) } + jobs := make([]*actions_model.ActionRunJob, 0, len(tasks)) for _, task := range tasks { if err := db.WithTx(ctx, func(ctx context.Context) error { if err := actions_model.StopTask(ctx, task.ID, actions_model.StatusFailure); err != nil { @@ -51,7 +52,8 @@ func stopTasks(ctx context.Context, opts actions_model.FindTaskOptions) error { if err := task.LoadJob(ctx); err != nil { return err } - return CreateCommitStatus(ctx, task.Job) + jobs = append(jobs, task.Job) + return nil }); err != nil { log.Warn("Cannot stop task %v: %v", task.ID, err) // go on @@ -61,6 +63,14 @@ func stopTasks(ctx context.Context, opts actions_model.FindTaskOptions) error { remove() } } + + for _, job := range jobs { + if err := CreateCommitStatus(ctx, job); err != nil { + log.Error("Update commit status for job %v failed: %v", job.ID, err) + // go on + } + } + return nil } @@ -80,14 +90,16 @@ func CancelAbandonedJobs(ctx context.Context) error { job.Status = actions_model.StatusCancelled job.Stopped = now if err := db.WithTx(ctx, func(ctx context.Context) error { - if _, err := actions_model.UpdateRunJob(ctx, job, nil, "status", "stopped"); err != nil { - return err - } - return CreateCommitStatus(ctx, job) + _, err := actions_model.UpdateRunJob(ctx, job, nil, "status", "stopped") + return err }); err != nil { log.Warn("cancel abandoned job %v: %v", job.ID, err) // go on } + if err := CreateCommitStatus(ctx, job); err != nil { + log.Error("Update commit status for job %v failed: %v", job.ID, err) + // go on + } } return nil diff --git a/services/actions/commit_status.go b/services/actions/commit_status.go index efb5ec6d44a3c..410d8e785210e 100644 --- a/services/actions/commit_status.go +++ b/services/actions/commit_status.go @@ -30,6 +30,16 @@ func CreateCommitStatus(ctx context.Context, job *actions_model.ActionRunJob) er return fmt.Errorf("GetPushEventPayload: %w", err) } + // Since the payload comes from json data, we should check if it's broken, or it will cause panic + switch { + case payload.Repo == nil: + return fmt.Errorf("miss Repo in event payload") + case payload.Pusher == nil: + return fmt.Errorf("miss Pusher in event payload") + case payload.HeadCommit == nil: + return fmt.Errorf("miss HeadCommit in event payload") + } + creator, err := user_model.GetUserByID(ctx, payload.Pusher.ID) if err != nil { return fmt.Errorf("GetUserByID: %w", err) diff --git a/services/actions/notifier_helper.go b/services/actions/notifier_helper.go index ef63b8cf94641..574a37e9ab168 100644 --- a/services/actions/notifier_helper.go +++ b/services/actions/notifier_helper.go @@ -187,7 +187,8 @@ func notify(ctx context.Context, input *notifyInput) error { } else { for _, job := range jobs { if err := CreateCommitStatus(ctx, job); err != nil { - log.Error("CreateCommitStatus: %v", err) + log.Error("Update commit status for job %v failed: %v", job.ID, err) + // go on } } } From b8ca75bde551846dc936a449f116183fd5c9db39 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Sat, 4 Mar 2023 08:23:37 +0800 Subject: [PATCH 4/4] Update services/actions/commit_status.go Co-authored-by: delvh --- services/actions/commit_status.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/services/actions/commit_status.go b/services/actions/commit_status.go index 410d8e785210e..4f313493523ee 100644 --- a/services/actions/commit_status.go +++ b/services/actions/commit_status.go @@ -33,11 +33,11 @@ func CreateCommitStatus(ctx context.Context, job *actions_model.ActionRunJob) er // Since the payload comes from json data, we should check if it's broken, or it will cause panic switch { case payload.Repo == nil: - return fmt.Errorf("miss Repo in event payload") + return fmt.Errorf("repo is missing in event payload") case payload.Pusher == nil: - return fmt.Errorf("miss Pusher in event payload") + return fmt.Errorf("pusher is missing in event payload") case payload.HeadCommit == nil: - return fmt.Errorf("miss HeadCommit in event payload") + return fmt.Errorf("head commit is missing in event payload") } creator, err := user_model.GetUserByID(ctx, payload.Pusher.ID)