From 321cbcb45a947262471d273e8d10ee19ee614932 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 27 Mar 2025 17:20:56 -0700 Subject: [PATCH] Fix bug on downloading job logs (#34041) Fix #34038 --- routers/common/actions.go | 2 +- tests/integration/actions_log_test.go | 193 ++++++++++++++++---------- 2 files changed, 121 insertions(+), 74 deletions(-) diff --git a/routers/common/actions.go b/routers/common/actions.go index 3e9198b4f4..a4eabb6ba2 100644 --- a/routers/common/actions.go +++ b/routers/common/actions.go @@ -22,7 +22,7 @@ func DownloadActionsRunJobLogsWithIndex(ctx *context.Base, ctxRepo *repo_model.R if err = runJobs.LoadRepos(ctx); err != nil { return fmt.Errorf("LoadRepos: %w", err) } - if 0 < jobIndex || jobIndex >= int64(len(runJobs)) { + if jobIndex < 0 || jobIndex >= int64(len(runJobs)) { return util.NewNotExistErrorf("job index is out of range: %d", jobIndex) } return DownloadActionsRunJobLogs(ctx, ctxRepo, runJobs[jobIndex]) diff --git a/tests/integration/actions_log_test.go b/tests/integration/actions_log_test.go index a157a923f6..cd20604b84 100644 --- a/tests/integration/actions_log_test.go +++ b/tests/integration/actions_log_test.go @@ -31,7 +31,7 @@ func TestDownloadTaskLogs(t *testing.T) { testCases := []struct { treePath string fileContent string - outcome *mockTaskOutcome + outcome []*mockTaskOutcome zstdEnabled bool }{ { @@ -46,21 +46,44 @@ jobs: runs-on: ubuntu-latest steps: - run: echo job1 with zstd enabled + job2: + runs-on: ubuntu-latest + steps: + - run: echo job2 with zstd enabled `, - outcome: &mockTaskOutcome{ - result: runnerv1.Result_RESULT_SUCCESS, - logRows: []*runnerv1.LogRow{ - { - Time: timestamppb.New(now.Add(1 * time.Second)), - Content: " \U0001F433 docker create image", + outcome: []*mockTaskOutcome{ + { + result: runnerv1.Result_RESULT_SUCCESS, + logRows: []*runnerv1.LogRow{ + { + Time: timestamppb.New(now.Add(1 * time.Second)), + Content: " \U0001F433 docker create image", + }, + { + Time: timestamppb.New(now.Add(2 * time.Second)), + Content: "job1 zstd enabled", + }, + { + Time: timestamppb.New(now.Add(3 * time.Second)), + Content: "\U0001F3C1 Job succeeded", + }, }, - { - Time: timestamppb.New(now.Add(2 * time.Second)), - Content: "job1 zstd enabled", - }, - { - Time: timestamppb.New(now.Add(3 * time.Second)), - Content: "\U0001F3C1 Job succeeded", + }, + { + result: runnerv1.Result_RESULT_SUCCESS, + logRows: []*runnerv1.LogRow{ + { + Time: timestamppb.New(now.Add(1 * time.Second)), + Content: " \U0001F433 docker create image", + }, + { + Time: timestamppb.New(now.Add(2 * time.Second)), + Content: "job2 zstd enabled", + }, + { + Time: timestamppb.New(now.Add(3 * time.Second)), + Content: "\U0001F3C1 Job succeeded", + }, }, }, }, @@ -78,21 +101,44 @@ jobs: runs-on: ubuntu-latest steps: - run: echo job1 with zstd disabled + job2: + runs-on: ubuntu-latest + steps: + - run: echo job2 with zstd disabled `, - outcome: &mockTaskOutcome{ - result: runnerv1.Result_RESULT_SUCCESS, - logRows: []*runnerv1.LogRow{ - { - Time: timestamppb.New(now.Add(4 * time.Second)), - Content: " \U0001F433 docker create image", + outcome: []*mockTaskOutcome{ + { + result: runnerv1.Result_RESULT_SUCCESS, + logRows: []*runnerv1.LogRow{ + { + Time: timestamppb.New(now.Add(4 * time.Second)), + Content: " \U0001F433 docker create image", + }, + { + Time: timestamppb.New(now.Add(5 * time.Second)), + Content: "job1 zstd disabled", + }, + { + Time: timestamppb.New(now.Add(6 * time.Second)), + Content: "\U0001F3C1 Job succeeded", + }, }, - { - Time: timestamppb.New(now.Add(5 * time.Second)), - Content: "job1 zstd disabled", - }, - { - Time: timestamppb.New(now.Add(6 * time.Second)), - Content: "\U0001F3C1 Job succeeded", + }, + { + result: runnerv1.Result_RESULT_SUCCESS, + logRows: []*runnerv1.LogRow{ + { + Time: timestamppb.New(now.Add(4 * time.Second)), + Content: " \U0001F433 docker create image", + }, + { + Time: timestamppb.New(now.Add(5 * time.Second)), + Content: "job2 zstd disabled", + }, + { + Time: timestamppb.New(now.Add(6 * time.Second)), + Content: "\U0001F3C1 Job succeeded", + }, }, }, }, @@ -124,54 +170,55 @@ jobs: opts := getWorkflowCreateFileOptions(user2, repo.DefaultBranch, fmt.Sprintf("create %s", tc.treePath), tc.fileContent) createWorkflowFile(t, token, user2.Name, repo.Name, tc.treePath, opts) - // fetch and execute task - task := runner.fetchTask(t) - runner.execTask(t, task, tc.outcome) + // fetch and execute tasks + for jobIndex, outcome := range tc.outcome { + task := runner.fetchTask(t) + runner.execTask(t, task, outcome) - // check whether the log file exists - logFileName := fmt.Sprintf("%s/%02x/%d.log", repo.FullName(), task.Id%256, task.Id) - if setting.Actions.LogCompression.IsZstd() { - logFileName += ".zst" + // check whether the log file exists + logFileName := fmt.Sprintf("%s/%02x/%d.log", repo.FullName(), task.Id%256, task.Id) + if setting.Actions.LogCompression.IsZstd() { + logFileName += ".zst" + } + _, err := storage.Actions.Stat(logFileName) + assert.NoError(t, err) + + // download task logs and check content + runIndex := task.Context.GetFields()["run_number"].GetStringValue() + req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/actions/runs/%s/jobs/%d/logs", user2.Name, repo.Name, runIndex, jobIndex)). + AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) + logTextLines := strings.Split(strings.TrimSpace(resp.Body.String()), "\n") + assert.Len(t, logTextLines, len(outcome.logRows)) + for idx, lr := range outcome.logRows { + assert.Equal( + t, + fmt.Sprintf("%s %s", lr.Time.AsTime().Format("2006-01-02T15:04:05.0000000Z07:00"), lr.Content), + logTextLines[idx], + ) + } + + runID, _ := strconv.ParseInt(task.Context.GetFields()["run_id"].GetStringValue(), 10, 64) + + jobs, err := actions_model.GetRunJobsByRunID(t.Context(), runID) + assert.NoError(t, err) + assert.Len(t, jobs, len(tc.outcome)) + jobID := jobs[jobIndex].ID + + // download task logs from API and check content + req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/actions/jobs/%d/logs", user2.Name, repo.Name, jobID)). + AddTokenAuth(token) + resp = MakeRequest(t, req, http.StatusOK) + logTextLines = strings.Split(strings.TrimSpace(resp.Body.String()), "\n") + assert.Len(t, logTextLines, len(outcome.logRows)) + for idx, lr := range outcome.logRows { + assert.Equal( + t, + fmt.Sprintf("%s %s", lr.Time.AsTime().Format("2006-01-02T15:04:05.0000000Z07:00"), lr.Content), + logTextLines[idx], + ) + } } - _, err := storage.Actions.Stat(logFileName) - assert.NoError(t, err) - - // download task logs and check content - runIndex := task.Context.GetFields()["run_number"].GetStringValue() - req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/actions/runs/%s/jobs/0/logs", user2.Name, repo.Name, runIndex)). - AddTokenAuth(token) - resp := MakeRequest(t, req, http.StatusOK) - logTextLines := strings.Split(strings.TrimSpace(resp.Body.String()), "\n") - assert.Len(t, logTextLines, len(tc.outcome.logRows)) - for idx, lr := range tc.outcome.logRows { - assert.Equal( - t, - fmt.Sprintf("%s %s", lr.Time.AsTime().Format("2006-01-02T15:04:05.0000000Z07:00"), lr.Content), - logTextLines[idx], - ) - } - - runID, _ := strconv.ParseInt(task.Context.GetFields()["run_id"].GetStringValue(), 10, 64) - - jobs, err := actions_model.GetRunJobsByRunID(t.Context(), runID) - assert.NoError(t, err) - assert.Len(t, jobs, 1) - jobID := jobs[0].ID - - // download task logs from API and check content - req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/actions/jobs/%d/logs", user2.Name, repo.Name, jobID)). - AddTokenAuth(token) - resp = MakeRequest(t, req, http.StatusOK) - logTextLines = strings.Split(strings.TrimSpace(resp.Body.String()), "\n") - assert.Len(t, logTextLines, len(tc.outcome.logRows)) - for idx, lr := range tc.outcome.logRows { - assert.Equal( - t, - fmt.Sprintf("%s %s", lr.Time.AsTime().Format("2006-01-02T15:04:05.0000000Z07:00"), lr.Content), - logTextLines[idx], - ) - } - resetFunc() }) }