From c30ddc3e1492ca44f31b81d35fb616709d1e4f3c Mon Sep 17 00:00:00 2001 From: bircni Date: Sat, 10 Oct 2026 14:55:28 +0200 Subject: [PATCH] refactor(actions): small cleanups across models, services and routers (#39722) Collection of small, behaviour-preserving cleanups in the Actions code. - Fix log and error prefixes that named non-existent or wrong functions, and wrap errors with `%w` so `errors.Is` keeps working. - Remove unused code. - Rely on `ctx.ServerError` / `ctx.APIErrorAuto`, which already map `util.ErrNotExist` and friends, instead of equivalent hand-written switches. - Load runs referenced by commit statuses with one query instead of one per run (used on every PR view); the result order is now stable. - Encode an event's payload once instead of up to three times. - Share the duplicated runner-token "get or create" logic and the "notify and emit" call pair. - Simplify `ToActionArtifact` / `ToActionRunner` signatures. --------- Signed-off-by: bircni Co-authored-by: delvh Co-authored-by: wxiaoguang --- models/actions/run.go | 4 +- models/actions/run_attempt.go | 2 +- models/actions/runner_token.go | 10 ++++ models/actions/schedule.go | 4 +- models/actions/status.go | 4 -- models/actions/utils.go | 19 ------- modules/actions/jobparser/jobparser.go | 8 ++- modules/actions/jobparser/model.go | 4 -- routers/api/actions/artifacts_chunks.go | 10 ++-- routers/api/v1/repo/action.go | 67 ++++--------------------- routers/api/v1/shared/runners.go | 11 ++-- routers/private/actions.go | 14 ++---- routers/web/repo/actions/actions.go | 2 +- routers/web/repo/actions/view.go | 50 +++++------------- routers/web/repo/pull.go | 2 +- routers/web/shared/actions/runners.go | 13 ++--- services/actions/clear_tasks.go | 12 ++--- services/actions/commit_status.go | 30 +++-------- services/actions/invalid_workflow.go | 5 +- services/actions/job_emitter.go | 5 +- services/actions/notifier.go | 10 ++-- services/actions/notifier_helper.go | 32 ++++++++---- services/actions/notify.go | 6 +++ services/actions/rerun.go | 3 +- services/actions/run.go | 3 +- services/convert/convert.go | 6 +-- 26 files changed, 113 insertions(+), 223 deletions(-) diff --git a/models/actions/run.go b/models/actions/run.go index e76ccccdb72..2a693af9bf6 100644 --- a/models/actions/run.go +++ b/models/actions/run.go @@ -296,7 +296,9 @@ func GetRunByRepoAndID(ctx context.Context, repoID, runID int64) (*ActionRun, er } func GetRunsByRepoAndID(ctx context.Context, repoID int64, runIDs []int64) (runs []*ActionRun, err error) { - err = db.GetEngine(ctx).In("id", runIDs).Where("repo_id=?", repoID).OrderBy("id").Find(&runs) + if len(runIDs) > 0 { + err = db.GetEngine(ctx).In("id", runIDs).Where("repo_id=?", repoID).OrderBy("id").Find(&runs) + } return runs, err } diff --git a/models/actions/run_attempt.go b/models/actions/run_attempt.go index 384ecf0ae7f..d2a4b026bee 100644 --- a/models/actions/run_attempt.go +++ b/models/actions/run_attempt.go @@ -179,7 +179,7 @@ func UpdateRunAttempt(ctx context.Context, attempt *ActionRunAttempt, cols ...st return err } if run.LatestAttemptID != attempt.ID { - log.Warn("run %d cannot be updated by an old attempt %d", run.LatestAttemptID, attempt.ID) + log.Warn("run %d cannot be updated by old attempt %d, latest attempt is %d", run.ID, attempt.ID, run.LatestAttemptID) return nil } diff --git a/models/actions/runner_token.go b/models/actions/runner_token.go index 0945ce3d489..3bb16e9d194 100644 --- a/models/actions/runner_token.go +++ b/models/actions/runner_token.go @@ -5,6 +5,7 @@ package actions import ( "context" + "errors" "fmt" "gitea.dev/models/db" @@ -119,3 +120,12 @@ func GetLatestRunnerToken(ctx context.Context, ownerID, repoID int64) (*ActionRu } return &runnerToken, nil } + +// GetOrCreateActiveRunnerToken returns the latest runner token, or a new one if there is none or it is no longer active +func GetOrCreateActiveRunnerToken(ctx context.Context, ownerID, repoID int64) (*ActionRunnerToken, error) { + token, err := GetLatestRunnerToken(ctx, ownerID, repoID) + if errors.Is(err, util.ErrNotExist) || (token != nil && !token.IsActive) { + return NewRunnerToken(ctx, ownerID, repoID) + } + return token, err +} diff --git a/models/actions/schedule.go b/models/actions/schedule.go index 79679203489..8ea30860ad3 100644 --- a/models/actions/schedule.go +++ b/models/actions/schedule.go @@ -104,7 +104,7 @@ func CleanRepoScheduleTasks(ctx context.Context, repo *repo_model.Repository) ([ // If actions disabled when there is schedule task, this will remove the outdated schedule tasks // There is no other place we can do this because the app.ini will be changed manually if err := DeleteScheduleTaskByRepo(ctx, repo.ID); err != nil { - return nil, fmt.Errorf("DeleteCronTaskByRepo: %v", err) + return nil, fmt.Errorf("CleanRepoScheduleTasks: %w", err) } // cancel running cron jobs of this repository and delete old schedules jobs, err := CancelPreviousJobs( @@ -115,7 +115,7 @@ func CleanRepoScheduleTasks(ctx context.Context, repo *repo_model.Repository) ([ webhook_module.HookEventSchedule, ) if err != nil { - return jobs, fmt.Errorf("CancelPreviousJobs: %v", err) + return jobs, fmt.Errorf("CleanRepoScheduleTasks: %w", err) } return jobs, nil } diff --git a/models/actions/status.go b/models/actions/status.go index 1a63aeb7b31..82494f27e36 100644 --- a/models/actions/status.go +++ b/models/actions/status.go @@ -59,10 +59,6 @@ func (s Status) HasRun() bool { return s.In(StatusSuccess, StatusFailure) } -func (s Status) IsUnknown() bool { - return s == StatusUnknown -} - func (s Status) IsSuccess() bool { return s == StatusSuccess } diff --git a/models/actions/utils.go b/models/actions/utils.go index 77c06a94227..f3f0e4445a7 100644 --- a/models/actions/utils.go +++ b/models/actions/utils.go @@ -89,22 +89,3 @@ func calculateDuration(started, stopped timeutil.TimeStamp, status Status, fallb } return max(timeSince(s), 0).Truncate(time.Second) } - -// best effort function to convert an action schedule to action run, to be used in GenerateGiteaContext -func (s *ActionSchedule) ToActionRun() *ActionRun { - return &ActionRun{ - Title: s.Title, - RepoID: s.RepoID, - Repo: s.Repo, - OwnerID: s.OwnerID, - WorkflowID: s.WorkflowID, - TriggerUserID: s.TriggerUserID, - TriggerUser: s.TriggerUser, - Ref: s.Ref, - CommitSHA: s.CommitSHA, - Event: s.Event, - EventPayload: s.EventPayload, - Created: s.Created, - Updated: s.Updated, - } -} diff --git a/modules/actions/jobparser/jobparser.go b/modules/actions/jobparser/jobparser.go index f37e4c92bb6..222290a5f00 100644 --- a/modules/actions/jobparser/jobparser.go +++ b/modules/actions/jobparser/jobparser.go @@ -95,7 +95,6 @@ func Parse(content []byte, options ...ParseOption) ([]*SingleWorkflow, error) { } results[id] = &JobResult{ Needs: job.Needs(), - Result: pc.jobResults[id], Outputs: nil, // resolved at expansion time, not at plan time } } @@ -129,7 +128,7 @@ func Parse(content []byte, options ...ParseOption) ([]*SingleWorkflow, error) { // Keep accepting empty exclude mappings for workflow compatibility, although GitHub rejects them. matrixes, err := (&model.Job{Strategy: job.Strategy.actStrategy()}).MatrixCombinations() if err != nil { - return nil, fmt.Errorf("getMatrixes: %w", err) + return nil, fmt.Errorf("Parse: %w", err) } if combos, err = buildMatrixCombos(id, job, matrixes, pc.gitContext, results, pc.vars, pc.inputs); err != nil { return nil, err @@ -138,7 +137,7 @@ func Parse(content []byte, options ...ParseOption) ([]*SingleWorkflow, error) { for _, combo := range combos { swf := workflow.CloneHeader() if err := swf.SetJob(id, combo); err != nil { - return nil, fmt.Errorf("SetJob: %w", err) + return nil, fmt.Errorf("Parse: %w", err) } ret = append(ret, swf) } @@ -170,7 +169,7 @@ func ExpandMatrixWithNeeds(jobID string, job *Job, gitCtx *model.GithubContext, } matrixes, err := (&model.Job{Strategy: job.Strategy.actStrategy()}).MatrixCombinations() if err != nil { - return nil, fmt.Errorf("getMatrixes: %w", err) + return nil, fmt.Errorf("ExpandMatrixWithNeeds: %w", err) } if len(matrixes) > maxCombinations { return nil, fmt.Errorf("matrix expands to %d combinations, exceeding the limit of %d", len(matrixes), maxCombinations) @@ -293,7 +292,6 @@ func WithInputs(inputs map[string]any) ParseOption { } type parseContext struct { - jobResults map[string]string gitContext *model.GithubContext vars map[string]string inputs map[string]any diff --git a/modules/actions/jobparser/model.go b/modules/actions/jobparser/model.go index 9fbb8599ee7..a84d25cbd6d 100644 --- a/modules/actions/jobparser/model.go +++ b/modules/actions/jobparser/model.go @@ -310,10 +310,6 @@ func (evt *Event) Acts() map[string][]string { return evt.acts } -func (evt *Event) Schedules() []map[string]string { - return evt.schedules -} - func ReadWorkflowRawConcurrency(content []byte) (*model.RawConcurrency, error) { w, err := ReadWorkflow(content) if err != nil { diff --git a/routers/api/actions/artifacts_chunks.go b/routers/api/actions/artifacts_chunks.go index 51098c86f42..73db6e3471b 100644 --- a/routers/api/actions/artifacts_chunks.go +++ b/routers/api/actions/artifacts_chunks.go @@ -96,7 +96,7 @@ func saveUploadChunkV3(st storage.ObjectStorage, ctx *ArtifactContext, artifact // save chunk to storage writtenSize, err := st.Save(storagePath, r, contentSize) if err != nil { - return 0, fmt.Errorf("save chunk to storage error: %v", err) + return 0, fmt.Errorf("save chunk to storage error: %w", err) } defer func() { @@ -129,7 +129,7 @@ func saveUploadChunkV3GetTotalSize(st storage.ObjectStorage, ctx *ArtifactContex contentRange := ctx.Req.Header.Get("Content-Range") var start, end int64 if _, err := fmt.Sscanf(contentRange, "bytes %d-%d/%d", &start, &end, &totalSize); err != nil { - return 0, fmt.Errorf("parse content range error: %v", err) + return 0, fmt.Errorf("parse content range error: %w", err) } _, err := saveUploadChunkV3(st, ctx, artifact, runID, saveUploadChunkOptions{start: start, end: &end, checkMd5: true}) if err != nil { @@ -289,7 +289,7 @@ func mergeChunksForRun(ctx *ArtifactContext, st storage.ObjectStorage, runID, ru art.StoragePath = storagePath art.Status = actions.ArtifactStatusUploadConfirmed if err := actions.UpdateArtifact(ctx, art, "storage_path", "status"); err != nil { - return fmt.Errorf("update artifact error: %v", err) + return fmt.Errorf("update artifact error: %w", err) } } return nil @@ -342,7 +342,7 @@ func mergeChunksForArtifact(chunks []*chunkFileItem, st storage.ObjectStorage, a var readCloser io.ReadCloser var err error if readCloser, err = st.Open(c.Path); err != nil { - return "", fmt.Errorf("open chunk error: %v, %s", err, c.Path) + return "", fmt.Errorf("open chunk error: %w, %s", err, c.Path) } readers = append(readers, readCloser) } @@ -362,7 +362,7 @@ func mergeChunksForArtifact(chunks []*chunkFileItem, st storage.ObjectStorage, a storagePath := generateArtifactStoragePath(artifact) written, err := st.Save(storagePath, mergedReader, artifact.FileCompressedSize) if err != nil { - return "", fmt.Errorf("save merged file error: %v", err) + return "", fmt.Errorf("save merged file error: %w", err) } if written != artifact.FileCompressedSize { return "", errors.New("merged file size is not equal to chunk length") diff --git a/routers/api/v1/repo/action.go b/routers/api/v1/repo/action.go index 94ca4096c00..b798b79b233 100644 --- a/routers/api/v1/repo/action.go +++ b/routers/api/v1/repo/action.go @@ -238,11 +238,7 @@ func (Action) GetVariable(ctx *context.APIContext) { Name: ctx.PathParam("variablename"), }) if err != nil { - if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -939,11 +935,7 @@ func ActionsGetWorkflow(ctx *context.APIContext) { workflowID := ctx.PathParam("workflow_id") workflow, err := convert.GetActionWorkflow(ctx, ctx.Repo.GitRepo, ctx.Repo.Repository, workflowID) if err != nil { - if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -1088,11 +1080,7 @@ func ActionsDisableWorkflow(ctx *context.APIContext) { workflowID := ctx.PathParam("workflow_id") err := actions_service.EnableOrDisableWorkflow(ctx, workflowID, false) if err != nil { - if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -1241,11 +1229,7 @@ func ActionsEnableWorkflow(ctx *context.APIContext) { workflowID := ctx.PathParam("workflow_id") err := actions_service.EnableOrDisableWorkflow(ctx, workflowID, true) if err != nil { - if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - } else { - ctx.APIErrorInternal(err) - } + ctx.APIErrorAuto(err) return } @@ -1434,7 +1418,7 @@ func RerunWorkflowRun(ctx *context.APIContext) { } if _, err := actions_service.RerunWorkflowRunJobs(ctx, ctx.Repo.Repository, run, ctx.Doer, jobs); err != nil { - handleWorkflowRerunError(ctx, err) + ctx.APIErrorAuto(err) return } @@ -1494,7 +1478,7 @@ func RerunFailedWorkflowRun(ctx *context.APIContext) { } if _, err := actions_service.RerunWorkflowRunJobs(ctx, ctx.Repo.Repository, run, ctx.Doer, failedJobs); err != nil { - handleWorkflowRerunError(ctx, err) + ctx.APIErrorAuto(err) return } @@ -1558,7 +1542,7 @@ func RerunWorkflowJob(ctx *context.APIContext) { targetJob := jobs[jobIdx] newAttempt, err := actions_service.RerunWorkflowRunJobs(ctx, ctx.Repo.Repository, run, ctx.Doer, []*actions_model.ActionRunJob{targetJob}) if err != nil { - handleWorkflowRerunError(ctx, err) + ctx.APIErrorAuto(err) return } @@ -1573,7 +1557,7 @@ func RerunWorkflowJob(ctx *context.APIContext) { } rerunJob, err := actions_model.GetRunJobByAttemptJobID(ctx, run.ID, newAttempt.ID, targetJob.AttemptJobID) if err != nil { - handleWorkflowRerunError(ctx, err) + ctx.APIErrorAuto(err) return } @@ -1585,20 +1569,6 @@ func RerunWorkflowJob(ctx *context.APIContext) { ctx.JSON(http.StatusCreated, convertedJob) } -func handleWorkflowRerunError(ctx *context.APIContext, err error) { - if errors.Is(err, util.ErrInvalidArgument) { - ctx.APIError(http.StatusBadRequest, err.Error()) - return - } else if errors.Is(err, util.ErrAlreadyExist) { - ctx.APIError(http.StatusConflict, err.Error()) - return - } else if errors.Is(err, util.ErrNotExist) { - ctx.APIError(http.StatusNotFound, err.Error()) - return - } - ctx.APIErrorInternal(err) -} - // ListWorkflowRunJobs Lists all jobs for a workflow run. func ListWorkflowRunJobs(ctx *context.APIContext) { // swagger:operation GET /repos/{owner}/{repo}/actions/runs/{run}/jobs repository listWorkflowRunJobs @@ -1835,12 +1805,7 @@ func GetArtifactsOfRun(ctx *context.APIContext) { res.Entries = make([]*api.ActionArtifact, len(artifacts)) for i := range artifacts { - convertedArtifact, err := convert.ToActionArtifact(ctx.Repo.Repository, artifacts[i]) - if err != nil { - ctx.APIErrorInternal(err) - return - } - res.Entries[i] = convertedArtifact + res.Entries[i] = convert.ToActionArtifact(ctx.Repo.Repository, artifacts[i]) } ctx.JSON(http.StatusOK, &res) @@ -1944,12 +1909,7 @@ func GetArtifacts(ctx *context.APIContext) { res.Entries = make([]*api.ActionArtifact, len(artifacts)) for i := range artifacts { - convertedArtifact, err := convert.ToActionArtifact(ctx.Repo.Repository, artifacts[i]) - if err != nil { - ctx.APIErrorInternal(err) - return - } - res.Entries[i] = convertedArtifact + res.Entries[i] = convert.ToActionArtifact(ctx.Repo.Repository, artifacts[i]) } ctx.JSON(http.StatusOK, &res) @@ -1992,12 +1952,7 @@ func GetArtifact(ctx *context.APIContext) { } if actions_service.IsArtifactV4(art) { - convertedArtifact, err := convert.ToActionArtifact(ctx.Repo.Repository, art) - if err != nil { - ctx.APIErrorInternal(err) - return - } - ctx.JSON(http.StatusOK, convertedArtifact) + ctx.JSON(http.StatusOK, convert.ToActionArtifact(ctx.Repo.Repository, art)) return } // v3 not supported due to not having one unique id diff --git a/routers/api/v1/shared/runners.go b/routers/api/v1/shared/runners.go index ed4341033c3..55f82a367f9 100644 --- a/routers/api/v1/shared/runners.go +++ b/routers/api/v1/shared/runners.go @@ -4,14 +4,12 @@ package shared import ( - "errors" "net/http" actions_model "gitea.dev/models/actions" "gitea.dev/models/db" "gitea.dev/modules/setting" api "gitea.dev/modules/structs" - "gitea.dev/modules/util" "gitea.dev/modules/web" "gitea.dev/routers/api/v1/utils" "gitea.dev/services/context" @@ -25,10 +23,7 @@ type RegistrationToken struct { } func GetRegistrationToken(ctx *context.APIContext, ownerID, repoID int64) { - token, err := actions_model.GetLatestRunnerToken(ctx, ownerID, repoID) - if errors.Is(err, util.ErrNotExist) || (token != nil && !token.IsActive) { - token, err = actions_model.NewRunnerToken(ctx, ownerID, repoID) - } + token, err := actions_model.GetOrCreateActiveRunnerToken(ctx, ownerID, repoID) if err != nil { ctx.APIErrorInternal(err) return @@ -64,7 +59,7 @@ func ListRunners(ctx *context.APIContext, ownerID, repoID int64) { res.Entries = make([]*api.ActionRunner, len(runners)) for i, runner := range runners { - res.Entries[i] = convert.ToActionRunner(ctx, runner) + res.Entries[i] = convert.ToActionRunner(runner) } ctx.JSON(http.StatusOK, &res) @@ -102,7 +97,7 @@ func GetRunner(ctx *context.APIContext, ownerID, repoID, runnerID int64) { if !ok { return } - ctx.JSON(http.StatusOK, convert.ToActionRunner(ctx, runner)) + ctx.JSON(http.StatusOK, convert.ToActionRunner(runner)) } // DeleteRunner deletes the runner for api route validated ownerID and repoID diff --git a/routers/private/actions.go b/routers/private/actions.go index fa4c200fa60..ab08c53721e 100644 --- a/routers/private/actions.go +++ b/routers/private/actions.go @@ -4,7 +4,6 @@ package private import ( - "errors" "net/http" "strings" @@ -13,7 +12,6 @@ import ( user_model "gitea.dev/models/user" "gitea.dev/modules/json" "gitea.dev/modules/private" - "gitea.dev/modules/util" "gitea.dev/services/context" ) @@ -34,15 +32,9 @@ func GenerateActionsRunnerToken(ctx *context.PrivateContext) { return } - token, err := actions_model.GetLatestRunnerToken(ctx, owner, repo) - if errors.Is(err, util.ErrNotExist) || (token != nil && !token.IsActive) { - token, err = actions_model.NewRunnerToken(ctx, owner, repo) - if err != nil { - ctx.PrivateInternalErrorf("error while creating runner token: %v", err) - return - } - } else if err != nil { - ctx.PrivateInternalErrorf("could not get unactivated runner token: %v", err) + token, err := actions_model.GetOrCreateActiveRunnerToken(ctx, owner, repo) + if err != nil { + ctx.PrivateInternalErrorf("GetOrCreateActiveRunnerToken failed: %v", err) return } diff --git a/routers/web/repo/actions/actions.go b/routers/web/repo/actions/actions.go index d75ee062e13..0fb986d0bc2 100644 --- a/routers/web/repo/actions/actions.go +++ b/routers/web/repo/actions/actions.go @@ -664,7 +664,7 @@ func (data *actionRunListData) prepareCommon(ctx *context.Context, workflows []W return false } if err := loadIsRefDeleted(ctx, ctx.Repo.Repository.ID, data.ActionRuns); err != nil { - log.Error("LoadIsRefDeleted", err) + log.Error("loadIsRefDeleted: %v", err) } if !data.processActionRuns(ctx) { diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index 03bbdf6a9d8..6f3667ee8b7 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -252,9 +252,7 @@ func ViewWorkflowFile(ctx *context_module.Context) { commit, err := ctx.Repo.GitRepo.GetCommit(ctx, run.CommitSHA) if err != nil { - ctx.NotFoundOrServerError("GetCommit", func(err error) bool { - return errors.Is(err, util.ErrNotExist) - }, err) + ctx.ServerError("GetCommit", err) return } rpath, entries, err := actions.ListWorkflows(ctx, ctx.Repo.GitRepo, commit) @@ -1037,9 +1035,7 @@ func Logs(ctx *context_module.Context) { jobID := ctx.PathParamInt64("job") if err := common.DownloadActionsRunJobLogsWithID(ctx.Base, ctx.Repo.Repository, run.ID, jobID); err != nil { - ctx.NotFoundOrServerError("DownloadActionsRunJobLogsWithID", func(err error) bool { - return errors.Is(err, util.ErrNotExist) - }, err) + ctx.ServerError("DownloadActionsRunJobLogsWithID", err) } } @@ -1065,9 +1061,7 @@ func Approve(ctx *context_module.Context) { return } if _, err := actions_service.ApproveRuns(ctx, ctx.Repo.Repository, ctx.Doer, []int64{run.ID}); err != nil { - ctx.NotFoundOrServerError("ApproveRuns", func(err error) bool { - return errors.Is(err, util.ErrNotExist) - }, err) + ctx.ServerError("ApproveRuns", err) return } @@ -1115,9 +1109,7 @@ func getCurrentRunJobsByPathParam(ctx *context_module.Context) (*actions_model.A jobID := ctx.PathParamInt64("job") selectedJob, err = actions_model.GetRunJobByRunAndID(ctx, run.ID, jobID) if err != nil { - ctx.NotFoundOrServerError("GetRunJobByRepoAndID", func(err error) bool { - return errors.Is(err, util.ErrNotExist) - }, err) + ctx.ServerError("GetRunJobByRunAndID", err) return nil, nil, nil } } @@ -1132,27 +1124,21 @@ func getCurrentRunJobsByPathParam(ctx *context_module.Context) (*actions_model.A // Explicit attempt number in the URL — user is viewing a historical attempt. attempt, err = actions_model.GetRunAttemptByRunIDAndAttemptNum(ctx, run.ID, attemptNum) if err != nil { - ctx.NotFoundOrServerError("GetRunAttemptByRunIDAndAttempt", func(err error) bool { - return errors.Is(err, util.ErrNotExist) - }, err) + ctx.ServerError("GetRunAttemptByRunIDAndAttemptNum", err) return nil, nil, nil } case selectedJob != nil && selectedJob.RunAttemptID > 0: // No explicit attempt in the URL, but the requested job belongs to a known attempt — resolve via the job. attempt, err = actions_model.GetRunAttemptByRepoAndID(ctx, selectedJob.RepoID, selectedJob.RunAttemptID) if err != nil { - ctx.NotFoundOrServerError("GetRunAttemptByRepoAndID", func(err error) bool { - return errors.Is(err, util.ErrNotExist) - }, err) + ctx.ServerError("GetRunAttemptByRepoAndID", err) return nil, nil, nil } default: // No attempt context at all — show the latest attempt (nil for legacy runs). attempt, _, err = run.GetLatestAttempt(ctx) if err != nil { - ctx.NotFoundOrServerError("GetLatestAttempt", func(err error) bool { - return errors.Is(err, util.ErrNotExist) - }, err) + ctx.ServerError("GetLatestAttempt", err) return nil, nil, nil } } @@ -1201,9 +1187,7 @@ func ArtifactsDeleteView(ctx *context_module.Context) { } resolvedAttemptID, err := resolveArtifactAttemptIDFromQuery(ctx, run) if err != nil { - ctx.NotFoundOrServerError("resolveArtifactAttemptIDFromQuery", func(err error) bool { - return errors.Is(err, util.ErrNotExist) - }, err) + ctx.ServerError("resolveArtifactAttemptIDFromQuery", err) return } artifactName := ctx.PathParam("artifact_name") @@ -1221,9 +1205,7 @@ func ArtifactsDownloadView(ctx *context_module.Context) { } resolvedAttemptID, err := resolveArtifactAttemptIDFromQuery(ctx, run) if err != nil { - ctx.NotFoundOrServerError("resolveArtifactAttemptIDFromQuery", func(err error) bool { - return errors.Is(err, util.ErrNotExist) - }, err) + ctx.ServerError("resolveArtifactAttemptIDFromQuery", err) return } artifactName := ctx.PathParam("artifact_name") @@ -1308,7 +1290,7 @@ func ApproveAllChecks(ctx *context_module.Context) { ctx.ServerError("GetLatestCommitStatus", err) return } - runs, err := actions_service.GetRunsFromCommitStatuses(ctx, commitStatuses) + runs, err := actions_service.GetRunsFromCommitStatuses(ctx, repo.ID, commitStatuses) if err != nil { ctx.ServerError("GetRunsFromCommitStatuses", err) return @@ -1327,9 +1309,7 @@ func ApproveAllChecks(ctx *context_module.Context) { } if _, err := actions_service.ApproveRuns(ctx, repo, ctx.Doer, runIDs); err != nil { - ctx.NotFoundOrServerError("ApproveRuns", func(err error) bool { - return errors.Is(err, util.ErrNotExist) - }, err) + ctx.ServerError("ApproveRuns", err) return } @@ -1442,9 +1422,7 @@ func Run(ctx *context_module.Context) { func viewScopedWorkflowFile(ctx *context_module.Context, run *actions_model.ActionRun) { sourceRepo, err := repo_model.GetRepositoryByID(ctx, run.WorkflowRepoID) if err != nil { - ctx.NotFoundOrServerError("GetRepositoryByID", func(err error) bool { - return errors.Is(err, util.ErrNotExist) - }, err) + ctx.ServerError("GetRepositoryByID", err) return } @@ -1467,9 +1445,7 @@ func viewScopedWorkflowFile(ctx *context_module.Context, run *actions_model.Acti commit, err := sourceGitRepo.GetCommit(ctx, run.WorkflowCommitSHA) if err != nil { - ctx.NotFoundOrServerError("GetCommit", func(err error) bool { - return errors.Is(err, util.ErrNotExist) - }, err) + ctx.ServerError("GetCommit", err) return } rpath, entries, err := actions.ListScopedWorkflows(ctx, sourceGitRepo, commit) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 53fd535ea6c..469e09169ab 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -438,7 +438,7 @@ func (prInfo *pullRequestViewInfo) prepareMergeBoxStatusCheckData(ctx *context.C // so the status-check section must render when there are any required contexts, not only when enableStatusCheck is on. data.ShowStatusCheck = data.enableStatusCheck || data.hasRequiredStatusContexts || len(statusCheckData.PullCommitStatuses) > 0 - runs, err := actions_service.GetRunsFromCommitStatuses(ctx, commitStatuses) + runs, err := actions_service.GetRunsFromCommitStatuses(ctx, ctx.Repo.Repository.ID, commitStatuses) if err != nil { log.Error("GetRunsFromCommitStatuses: %v", err) } diff --git a/routers/web/shared/actions/runners.go b/routers/web/shared/actions/runners.go index 71c693589df..d199ffc43b4 100644 --- a/routers/web/shared/actions/runners.go +++ b/routers/web/shared/actions/runners.go @@ -139,16 +139,9 @@ func Runners(ctx *context.Context) { } // ownid=0,repo_id=0,means this token is used for global - var token *actions_model.ActionRunnerToken - token, err = actions_model.GetLatestRunnerToken(ctx, opts.OwnerID, opts.RepoID) - if errors.Is(err, util.ErrNotExist) || (token != nil && !token.IsActive) { - token, err = actions_model.NewRunnerToken(ctx, opts.OwnerID, opts.RepoID) - if err != nil { - ctx.ServerError("CreateRunnerToken", err) - return - } - } else if err != nil { - ctx.ServerError("GetLatestRunnerToken", err) + token, err := actions_model.GetOrCreateActiveRunnerToken(ctx, opts.OwnerID, opts.RepoID) + if err != nil { + ctx.ServerError("GetOrCreateActiveRunnerToken", err) return } diff --git a/services/actions/clear_tasks.go b/services/actions/clear_tasks.go index a2fe4bc1b76..f60eebb30a6 100644 --- a/services/actions/clear_tasks.go +++ b/services/actions/clear_tasks.go @@ -50,15 +50,13 @@ func stopTasksByStatuses(ctx context.Context, opts actions_model.FindTaskOptions func CancelPreviousJobs(ctx context.Context, repoID int64, ref, workflowID string, event webhook_module.HookEventType) error { jobs, err := actions_model.CancelPreviousJobs(ctx, repoID, ref, workflowID, event) - NotifyWorkflowJobsAndRunsStatusUpdate(ctx, jobs) - EmitJobsIfReadyByJobs(jobs) + notifyAndEmitFinishedJobs(ctx, jobs) return err } func CleanRepoScheduleTasks(ctx context.Context, repo *repo_model.Repository) error { jobs, err := actions_model.CleanRepoScheduleTasks(ctx, repo) - NotifyWorkflowJobsAndRunsStatusUpdate(ctx, jobs) - EmitJobsIfReadyByJobs(jobs) + notifyAndEmitFinishedJobs(ctx, jobs) return err } @@ -171,8 +169,7 @@ func stopTasks(ctx context.Context, opts actions_model.FindTaskOptions) error { remove() } - NotifyWorkflowJobsAndRunsStatusUpdate(ctx, jobs) - EmitJobsIfReadyByJobs(jobs) + notifyAndEmitFinishedJobs(ctx, jobs) return nil } @@ -193,8 +190,7 @@ func CancelAbandonedJobs(ctx context.Context) error { log.Warn("cancel abandoned jobs: %v", err) } - NotifyWorkflowJobsAndRunsStatusUpdate(ctx, updatedJobs) - EmitJobsIfReadyByJobs(updatedJobs) + notifyAndEmitFinishedJobs(ctx, updatedJobs) return nil } diff --git a/services/actions/commit_status.go b/services/actions/commit_status.go index 8eddeb03b5a..f174adc3142 100644 --- a/services/actions/commit_status.go +++ b/services/actions/commit_status.go @@ -17,10 +17,10 @@ import ( actions_module "gitea.dev/modules/actions" "gitea.dev/modules/actions/jobparser" "gitea.dev/modules/commitstatus" + "gitea.dev/modules/container" "gitea.dev/modules/glob" "gitea.dev/modules/log" api "gitea.dev/modules/structs" - "gitea.dev/modules/util" webhook_module "gitea.dev/modules/webhook" pull_service "gitea.dev/services/pull" commitstatus_service "gitea.dev/services/repository/commitstatus" @@ -36,7 +36,7 @@ func CreateCommitStatusForRunJobs(ctx context.Context, run *actions_model.Action event, commitID, err := getCommitStatusEventNameAndCommitID(run) if err != nil { - log.Error("GetCommitStatusEventNameAndSHA: %v", err) + log.Error("getCommitStatusEventNameAndCommitID: %v", err) } if event == "" || commitID == "" { return // unsupported event, or no commit id, or error occurs, do nothing @@ -71,31 +71,15 @@ func CreateCommitStatusForRunJobs(ctx context.Context, run *actions_model.Action } } -func GetRunsFromCommitStatuses(ctx context.Context, statuses []*git_model.CommitStatus) ([]*actions_model.ActionRun, error) { - runMap := make(map[int64]*actions_model.ActionRun) +func GetRunsFromCommitStatuses(ctx context.Context, repoID int64, statuses []*git_model.CommitStatus) (runs []*actions_model.ActionRun, _ error) { + runIDs := container.Set[int64]{} for _, status := range statuses { runID, _, ok := status.ParseGiteaActionsTargetURL(ctx) - if !ok { - continue - } - _, ok = runMap[runID] - if !ok { - run, err := actions_model.GetRunByRepoAndID(ctx, status.RepoID, runID) - if err != nil { - if errors.Is(err, util.ErrNotExist) { - // the run may be deleted manually, just skip it - continue - } - return nil, fmt.Errorf("GetRunByRepoAndID: %w", err) - } - runMap[runID] = run + if ok { + runIDs.Add(runID) } } - runs := make([]*actions_model.ActionRun, 0, len(runMap)) - for _, run := range runMap { - runs = append(runs, run) - } - return runs, nil + return actions_model.GetRunsByRepoAndID(ctx, repoID, runIDs.Values()) } func getCommitStatusEventNameAndCommitID(run *actions_model.ActionRun) (event, commitID string, _ error) { diff --git a/services/actions/invalid_workflow.go b/services/actions/invalid_workflow.go index 56edb0a9776..74056797c88 100644 --- a/services/actions/invalid_workflow.go +++ b/services/actions/invalid_workflow.go @@ -12,7 +12,6 @@ import ( "gitea.dev/models/unit" "gitea.dev/modules/commitstatus" "gitea.dev/modules/git" - "gitea.dev/modules/json" "gitea.dev/modules/log" "gitea.dev/modules/timeutil" "gitea.dev/modules/util" @@ -22,7 +21,7 @@ func handleInvalidWorkflows(ctx context.Context, input *notifyInput, ref git.Ref if len(invalid) == 0 { return } - payload, err := json.Marshal(input.Payload) + payload, err := input.EncodedPayload() if err != nil { log.Error("marshal event payload: %v", err) return @@ -35,7 +34,7 @@ func handleInvalidWorkflows(ctx context.Context, input *notifyInput, ref git.Ref insertInvalidWorkflowRun(ctx, &actions_model.ActionRun{ Title: commit.MessageTitle(), RepoID: input.Repo.ID, Repo: input.Repo, OwnerID: input.Repo.OwnerID, WorkflowID: entryName, TriggerUserID: input.Doer.ID, TriggerUser: input.Doer, Ref: ref.String(), - CommitSHA: commit.ID.String(), Event: input.Event, TriggerEvent: string(input.Event), EventPayload: string(payload), + CommitSHA: commit.ID.String(), Event: input.Event, TriggerEvent: string(input.Event), EventPayload: payload, WorkflowRepoID: input.Repo.ID, WorkflowCommitSHA: commit.ID.String(), }, parseErr) } diff --git a/services/actions/job_emitter.go b/services/actions/job_emitter.go index 804fbe0d0e9..f92625e31c8 100644 --- a/services/actions/job_emitter.go +++ b/services/actions/job_emitter.go @@ -103,8 +103,7 @@ func checkJobsByRunID(ctx context.Context, runID int64) error { log.Error("re-emit run %d: %v", rid, err) } } - NotifyWorkflowJobsAndRunsStatusUpdate(ctx, result.CancelledJobs) - EmitJobsIfReadyByJobs(result.CancelledJobs) + notifyAndEmitFinishedJobs(ctx, result.CancelledJobs) if err := createCommitStatusesForJobsByRun(ctx, result.Jobs); err != nil { return err } @@ -651,7 +650,7 @@ func (r *jobStatusResolver) resolve(ctx context.Context) (map[int64]actions_mode newStatus, cancelledJobs, err := PrepareToStartJobWithConcurrency(ctx, actionRunJob) if err != nil { - log.Error("ShouldBlockJobByConcurrency failed, this job will stay blocked: job: %d, err: %v", id, err) + log.Error("PrepareToStartJobWithConcurrency failed, this job will stay blocked: job: %d, err: %v", id, err) } else { r.cancelledJobs = append(r.cancelledJobs, cancelledJobs...) for _, cancelled := range cancelledJobs { diff --git a/services/actions/notifier.go b/services/actions/notifier.go index a9f8f9a4200..10ecc12bc7c 100644 --- a/services/actions/notifier.go +++ b/services/actions/notifier.go @@ -56,7 +56,7 @@ func (n *actionsNotifier) NewIssue(ctx context.Context, issue *issues_model.Issu Issue: convert.ToAPIIssue(ctx, issue.Poster, issue), Repository: convert.ToRepo(ctx, issue.Repo, permission), Sender: convert.ToUser(ctx, issue.Poster, nil), - }).Notify(withMethod(ctx, "NewIssue")) + }).Notify(ctx) } // IssueChangeContent notifies change content of issue @@ -455,7 +455,7 @@ func (n *actionsNotifier) PullRequestReview(ctx context.Context, pr *issues_mode permission, err := access_model.GetIndividualUserRepoPermission(ctx, review.Issue.Repo, review.Issue.Poster) if err != nil { - log.Error("models.GetIndividualUserRepoPermission: %v", err) + log.Error("GetIndividualUserRepoPermission: %v", err) return } @@ -516,7 +516,7 @@ func (*actionsNotifier) MergePullRequest(ctx context.Context, doer *user_model.U } if err := pr.LoadIssue(ctx); err != nil { - log.Error("LoadAttributes: %v", err) + log.Error("pr.LoadIssue: %v", err) return } @@ -689,7 +689,7 @@ func (n *actionsNotifier) PullRequestSynchronized(ctx context.Context, doer *use ctx = withMethod(ctx, "PullRequestSynchronized") if err := pr.LoadIssue(ctx); err != nil { - log.Error("LoadAttributes: %v", err) + log.Error("pr.LoadIssue: %v", err) return } @@ -716,7 +716,7 @@ func (n *actionsNotifier) PullRequestChangeTargetBranch(ctx context.Context, doe ctx = withMethod(ctx, "PullRequestChangeTargetBranch") if err := pr.LoadIssue(ctx); err != nil { - log.Error("LoadAttributes: %v", err) + log.Error("pr.LoadIssue: %v", err) return } diff --git a/services/actions/notifier_helper.go b/services/actions/notifier_helper.go index e6223c0a6a5..bdb93fb20e9 100644 --- a/services/actions/notifier_helper.go +++ b/services/actions/notifier_helper.go @@ -67,6 +67,8 @@ type notifyInput struct { Ref git.RefName Payload api.Payloader PullRequest *issues_model.PullRequest + + encodedPayload string // cached by EncodedPayload } func newNotifyInput(repo *repo_model.Repository, doer *user_model.User, event webhook_module.HookEventType) *notifyInput { @@ -111,6 +113,18 @@ func (input *notifyInput) WithPullRequest(pr *issues_model.PullRequest) *notifyI return input } +// EncodedPayload marshals Payload once, as one event can start repo, scheduled and scoped workflows +func (input *notifyInput) EncodedPayload() (string, error) { + if input.encodedPayload == "" { + p, err := json.Marshal(input.Payload) + if err != nil { + return "", fmt.Errorf("EncodedPayload: %w", err) + } + input.encodedPayload = string(p) + } + return input.encodedPayload, nil +} + func (input *notifyInput) Notify(ctx context.Context) { log.Trace("execute %v for event %v whose doer is %v", getMethod(ctx), input.Event, input.Doer.Name) @@ -337,16 +351,16 @@ func handleWorkflows( return nil } - p, err := json.Marshal(input.Payload) + payload, err := input.EncodedPayload() if err != nil { - return fmt.Errorf("json.Marshal: %w", err) + return err } isForkPullRequest := isForkPullRequestInput(input) for _, dwf := range detectedWorkflows { // repo-level run: the workflow content is this repo at dwf.SourceCommitSHA - if err := buildApproveAndInsertRun(ctx, input, ref, commit, string(p), isForkPullRequest, dwf, input.Repo.ID, false); err != nil { + if err := buildApproveAndInsertRun(ctx, input, ref, commit, payload, isForkPullRequest, dwf, input.Repo.ID, false); err != nil { log.Error("repo %s: %v", input.Repo.FullName(), err) continue } @@ -595,9 +609,9 @@ func handleSchedules( return nil } - p, err := json.Marshal(input.Payload) + payload, err := input.EncodedPayload() if err != nil { - return fmt.Errorf("json.Marshal: %w", err) + return err } crons := make([]*actions_model.ActionSchedule, 0, len(detectedWorkflows)) @@ -625,7 +639,7 @@ func handleSchedules( Ref: ref.String(), CommitSHA: commit.ID.String(), Event: input.Event, - EventPayload: string(p), + EventPayload: payload, Specs: schedules, Content: dwf.Content, } @@ -709,9 +723,9 @@ func detectAndHandleScopedWorkflows( return nil } - p, err := json.Marshal(input.Payload) + payload, err := input.EncodedPayload() if err != nil { - return fmt.Errorf("json.Marshal: %w", err) + return err } isForkPullRequest := isForkPullRequestInput(input) actionsConfig := input.Repo.MustGetUnit(ctx, unit_model.TypeActions).ActionsConfig() @@ -759,7 +773,7 @@ func detectAndHandleScopedWorkflows( continue } - if err := buildApproveAndInsertRun(ctx, input, ref, consumerCommit, string(p), isForkPullRequest, dwf, sourceRepo.ID, true); err != nil { + if err := buildApproveAndInsertRun(ctx, input, ref, consumerCommit, payload, isForkPullRequest, dwf, sourceRepo.ID, true); err != nil { log.Error("scoped workflows: source %s workflow %s: %v", sourceRepo.FullName(), dwf.EntryName, err) continue } diff --git a/services/actions/notify.go b/services/actions/notify.go index 040ddec98eb..549c5b25a70 100644 --- a/services/actions/notify.go +++ b/services/actions/notify.go @@ -46,6 +46,12 @@ func NotifyWorkflowJobsAndRunsStatusUpdate(ctx context.Context, jobs []*actions_ } } +// notifyAndEmitFinishedJobs notifies status changes of jobs that may have finished and emits their runs' next jobs +func notifyAndEmitFinishedJobs(ctx context.Context, jobs []*actions_model.ActionRunJob) { + NotifyWorkflowJobsAndRunsStatusUpdate(ctx, jobs) + EmitJobsIfReadyByJobs(jobs) +} + // NotifyWorkflowRunStatusUpdateWithReload reloads the run before notifying its status update. // Use it when only repo/run IDs are available or when the in-memory run may be stale after job updates. func NotifyWorkflowRunStatusUpdateWithReload(ctx context.Context, repoID, runID int64) { diff --git a/services/actions/rerun.go b/services/actions/rerun.go index 59d63f83dff..2263b94df39 100644 --- a/services/actions/rerun.go +++ b/services/actions/rerun.go @@ -385,8 +385,7 @@ func execRerunPlan(ctx context.Context, plan *rerunPlan) (*actions_model.ActionR return nil, err } - NotifyWorkflowJobsAndRunsStatusUpdate(ctx, cancelledConcurrencyJobs) - EmitJobsIfReadyByJobs(cancelledConcurrencyJobs) + notifyAndEmitFinishedJobs(ctx, cancelledConcurrencyJobs) CreateCommitStatusForRunJobs(ctx, plan.run, newJobs...) NotifyWorkflowJobsAndRunsStatusUpdate(ctx, newJobsToRerun) diff --git a/services/actions/run.go b/services/actions/run.go index 4b4011fccec..c2565902ce9 100644 --- a/services/actions/run.go +++ b/services/actions/run.go @@ -169,8 +169,7 @@ func InsertRun(ctx context.Context, run *actions_model.ActionRun, content []byte return err } - NotifyWorkflowJobsAndRunsStatusUpdate(ctx, cancelledConcurrencyJobs) - EmitJobsIfReadyByJobs(cancelledConcurrencyJobs) + notifyAndEmitFinishedJobs(ctx, cancelledConcurrencyJobs) // Post-commit kick: let the job emitter resolve jobs if needed if needPostCommitEmit { diff --git a/services/convert/convert.go b/services/convert/convert.go index a89d6a89f3b..e1b848a5f3e 100644 --- a/services/convert/convert.go +++ b/services/convert/convert.go @@ -720,7 +720,7 @@ func ResolveActionWorkflowForRun(ctx context.Context, repo *repo_model.Repositor } // ToActionArtifact convert a actions_model.ActionArtifact to an api.ActionArtifact -func ToActionArtifact(repo *repo_model.Repository, art *actions_model.ActionArtifact) (*api.ActionArtifact, error) { +func ToActionArtifact(repo *repo_model.Repository, art *actions_model.ActionArtifact) *api.ActionArtifact { url := fmt.Sprintf("%s/actions/artifacts/%d", repo.APIURL(), art.ID) return &api.ActionArtifact{ @@ -738,10 +738,10 @@ func ToActionArtifact(repo *repo_model.Repository, art *actions_model.ActionArti RepositoryID: art.RepoID, HeadSha: art.CommitSHA, }, - }, nil + } } -func ToActionRunner(ctx context.Context, runner *actions_model.ActionRunner) *api.ActionRunner { +func ToActionRunner(runner *actions_model.ActionRunner) *api.ActionRunner { status := runner.Status() apiStatus := "offline" if runner.IsOnline() {