Refactor commit status for Actions jobs (#23786)

Before: <img width="353" alt="xnip_230329_163852" src="https://user-images.githubusercontent.com/9418365/228479807-424452df-10fa-45cf-ae4b-09939c0ed54c.png"> After: <img width="508" alt="xnip_230329_163358" src="https://user-images.githubusercontent.com/9418365/228479923-537b54fe-9564-4105-a068-bcc75fa2a7ea.png"> 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 `<run_name> / <job_name> / (<event>)` 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.
2023-03-29 23:27:37 +08:00 · 2023-03-29 23:27:37 +08:00 · 3e8db31a5b
commit 3e8db31a5b
parent e57e1144c5
7 changed files with 76 additions and 71 deletions
--- 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 {
--- 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,
--- 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{}{})
 }

--- 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
--- 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
--- 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 {
--- 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...)
 		}

 	}