From 3e8db31a5b5a7be731b19da4bd666b0ddc35bd31 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Wed, 29 Mar 2023 23:27:37 +0800 Subject: [PATCH] Refactor commit status for Actions jobs (#23786) Before: xnip_230329_163852 After: xnip_230329_163358 Highlights: - Treat `StatusSkipped` as `CommitStatusSuccess` instead of `CommitStatusFailure`, so it fixed #23599. - Use the bot user `gitea-actions` instead of the trigger as the creator of commit status. - New format ` / / ()` for the context of commit status to avoid conflicts. - Add descriptions for commit status. - Add the missing calls to `CreateCommitStatus`. - Refactor `CreateCommitStatus` to make it easier to use. --- routers/api/actions/runner/runner.go | 5 +- routers/api/actions/runner/utils.go | 3 + routers/web/repo/actions/view.go | 15 ++--- services/actions/clear_tasks.go | 12 +--- services/actions/commit_status.go | 89 +++++++++++++++++----------- services/actions/job_emitter.go | 16 +++-- services/actions/notifier_helper.go | 7 +-- 7 files changed, 76 insertions(+), 71 deletions(-) diff --git a/routers/api/actions/runner/runner.go b/routers/api/actions/runner/runner.go index 07657c91207e..a44586485893 100644 --- a/routers/api/actions/runner/runner.go +++ b/routers/api/actions/runner/runner.go @@ -149,10 +149,7 @@ func (s *Service) UpdateTask( return nil, status.Errorf(codes.Internal, "load job: %v", err) } - if err := actions_service.CreateCommitStatus(ctx, task.Job); err != nil { - log.Error("Update commit status for job %v failed: %v", task.Job.ID, err) - // go on - } + actions_service.CreateCommitStatus(ctx, task.Job) if req.Msg.State.Result != runnerv1.Result_RESULT_UNSPECIFIED { if err := actions_service.EmitJobsIfReady(task.Job.RunID); err != nil { diff --git a/routers/api/actions/runner/utils.go b/routers/api/actions/runner/utils.go index 80e71941f21c..a022445ff38a 100644 --- a/routers/api/actions/runner/utils.go +++ b/routers/api/actions/runner/utils.go @@ -13,6 +13,7 @@ import ( "code.gitea.io/gitea/modules/log" secret_module "code.gitea.io/gitea/modules/secret" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/services/actions" runnerv1 "code.gitea.io/actions-proto-go/runner/v1" "google.golang.org/protobuf/types/known/structpb" @@ -27,6 +28,8 @@ func pickTask(ctx context.Context, runner *actions_model.ActionRunner) (*runnerv return nil, false, nil } + actions.CreateCommitStatus(ctx, t.Job) + task := &runnerv1.Task{ Id: t.ID, WorkflowPayload: t.Job.WorkflowPayload, diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index 0fa255b7e65c..b2b625ea23ee 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -16,7 +16,6 @@ import ( "code.gitea.io/gitea/modules/actions" "code.gitea.io/gitea/modules/base" 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" @@ -264,10 +263,7 @@ func Rerun(ctx *context_module.Context) { 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 - } + actions_service.CreateCommitStatus(ctx, job) ctx.JSON(http.StatusOK, struct{}{}) } @@ -308,12 +304,7 @@ 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 - } - } + actions_service.CreateCommitStatus(ctx, jobs...) ctx.JSON(http.StatusOK, struct{}{}) } @@ -349,6 +340,8 @@ func Approve(ctx *context_module.Context) { return } + actions_service.CreateCommitStatus(ctx, jobs...) + ctx.JSON(http.StatusOK, struct{}{}) } diff --git a/services/actions/clear_tasks.go b/services/actions/clear_tasks.go index 6f8e95218d67..0616a5fc0dcb 100644 --- a/services/actions/clear_tasks.go +++ b/services/actions/clear_tasks.go @@ -64,12 +64,7 @@ func stopTasks(ctx context.Context, opts actions_model.FindTaskOptions) error { } } - 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 - } - } + CreateCommitStatus(ctx, jobs...) return nil } @@ -96,10 +91,7 @@ func CancelAbandonedJobs(ctx context.Context) error { 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 - } + CreateCommitStatus(ctx, job) } return nil diff --git a/services/actions/commit_status.go b/services/actions/commit_status.go index 6604a149a5c9..984c41295677 100644 --- a/services/actions/commit_status.go +++ b/services/actions/commit_status.go @@ -6,79 +6,80 @@ package actions import ( "context" "fmt" + "path" actions_model "code.gitea.io/gitea/models/actions" "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/log" api "code.gitea.io/gitea/modules/structs" webhook_module "code.gitea.io/gitea/modules/webhook" + + "github.com/nektos/act/pkg/jobparser" ) -func CreateCommitStatus(ctx context.Context, job *actions_model.ActionRunJob) error { +// CreateCommitStatus creates a commit status for the given job. +// It won't return an error failed, but will log it, because it's not critical. +func CreateCommitStatus(ctx context.Context, jobs ...*actions_model.ActionRunJob) { + for _, job := range jobs { + if err := createCommitStatus(ctx, job); err != nil { + log.Error("Failed to create commit status for job %d: %v", job.ID, err) + } + } +} + +func createCommitStatus(ctx context.Context, job *actions_model.ActionRunJob) error { if err := job.LoadAttributes(ctx); err != nil { return fmt.Errorf("load run: %w", err) } run := job.Run - var ( - sha string - creatorID int64 - ) + var ( + sha string + event string + ) switch run.Event { case webhook_module.HookEventPush: + event = "push" payload, err := run.GetPushEventPayload() if err != nil { 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("repo is missing in event payload") - case payload.Pusher == nil: - return fmt.Errorf("pusher is missing in event payload") - case payload.HeadCommit == nil: + if payload.HeadCommit == nil { return fmt.Errorf("head commit is missing in event payload") } - sha = payload.HeadCommit.ID - creatorID = payload.Pusher.ID case webhook_module.HookEventPullRequest, webhook_module.HookEventPullRequestSync: + event = "pull_request" payload, err := run.GetPullRequestEventPayload() if err != nil { return fmt.Errorf("GetPullRequestEventPayload: %w", err) } - - switch { - case payload.PullRequest == nil: + if payload.PullRequest == nil { return fmt.Errorf("pull request is missing in event payload") - case payload.PullRequest.Head == nil: + } else if payload.PullRequest.Head == nil { return fmt.Errorf("head of pull request is missing in event payload") - case payload.PullRequest.Head.Repository == nil: - return fmt.Errorf("head repository of pull request is missing in event payload") - case payload.PullRequest.Head.Repository.Owner == nil: - return fmt.Errorf("owner of head repository of pull request is missing in evnt payload") } - sha = payload.PullRequest.Head.Sha - creatorID = payload.PullRequest.Head.Repository.Owner.ID default: return nil } repo := run.Repo - ctxname := job.Name - state := toCommitStatus(job.Status) - creator, err := user_model.GetUserByID(ctx, creatorID) - if err != nil { - return fmt.Errorf("GetUserByID: %w", err) + // TODO: store workflow name as a field in ActionRun to avoid parsing + runName := path.Base(run.WorkflowID) + if wfs, err := jobparser.Parse(job.WorkflowPayload); err == nil && len(wfs) > 0 { + runName = wfs[0].Name } + ctxname := fmt.Sprintf("%s / %s (%s)", runName, job.Name, event) + state := toCommitStatus(job.Status) if statuses, _, err := git_model.GetLatestCommitStatus(ctx, repo.ID, sha, db.ListOptions{}); err == nil { for _, v := range statuses { if v.Context == ctxname { if v.State == state { + // no need to update return nil } break @@ -88,6 +89,26 @@ func CreateCommitStatus(ctx context.Context, job *actions_model.ActionRunJob) er return fmt.Errorf("GetLatestCommitStatus: %w", err) } + description := "" + switch job.Status { + // TODO: if we want support description in different languages, we need to support i18n placeholders in it + case actions_model.StatusSuccess: + description = fmt.Sprintf("Successful in %s", job.Duration()) + case actions_model.StatusFailure: + description = fmt.Sprintf("Failing after %s", job.Duration()) + case actions_model.StatusCancelled: + description = "Has been cancelled" + case actions_model.StatusSkipped: + description = "Has been skipped" + case actions_model.StatusRunning: + description = "Has started running" + case actions_model.StatusWaiting: + description = "Waiting to run" + case actions_model.StatusBlocked: + description = "Blocked by required conditions" + } + + creator := user_model.NewActionsUser() if err := git_model.NewCommitStatus(ctx, git_model.NewCommitStatusOptions{ Repo: repo, SHA: sha, @@ -95,9 +116,9 @@ func CreateCommitStatus(ctx context.Context, job *actions_model.ActionRunJob) er CommitStatus: &git_model.CommitStatus{ SHA: sha, TargetURL: run.Link(), - Description: "", + Description: description, Context: ctxname, - CreatorID: creatorID, + CreatorID: creator.ID, State: state, }, }); err != nil { @@ -109,9 +130,9 @@ func CreateCommitStatus(ctx context.Context, job *actions_model.ActionRunJob) er func toCommitStatus(status actions_model.Status) api.CommitStatusState { switch status { - case actions_model.StatusSuccess: + case actions_model.StatusSuccess, actions_model.StatusSkipped: return api.CommitStatusSuccess - case actions_model.StatusFailure, actions_model.StatusCancelled, actions_model.StatusSkipped: + case actions_model.StatusFailure, actions_model.StatusCancelled: return api.CommitStatusFailure case actions_model.StatusWaiting, actions_model.StatusBlocked: return api.CommitStatusPending diff --git a/services/actions/job_emitter.go b/services/actions/job_emitter.go index cb2cc8d1ac06..c6b6fc551e00 100644 --- a/services/actions/job_emitter.go +++ b/services/actions/job_emitter.go @@ -45,11 +45,11 @@ func jobEmitterQueueHandle(data ...queue.Data) []queue.Data { } func checkJobsOfRun(ctx context.Context, runID int64) error { - return db.WithTx(ctx, func(ctx context.Context) error { - jobs, _, err := actions_model.FindRunJobs(ctx, actions_model.FindRunJobOptions{RunID: runID}) - if err != nil { - return err - } + jobs, _, err := actions_model.FindRunJobs(ctx, actions_model.FindRunJobOptions{RunID: runID}) + if err != nil { + return err + } + if err := db.WithTx(ctx, func(ctx context.Context) error { idToJobs := make(map[string][]*actions_model.ActionRunJob, len(jobs)) for _, job := range jobs { idToJobs[job.JobID] = append(idToJobs[job.JobID], job) @@ -67,7 +67,11 @@ func checkJobsOfRun(ctx context.Context, runID int64) error { } } return nil - }) + }); err != nil { + return err + } + CreateCommitStatus(ctx, jobs...) + return nil } type jobStatusResolver struct { diff --git a/services/actions/notifier_helper.go b/services/actions/notifier_helper.go index 574a37e9ab16..b0e199fc6bd4 100644 --- a/services/actions/notifier_helper.go +++ b/services/actions/notifier_helper.go @@ -185,12 +185,7 @@ func notify(ctx context.Context, input *notifyInput) error { if jobs, _, err := actions_model.FindRunJobs(ctx, actions_model.FindRunJobOptions{RunID: run.ID}); err != nil { log.Error("FindRunJobs: %v", err) } else { - 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 - } - } + CreateCommitStatus(ctx, jobs...) } }