From c1ac671b555663c333528433f1c5e943ce9621a3 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Tue, 5 May 2026 02:59:34 +0200 Subject: [PATCH] feat: reusable workflow outer job is skipped if 'if:' block skips workflow (#12412) Follow-up to https://code.forgejo.org/forgejo/runner/pulls/1509 -- improves the UX in Forgejo when a reusable workflow is skipped, marking the workflow as skipped rather than succeeded. ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests for Go changes - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I ran... - [x] `make pr-go` before pushing ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [x] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change. - [ ] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12412 Reviewed-by: Andreas Ahlenstorf --- modules/actions/task_state.go | 2 ++ modules/actions/task_state_test.go | 15 +++++++++++++++ services/actions/job_emitter.go | 19 ++++++++++++++----- services/actions/job_emitter_test.go | 21 +++++++++++++++++++++ 4 files changed, 52 insertions(+), 5 deletions(-) diff --git a/modules/actions/task_state.go b/modules/actions/task_state.go index 77bfc747ee..889a0b0364 100644 --- a/modules/actions/task_state.go +++ b/modules/actions/task_state.go @@ -111,6 +111,8 @@ func fullStepsOfEmptySteps(task *actions_model.ActionTask) []*actions_model.Acti preStep.Status = task.Status if preStep.Status.IsSuccess() { postStep.Status = actions_model.StatusSuccess + } else if preStep.Status.IsSkipped() { + postStep.Status = actions_model.StatusSkipped } else { postStep.Status = actions_model.StatusCancelled } diff --git a/modules/actions/task_state_test.go b/modules/actions/task_state_test.go index e18de4573f..084de1c709 100644 --- a/modules/actions/task_state_test.go +++ b/modules/actions/task_state_test.go @@ -156,6 +156,21 @@ func TestFullSteps(t *testing.T) { {Name: postStepName, Status: actions_model.StatusSuccess, LogIndex: 90, LogLength: 10, Started: 10090, Stopped: 10100}, }, }, + { + // situation occurs with a reusable workflow's outer job which has no steps + name: "skipped task w/ zero steps", + task: &actions_model.ActionTask{ + Steps: []*actions_model.ActionTaskStep{}, + Status: actions_model.StatusSkipped, + Started: 0, + Stopped: 0, + LogLength: 0, + }, + want: []*actions_model.ActionTaskStep{ + {Name: preStepName, Status: actions_model.StatusSkipped, LogIndex: 0, LogLength: 0, Started: 0, Stopped: 0}, + {Name: postStepName, Status: actions_model.StatusSkipped, LogIndex: 0, LogLength: 0, Started: 0, Stopped: 0}, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/services/actions/job_emitter.go b/services/actions/job_emitter.go index 0a9c297033..faaf9d2cdf 100644 --- a/services/actions/job_emitter.go +++ b/services/actions/job_emitter.go @@ -77,7 +77,7 @@ func checkJobsOfRun(ctx context.Context, runID int64, recursionCount int) error job.Status = status updateColumns := []string{"status"} - if status == actions_model.StatusWaiting { + if status.IsWaiting() { behaviour, err := tryHandleIncompleteMatrix(ctx, job, jobs) switch behaviour { case behaviourError: @@ -95,7 +95,7 @@ func checkJobsOfRun(ctx context.Context, runID int64, recursionCount int) error // Stop processing any other jobs in this run. return nil } - } else if status == actions_model.StatusSuccess || status == actions_model.StatusFailure { + } else if status.IsSuccess() || status.IsFailure() || status.IsSkipped() { // Transition to these states can be triggered by workflow call outer jobs additionalColumns, err := tryHandleWorkflowCallOuterJob(ctx, job) if err != nil { @@ -192,7 +192,7 @@ func (r *jobStatusResolver) resolve() map[int64]actions_model.Status { if status != actions_model.StatusBlocked { continue } - allDone, allSucceed, allSucceedOrSkip := true, true, true + allDone, allSucceed, allSucceedOrSkip, allSkip := true, true, true, true for _, need := range r.needs[id] { needStatus := r.statuses[need] if !needStatus.IsDone() { @@ -204,12 +204,15 @@ func (r *jobStatusResolver) resolve() map[int64]actions_model.Status { if needStatus.In(actions_model.StatusFailure, actions_model.StatusCancelled) { allSucceedOrSkip = false } + if !needStatus.IsSkipped() { + allSkip = false + } } if allDone { if isWorkflowCallOuterJob, _ := r.jobMap[id].IsWorkflowCallOuterJob(); isWorkflowCallOuterJob { // If the dependent job was a workflow call outer job, then options aren't waiting/skipped, but rather - // success/failure. checkJobsOfRun will do additional work in these cases to "finish" the workflow call - // job as well. + // success/skip/failure. checkJobsOfRun will do additional work in these cases to "finish" the workflow + // call job as well. if allSucceedOrSkip { isIncompleteMatrix, _, _ := r.jobMap[id].HasIncompleteMatrix() isIncompleteWith, _, _, _ := r.jobMap[id].HasIncompleteWith() @@ -221,6 +224,12 @@ func (r *jobStatusResolver) resolve() map[int64]actions_model.Status { // `tryHandleIncompleteMatrix` to be reparsed, replaced with a full job definition, with new // `needs` that contain its inner jobs: ret[id] = actions_model.StatusWaiting + } else if allSkip { + // All of the inner jobs are skipped -- this most likely occurs because an outer job's `if:` + // condition was false, and that condition was populated to all the inner jobs. Even if that's + // not the case, it's effectively true that the reusable workflow was skipped if all the inner + // jobs had their own `if:` conditions that were skipped. + ret[id] = actions_model.StatusSkipped } else { // This job is done by virtue of its inner jobs being done successfully. ret[id] = actions_model.StatusSuccess diff --git a/services/actions/job_emitter_test.go b/services/actions/job_emitter_test.go index a7e3b7467b..d6696b3482 100644 --- a/services/actions/job_emitter_test.go +++ b/services/actions/job_emitter_test.go @@ -179,6 +179,27 @@ __metadata: 3: actions_model.StatusSuccess, }, }, + { + name: "unblocked workflow call outer job with only skip", + jobs: actions_model.ActionJobList{ + {ID: 1, JobID: "job1.innerjob1", Status: actions_model.StatusSkipped, Needs: []string{}}, + {ID: 2, JobID: "job1.innerjob2", Status: actions_model.StatusSkipped, Needs: []string{}}, + {ID: 3, JobID: "job1", Status: actions_model.StatusBlocked, Needs: []string{"job1.innerjob1", "job1.innerjob2"}, WorkflowPayload: []byte( + ` +name: test +on: push +jobs: + job2: + if: false + uses: ./.forgejo/workflows/reusable.yml +__metadata: + workflow_call_id: b5a9f46f1f2513d7777fde50b169d323a6519e349cc175484c947ac315a209ed +`)}, + }, + want: map[int64]actions_model.Status{ + 3: actions_model.StatusSkipped, + }, + }, { name: "unblocked workflow call outer job, incomplete `with`", jobs: actions_model.ActionJobList{