From 31a0c4dfb4156a7b4d856cceae1e61c7fc1a4a1b Mon Sep 17 00:00:00 2001
From: Zettat123 <zettat123@gmail.com>
Date: Mon, 27 May 2024 14:15:34 +0800
Subject: [PATCH] Improve the handling of `jobs.<job_id>.if` (#31070)

Fix #25897
Fix #30322

#29464 cannot handle some complex `if` conditions correctly because it
only checks `always()` literally. In fact, it's not easy to evaluate the
`if` condition on the Gitea side because evaluating it requires a series
of contexts. But act_runner is able to evaluate the `if` condition
before running the job (for more information, see
[`gitea/act`](https://gitea.com/gitea/act/src/commit/517d11c67126bd97c88e2faabda0832fff482258/pkg/runner/run_context.go#L739-L753))
. So we can use act_runner to check the `if` condition.

In this PR, how to handle a blocked job depends on its `needs` and `if`:
- If not all jobs in `needs` completed successfully and the job's `if`
is empty, set the job status to `StatusSkipped`
- In other cases, the job status will be set to `StatusWaiting`, and
then act_runner will check the `if` condition and run the job if the
condition is met
---
 services/actions/job_emitter.go      | 14 +++++++-------
 services/actions/job_emitter_test.go | 18 +++++++++---------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/services/actions/job_emitter.go b/services/actions/job_emitter.go
index d2bbbd9a7ca..1f859fcf705 100644
--- a/services/actions/job_emitter.go
+++ b/services/actions/job_emitter.go
@@ -7,7 +7,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"strings"
 
 	actions_model "code.gitea.io/gitea/models/actions"
 	"code.gitea.io/gitea/models/db"
@@ -141,18 +140,19 @@ func (r *jobStatusResolver) resolve() map[int64]actions_model.Status {
 			if allSucceed {
 				ret[id] = actions_model.StatusWaiting
 			} else {
-				// If a job's "if" condition is "always()", the job should always run even if some of its dependencies did not succeed.
-				// See https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idneeds
-				always := false
+				// Check if the job has an "if" condition
+				hasIf := false
 				if wfJobs, _ := jobparser.Parse(r.jobMap[id].WorkflowPayload); len(wfJobs) == 1 {
 					_, wfJob := wfJobs[0].Job()
-					expr := strings.TrimSpace(strings.TrimSuffix(strings.TrimPrefix(wfJob.If.Value, "${{"), "}}"))
-					always = expr == "always()"
+					hasIf = len(wfJob.If.Value) > 0
 				}
 
-				if always {
+				if hasIf {
+					// act_runner will check the "if" condition
 					ret[id] = actions_model.StatusWaiting
 				} else {
+					// If the "if" condition is empty and not all dependent jobs completed successfully,
+					// the job should be skipped.
 					ret[id] = actions_model.StatusSkipped
 				}
 			}
diff --git a/services/actions/job_emitter_test.go b/services/actions/job_emitter_test.go
index 038df7d4f86..58c2dc3b242 100644
--- a/services/actions/job_emitter_test.go
+++ b/services/actions/job_emitter_test.go
@@ -71,9 +71,9 @@ func Test_jobStatusResolver_Resolve(t *testing.T) {
 			want: map[int64]actions_model.Status{},
 		},
 		{
-			name: "with ${{ always() }} condition",
+			name: "`if` is not empty and all jobs in `needs` completed successfully",
 			jobs: actions_model.ActionJobList{
-				{ID: 1, JobID: "job1", Status: actions_model.StatusFailure, Needs: []string{}},
+				{ID: 1, JobID: "job1", Status: actions_model.StatusSuccess, Needs: []string{}},
 				{ID: 2, JobID: "job2", Status: actions_model.StatusBlocked, Needs: []string{"job1"}, WorkflowPayload: []byte(
 					`
 name: test
@@ -82,15 +82,15 @@ jobs:
   job2:
     runs-on: ubuntu-latest
     needs: job1
-    if: ${{ always() }}
+    if: ${{ always() && needs.job1.result == 'success' }}
     steps:
-      - run: echo "always run"
+      - run: echo "will be checked by act_runner"
 `)},
 			},
 			want: map[int64]actions_model.Status{2: actions_model.StatusWaiting},
 		},
 		{
-			name: "with always() condition",
+			name: "`if` is not empty and not all jobs in `needs` completed successfully",
 			jobs: actions_model.ActionJobList{
 				{ID: 1, JobID: "job1", Status: actions_model.StatusFailure, Needs: []string{}},
 				{ID: 2, JobID: "job2", Status: actions_model.StatusBlocked, Needs: []string{"job1"}, WorkflowPayload: []byte(
@@ -101,15 +101,15 @@ jobs:
   job2:
     runs-on: ubuntu-latest
     needs: job1
-    if: always()
+    if: ${{ always() && needs.job1.result == 'failure' }}
     steps:
-      - run: echo "always run"
+      - run: echo "will be checked by act_runner"
 `)},
 			},
 			want: map[int64]actions_model.Status{2: actions_model.StatusWaiting},
 		},
 		{
-			name: "without always() condition",
+			name: "`if` is empty and not all jobs in `needs` completed successfully",
 			jobs: actions_model.ActionJobList{
 				{ID: 1, JobID: "job1", Status: actions_model.StatusFailure, Needs: []string{}},
 				{ID: 2, JobID: "job2", Status: actions_model.StatusBlocked, Needs: []string{"job1"}, WorkflowPayload: []byte(
@@ -121,7 +121,7 @@ jobs:
     runs-on: ubuntu-latest
     needs: job1
     steps:
-      - run: echo "not always run"
+      - run: echo "should be skipped"
 `)},
 			},
 			want: map[int64]actions_model.Status{2: actions_model.StatusSkipped},