From 7b5c131f12f439fdd75fc352739ea09b020c6c26 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Wed, 28 Feb 2024 10:27:42 +0800 Subject: [PATCH 1/8] check the 'always' function --- go.mod | 2 +- services/actions/job_emitter.go | 32 +++++++++++++++++++++++++++++++- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index 03f6ad1215849..c27b086c7ca97 100644 --- a/go.mod +++ b/go.mod @@ -89,6 +89,7 @@ require ( github.com/prometheus/client_golang v1.18.0 github.com/quasoft/websspi v1.1.2 github.com/redis/go-redis/v9 v9.4.0 + github.com/rhysd/actionlint v1.6.26 github.com/robfig/cron/v3 v3.0.1 github.com/santhosh-tekuri/jsonschema/v5 v5.3.1 github.com/sassoftware/go-rpmutils v0.2.1-0.20240124161140-277b154961dd @@ -249,7 +250,6 @@ require ( github.com/prometheus/client_model v0.5.0 // indirect github.com/prometheus/common v0.46.0 // indirect github.com/prometheus/procfs v0.12.0 // indirect - github.com/rhysd/actionlint v1.6.26 // indirect github.com/rivo/uniseg v0.4.4 // indirect github.com/rogpeppe/go-internal v1.12.0 // indirect github.com/rs/xid v1.5.0 // indirect diff --git a/services/actions/job_emitter.go b/services/actions/job_emitter.go index fe39312386d92..384a68c9263f8 100644 --- a/services/actions/job_emitter.go +++ b/services/actions/job_emitter.go @@ -7,12 +7,15 @@ import ( "context" "errors" "fmt" + "strings" actions_model "code.gitea.io/gitea/models/actions" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/queue" + "github.com/nektos/act/pkg/jobparser" + "github.com/rhysd/actionlint" "xorm.io/builder" ) @@ -76,12 +79,15 @@ func checkJobsOfRun(ctx context.Context, runID int64) error { type jobStatusResolver struct { statuses map[int64]actions_model.Status needs map[int64][]int64 + jobMap map[int64]*actions_model.ActionRunJob } func newJobStatusResolver(jobs actions_model.ActionJobList) *jobStatusResolver { idToJobs := make(map[string][]*actions_model.ActionRunJob, len(jobs)) + jobMap := make(map[int64]*actions_model.ActionRunJob) for _, job := range jobs { idToJobs[job.JobID] = append(idToJobs[job.JobID], job) + jobMap[job.ID] = job } statuses := make(map[int64]actions_model.Status, len(jobs)) @@ -97,6 +103,7 @@ func newJobStatusResolver(jobs actions_model.ActionJobList) *jobStatusResolver { return &jobStatusResolver{ statuses: statuses, needs: needs, + jobMap: jobMap, } } @@ -135,7 +142,30 @@ func (r *jobStatusResolver) resolve() map[int64]actions_model.Status { if allSucceed { ret[id] = actions_model.StatusWaiting } else { - ret[id] = actions_model.StatusSkipped + // 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 + wfJobs, _ := jobparser.Parse(r.jobMap[id].WorkflowPayload) + if len(wfJobs) == 1 { + _, wfJob := wfJobs[0].Job() + if wfJob.If.Value != "" { + // We use "actionlint" to check whether the value of "if" is the "always()" function + value := strings.TrimPrefix(wfJob.If.Value, "${{") + exprParser := actionlint.NewExprParser() + exprNode, _ := exprParser.Parse(actionlint.NewExprLexer(value)) + if funcNode, ok := (exprNode).(*actionlint.FuncCallNode); ok { + if funcNode.Callee == "always" { + always = true + } + } + } + } + + if always { + ret[id] = actions_model.StatusWaiting + } else { + ret[id] = actions_model.StatusSkipped + } } } } From 608f2b32cb4075e14fb2a896813c3b9950827777 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Wed, 28 Feb 2024 11:07:22 +0800 Subject: [PATCH 2/8] make fmt --- services/actions/job_emitter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/actions/job_emitter.go b/services/actions/job_emitter.go index 384a68c9263f8..245722ace2947 100644 --- a/services/actions/job_emitter.go +++ b/services/actions/job_emitter.go @@ -13,8 +13,8 @@ import ( "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/queue" - "github.com/nektos/act/pkg/jobparser" + "github.com/nektos/act/pkg/jobparser" "github.com/rhysd/actionlint" "xorm.io/builder" ) From aac47fa86570bb2b1d386f4eaab5b739bad43b8a Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Wed, 28 Feb 2024 12:21:37 +0800 Subject: [PATCH 3/8] add test --- services/actions/job_emitter_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/services/actions/job_emitter_test.go b/services/actions/job_emitter_test.go index e81aa61d80730..1251df707e715 100644 --- a/services/actions/job_emitter_test.go +++ b/services/actions/job_emitter_test.go @@ -70,6 +70,24 @@ func Test_jobStatusResolver_Resolve(t *testing.T) { }, want: map[int64]actions_model.Status{}, }, + { + name: "with always() condition", + 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( + "name: test\non: push\njobs:\n job2:\n runs-on: ubuntu-latest\n needs: job1\n if: ${{ always() }}\n steps:\n - run: echo \"always run\"")}, + }, + want: map[int64]actions_model.Status{2: actions_model.StatusWaiting}, + }, + { + name: "without always() condition", + 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( + "name: test\non: push\njobs:\n job2:\n runs-on: ubuntu-latest\n needs: job1\n steps:\n - run: echo \"always run\"")}, + }, + want: map[int64]actions_model.Status{2: actions_model.StatusSkipped}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 2d5c994382c778e3bbed4f62c5c6afe1a9345e6d Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Wed, 28 Feb 2024 13:10:02 +0800 Subject: [PATCH 4/8] fix missing '}}' --- services/actions/job_emitter.go | 5 +++++ services/actions/job_emitter_test.go | 11 ++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/services/actions/job_emitter.go b/services/actions/job_emitter.go index 245722ace2947..ba5a1b12a64fe 100644 --- a/services/actions/job_emitter.go +++ b/services/actions/job_emitter.go @@ -151,6 +151,11 @@ func (r *jobStatusResolver) resolve() map[int64]actions_model.Status { if wfJob.If.Value != "" { // We use "actionlint" to check whether the value of "if" is the "always()" function value := strings.TrimPrefix(wfJob.If.Value, "${{") + if !strings.HasSuffix(value, "}}") { + // "}}" is necessary since lexer lexes it as end of tokens + // See https://github.com/rhysd/actionlint/blob/3e2f8eab86d3490068c620638bb2955598438492/rule_expression.go#L622 + value = value + "}}" + } exprParser := actionlint.NewExprParser() exprNode, _ := exprParser.Parse(actionlint.NewExprLexer(value)) if funcNode, ok := (exprNode).(*actionlint.FuncCallNode); ok { diff --git a/services/actions/job_emitter_test.go b/services/actions/job_emitter_test.go index 1251df707e715..82dcdc6c08709 100644 --- a/services/actions/job_emitter_test.go +++ b/services/actions/job_emitter_test.go @@ -71,7 +71,7 @@ func Test_jobStatusResolver_Resolve(t *testing.T) { want: map[int64]actions_model.Status{}, }, { - name: "with always() condition", + name: "with ${{ always() }} condition", 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( @@ -79,6 +79,15 @@ func Test_jobStatusResolver_Resolve(t *testing.T) { }, want: map[int64]actions_model.Status{2: actions_model.StatusWaiting}, }, + { + name: "with always() condition", + 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( + "name: test\non: push\njobs:\n job2:\n runs-on: ubuntu-latest\n needs: job1\n if: always()\n steps:\n - run: echo \"always run\"")}, + }, + want: map[int64]actions_model.Status{2: actions_model.StatusWaiting}, + }, { name: "without always() condition", jobs: actions_model.ActionJobList{ From 3952ec6ff17cbec8a5ac78544e0f2afcb57fbebc Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Wed, 28 Feb 2024 13:24:42 +0800 Subject: [PATCH 5/8] lint-backend --- services/actions/job_emitter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/actions/job_emitter.go b/services/actions/job_emitter.go index ba5a1b12a64fe..52c37ad5632ad 100644 --- a/services/actions/job_emitter.go +++ b/services/actions/job_emitter.go @@ -154,7 +154,7 @@ func (r *jobStatusResolver) resolve() map[int64]actions_model.Status { if !strings.HasSuffix(value, "}}") { // "}}" is necessary since lexer lexes it as end of tokens // See https://github.com/rhysd/actionlint/blob/3e2f8eab86d3490068c620638bb2955598438492/rule_expression.go#L622 - value = value + "}}" + value += "}}" } exprParser := actionlint.NewExprParser() exprNode, _ := exprParser.Parse(actionlint.NewExprLexer(value)) From ad96269b0bd154deaf6582b51d1abd11c33b8946 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Wed, 28 Feb 2024 13:43:47 +0800 Subject: [PATCH 6/8] improve the workflow format in test cases --- services/actions/job_emitter_test.go | 35 +++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/services/actions/job_emitter_test.go b/services/actions/job_emitter_test.go index 82dcdc6c08709..038df7d4f86b3 100644 --- a/services/actions/job_emitter_test.go +++ b/services/actions/job_emitter_test.go @@ -75,7 +75,17 @@ func Test_jobStatusResolver_Resolve(t *testing.T) { 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( - "name: test\non: push\njobs:\n job2:\n runs-on: ubuntu-latest\n needs: job1\n if: ${{ always() }}\n steps:\n - run: echo \"always run\"")}, + ` +name: test +on: push +jobs: + job2: + runs-on: ubuntu-latest + needs: job1 + if: ${{ always() }} + steps: + - run: echo "always run" +`)}, }, want: map[int64]actions_model.Status{2: actions_model.StatusWaiting}, }, @@ -84,7 +94,17 @@ func Test_jobStatusResolver_Resolve(t *testing.T) { 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( - "name: test\non: push\njobs:\n job2:\n runs-on: ubuntu-latest\n needs: job1\n if: always()\n steps:\n - run: echo \"always run\"")}, + ` +name: test +on: push +jobs: + job2: + runs-on: ubuntu-latest + needs: job1 + if: always() + steps: + - run: echo "always run" +`)}, }, want: map[int64]actions_model.Status{2: actions_model.StatusWaiting}, }, @@ -93,7 +113,16 @@ func Test_jobStatusResolver_Resolve(t *testing.T) { 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( - "name: test\non: push\njobs:\n job2:\n runs-on: ubuntu-latest\n needs: job1\n steps:\n - run: echo \"always run\"")}, + ` +name: test +on: push +jobs: + job2: + runs-on: ubuntu-latest + needs: job1 + steps: + - run: echo "not always run" +`)}, }, want: map[int64]actions_model.Status{2: actions_model.StatusSkipped}, }, From 81ecb43024c662062034f719e1813d4f3e292147 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Wed, 28 Feb 2024 13:47:12 +0800 Subject: [PATCH 7/8] do not use parser --- go.mod | 2 +- services/actions/job_emitter.go | 24 ++++++------------------ 2 files changed, 7 insertions(+), 19 deletions(-) diff --git a/go.mod b/go.mod index c27b086c7ca97..03f6ad1215849 100644 --- a/go.mod +++ b/go.mod @@ -89,7 +89,6 @@ require ( github.com/prometheus/client_golang v1.18.0 github.com/quasoft/websspi v1.1.2 github.com/redis/go-redis/v9 v9.4.0 - github.com/rhysd/actionlint v1.6.26 github.com/robfig/cron/v3 v3.0.1 github.com/santhosh-tekuri/jsonschema/v5 v5.3.1 github.com/sassoftware/go-rpmutils v0.2.1-0.20240124161140-277b154961dd @@ -250,6 +249,7 @@ require ( github.com/prometheus/client_model v0.5.0 // indirect github.com/prometheus/common v0.46.0 // indirect github.com/prometheus/procfs v0.12.0 // indirect + github.com/rhysd/actionlint v1.6.26 // indirect github.com/rivo/uniseg v0.4.4 // indirect github.com/rogpeppe/go-internal v1.12.0 // indirect github.com/rs/xid v1.5.0 // indirect diff --git a/services/actions/job_emitter.go b/services/actions/job_emitter.go index 52c37ad5632ad..a55351ef042ec 100644 --- a/services/actions/job_emitter.go +++ b/services/actions/job_emitter.go @@ -15,7 +15,6 @@ import ( "code.gitea.io/gitea/modules/queue" "github.com/nektos/act/pkg/jobparser" - "github.com/rhysd/actionlint" "xorm.io/builder" ) @@ -145,25 +144,14 @@ func (r *jobStatusResolver) resolve() map[int64]actions_model.Status { // 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 - wfJobs, _ := jobparser.Parse(r.jobMap[id].WorkflowPayload) + wfJobs, perr := jobparser.Parse(r.jobMap[id].WorkflowPayload) + if perr != nil && len(r.jobMap[id].WorkflowPayload) > 0 { + _ = perr + } if len(wfJobs) == 1 { _, wfJob := wfJobs[0].Job() - if wfJob.If.Value != "" { - // We use "actionlint" to check whether the value of "if" is the "always()" function - value := strings.TrimPrefix(wfJob.If.Value, "${{") - if !strings.HasSuffix(value, "}}") { - // "}}" is necessary since lexer lexes it as end of tokens - // See https://github.com/rhysd/actionlint/blob/3e2f8eab86d3490068c620638bb2955598438492/rule_expression.go#L622 - value += "}}" - } - exprParser := actionlint.NewExprParser() - exprNode, _ := exprParser.Parse(actionlint.NewExprLexer(value)) - if funcNode, ok := (exprNode).(*actionlint.FuncCallNode); ok { - if funcNode.Callee == "always" { - always = true - } - } - } + expr := strings.TrimSpace(strings.TrimSuffix(strings.TrimPrefix(wfJob.If.Value, "${{"), "}}")) + always = expr == "always()" } if always { From b036eada8f8c1566596bc9bc02ca087b142aee88 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Wed, 28 Feb 2024 13:52:52 +0800 Subject: [PATCH 8/8] simplify code --- services/actions/job_emitter.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/services/actions/job_emitter.go b/services/actions/job_emitter.go index a55351ef042ec..d2bbbd9a7ca29 100644 --- a/services/actions/job_emitter.go +++ b/services/actions/job_emitter.go @@ -144,11 +144,7 @@ func (r *jobStatusResolver) resolve() map[int64]actions_model.Status { // 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 - wfJobs, perr := jobparser.Parse(r.jobMap[id].WorkflowPayload) - if perr != nil && len(r.jobMap[id].WorkflowPayload) > 0 { - _ = perr - } - if len(wfJobs) == 1 { + 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()"