From 96a7f0a3f065c5db8fdf352c93c8367e24d259de Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Sat, 14 Dec 2024 10:22:30 +0800 Subject: [PATCH] Fix missing outputs for jobs with matrix (#32823) Fix #32795 If a job uses a matrix, multiple `ActionRunJobs` may have the same `JobID`. We need to merge the outputs of these jobs to make them available to the jobs that need them. (cherry picked from commit 7269130d2878d51dcdf11f7081a591f85bd493e8) Conflicts: models/fixtures/action_run.yml models/fixtures/action_run_job.yml trivial context conflicts --- models/actions/run_job.go | 4 +- models/fixtures/action_run.yml | 19 ++++++++ models/fixtures/action_run_job.yml | 43 +++++++++++++++++ models/fixtures/action_task.yml | 60 ++++++++++++++++++++++++ models/fixtures/action_task_output.yml | 20 ++++++++ routers/api/actions/runner/main_test.go | 16 +++++++ routers/api/actions/runner/utils.go | 60 +++++++++++++++++------- routers/api/actions/runner/utils_test.go | 29 ++++++++++++ 8 files changed, 233 insertions(+), 18 deletions(-) create mode 100644 models/fixtures/action_task_output.yml create mode 100644 routers/api/actions/runner/main_test.go create mode 100644 routers/api/actions/runner/utils_test.go diff --git a/models/actions/run_job.go b/models/actions/run_job.go index 4b8664077d..2319af8e08 100644 --- a/models/actions/run_job.go +++ b/models/actions/run_job.go @@ -137,7 +137,7 @@ func UpdateRunJob(ctx context.Context, job *ActionRunJob, cond builder.Cond, col if err != nil { return 0, err } - run.Status = aggregateJobStatus(jobs) + run.Status = AggregateJobStatus(jobs) if run.Started.IsZero() && run.Status.IsRunning() { run.Started = timeutil.TimeStampNow() } @@ -152,7 +152,7 @@ func UpdateRunJob(ctx context.Context, job *ActionRunJob, cond builder.Cond, col return affected, nil } -func aggregateJobStatus(jobs []*ActionRunJob) Status { +func AggregateJobStatus(jobs []*ActionRunJob) Status { allDone := true allWaiting := true hasFailure := false diff --git a/models/fixtures/action_run.yml b/models/fixtures/action_run.yml index 9c60b352f9..2fe9094d13 100644 --- a/models/fixtures/action_run.yml +++ b/models/fixtures/action_run.yml @@ -413,6 +413,25 @@ }, "total_commits": 0 } +- + id: 793 + title: "job output" + repo_id: 4 + owner_id: 1 + workflow_id: "test.yaml" + index: 189 + trigger_user_id: 1 + ref: "refs/heads/master" + commit_sha: "c2d72f548424103f01ee1dc02889c1e2bff816b0" + event: "push" + is_fork_pull_request: 0 + status: 1 + started: 1683636528 + stopped: 1683636626 + created: 1683636108 + updated: 1683636626 + need_approval: 0 + approved_by: 0 - id: 891 title: "update actions" diff --git a/models/fixtures/action_run_job.yml b/models/fixtures/action_run_job.yml index 0b02d0e17e..117bb5ea05 100644 --- a/models/fixtures/action_run_job.yml +++ b/models/fixtures/action_run_job.yml @@ -26,6 +26,49 @@ status: 1 started: 1683636528 stopped: 1683636626 +- + id: 194 + run_id: 793 + repo_id: 4 + owner_id: 1 + commit_sha: c2d72f548424103f01ee1dc02889c1e2bff816b0 + is_fork_pull_request: 0 + name: job1 (1) + attempt: 1 + job_id: job1 + task_id: 49 + status: 1 + started: 1683636528 + stopped: 1683636626 +- + id: 195 + run_id: 793 + repo_id: 4 + owner_id: 1 + commit_sha: c2d72f548424103f01ee1dc02889c1e2bff816b0 + is_fork_pull_request: 0 + name: job1 (2) + attempt: 1 + job_id: job1 + task_id: 50 + status: 1 + started: 1683636528 + stopped: 1683636626 +- + id: 196 + run_id: 793 + repo_id: 4 + owner_id: 1 + commit_sha: c2d72f548424103f01ee1dc02889c1e2bff816b0 + is_fork_pull_request: 0 + name: job2 + attempt: 1 + job_id: job2 + needs: [job1] + task_id: 51 + status: 5 + started: 1683636528 + stopped: 1683636626 - id: 292 run_id: 891 diff --git a/models/fixtures/action_task.yml b/models/fixtures/action_task.yml index d88a8ed8a9..506a47d8a0 100644 --- a/models/fixtures/action_task.yml +++ b/models/fixtures/action_task.yml @@ -57,3 +57,63 @@ log_length: 707 log_size: 90179 log_expired: 0 +- + id: 49 + job_id: 194 + attempt: 1 + runner_id: 1 + status: 1 # success + started: 1683636528 + stopped: 1683636626 + repo_id: 4 + owner_id: 1 + commit_sha: c2d72f548424103f01ee1dc02889c1e2bff816b0 + is_fork_pull_request: 0 + token_hash: b8d3962425466b6709b9ac51446f93260c54afe8e7b6d3686e34f991fb8a8953822b0deed86fe41a103f34bc48dbc4784220 + token_salt: ffffffffff + token_last_eight: ffffffff + log_filename: artifact-test2/2f/47.log + log_in_storage: 1 + log_length: 707 + log_size: 90179 + log_expired: 0 +- + id: 50 + job_id: 195 + attempt: 1 + runner_id: 1 + status: 1 # success + started: 1683636528 + stopped: 1683636626 + repo_id: 4 + owner_id: 1 + commit_sha: c2d72f548424103f01ee1dc02889c1e2bff816b0 + is_fork_pull_request: 0 + token_hash: b8d3962425466b6709b9ac51446f93260c54afe8e7b6d3686e34f991fb8a8953822b0deed86fe41a103f34bc48dbc4784221 + token_salt: ffffffffff + token_last_eight: ffffffff + log_filename: artifact-test2/2f/47.log + log_in_storage: 1 + log_length: 707 + log_size: 90179 + log_expired: 0 +- + id: 51 + job_id: 196 + attempt: 1 + runner_id: 1 + status: 6 # running + started: 1683636528 + stopped: 1683636626 + repo_id: 4 + owner_id: 1 + commit_sha: c2d72f548424103f01ee1dc02889c1e2bff816b0 + is_fork_pull_request: 0 + token_hash: b8d3962425466b6709b9ac51446f93260c54afe8e7b6d3686e34f991fb8a8953822b0deed86fe41a103f34bc48dbc4784222 + token_salt: ffffffffff + token_last_eight: ffffffff + log_filename: artifact-test2/2f/47.log + log_in_storage: 1 + log_length: 707 + log_size: 90179 + log_expired: 0 diff --git a/models/fixtures/action_task_output.yml b/models/fixtures/action_task_output.yml new file mode 100644 index 0000000000..314e9f7115 --- /dev/null +++ b/models/fixtures/action_task_output.yml @@ -0,0 +1,20 @@ +- + id: 1 + task_id: 49 + output_key: output_a + output_value: abc +- + id: 2 + task_id: 49 + output_key: output_b + output_value: '' +- + id: 3 + task_id: 50 + output_key: output_a + output_value: '' +- + id: 4 + task_id: 50 + output_key: output_b + output_value: bbb diff --git a/routers/api/actions/runner/main_test.go b/routers/api/actions/runner/main_test.go new file mode 100644 index 0000000000..bed63c166e --- /dev/null +++ b/routers/api/actions/runner/main_test.go @@ -0,0 +1,16 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package runner + +import ( + "testing" + + "code.gitea.io/gitea/models/unittest" + + _ "code.gitea.io/gitea/models/forgefed" +) + +func TestMain(m *testing.M) { + unittest.MainTest(m) +} diff --git a/routers/api/actions/runner/utils.go b/routers/api/actions/runner/utils.go index ff6ec5bd54..539be8d889 100644 --- a/routers/api/actions/runner/utils.go +++ b/routers/api/actions/runner/utils.go @@ -162,28 +162,56 @@ func findTaskNeeds(ctx context.Context, task *actions_model.ActionTask) (map[str return nil, fmt.Errorf("FindRunJobs: %w", err) } - ret := make(map[string]*runnerv1.TaskNeed, len(needs)) + jobIDJobs := make(map[string][]*actions_model.ActionRunJob) for _, job := range jobs { - if !needs.Contains(job.JobID) { + jobIDJobs[job.JobID] = append(jobIDJobs[job.JobID], job) + } + + ret := make(map[string]*runnerv1.TaskNeed, len(needs)) + for jobID, jobsWithSameID := range jobIDJobs { + if !needs.Contains(jobID) { continue } - if job.TaskID == 0 || !job.Status.IsDone() { - // it shouldn't happen, or the job has been rerun - continue + var jobOutputs map[string]string + for _, job := range jobsWithSameID { + if job.TaskID == 0 || !job.Status.IsDone() { + // it shouldn't happen, or the job has been rerun + continue + } + got, err := actions_model.FindTaskOutputByTaskID(ctx, job.TaskID) + if err != nil { + return nil, fmt.Errorf("FindTaskOutputByTaskID: %w", err) + } + outputs := make(map[string]string, len(got)) + for _, v := range got { + outputs[v.OutputKey] = v.OutputValue + } + if len(jobOutputs) == 0 { + jobOutputs = outputs + } else { + jobOutputs = mergeTwoOutputs(outputs, jobOutputs) + } } - outputs := make(map[string]string) - got, err := actions_model.FindTaskOutputByTaskID(ctx, job.TaskID) - if err != nil { - return nil, fmt.Errorf("FindTaskOutputByTaskID: %w", err) - } - for _, v := range got { - outputs[v.OutputKey] = v.OutputValue - } - ret[job.JobID] = &runnerv1.TaskNeed{ - Outputs: outputs, - Result: runnerv1.Result(job.Status), + ret[jobID] = &runnerv1.TaskNeed{ + Outputs: jobOutputs, + Result: runnerv1.Result(actions_model.AggregateJobStatus(jobsWithSameID)), } } return ret, nil } + +// mergeTwoOutputs merges two outputs from two different ActionRunJobs +// Values with the same output name may be overridden. The user should ensure the output names are unique. +// See https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#using-job-outputs-in-a-matrix-job +func mergeTwoOutputs(o1, o2 map[string]string) map[string]string { + ret := make(map[string]string, len(o1)) + for k1, v1 := range o1 { + if len(v1) > 0 { + ret[k1] = v1 + } else { + ret[k1] = o2[k1] + } + } + return ret +} diff --git a/routers/api/actions/runner/utils_test.go b/routers/api/actions/runner/utils_test.go new file mode 100644 index 0000000000..c8a0a28d65 --- /dev/null +++ b/routers/api/actions/runner/utils_test.go @@ -0,0 +1,29 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package runner + +import ( + "context" + "testing" + + actions_model "code.gitea.io/gitea/models/actions" + "code.gitea.io/gitea/models/unittest" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func Test_findTaskNeeds(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + task := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: 51}) + + ret, err := findTaskNeeds(context.Background(), task) + require.NoError(t, err) + assert.Len(t, ret, 1) + assert.Contains(t, ret, "job1") + assert.Len(t, ret["job1"].Outputs, 2) + assert.Equal(t, "abc", ret["job1"].Outputs["output_a"]) + assert.Equal(t, "bbb", ret["job1"].Outputs["output_b"]) +}