mirror of
https://github.com/go-gitea/gitea.git
synced 2026-06-19 17:44:00 +02:00
fix(actions): address review issues for Actions API endpoints
- Use EffectiveTaskID() for rerun jobs in DownloadActionsRunAllJobLogs - Add nil guard for deleted repos in shared.ListRuns - Make ApproveWorkflowRun idempotent (return 200 on re-approve) - Remove stale reload in ApproveWorkflowRun, use in-memory updates - Log zip errors instead of corrupting response mid-stream - Fix misleading Zip Slip comment - Revert DecodeJSON -> json.Unmarshal changes in tests - Add test coverage: cancel state assertions, zip headers, writer-but-non-admin approve, rerun job logs - Fix ToActionWorkflowRun to use repo parameter consistently Co-Authored-By: Ross Golder <rossg@rossgolder.com> Co-Authored-By: OpenCode Agent <opencode@rossgolder.com>
This commit is contained in:
Ross Golder
parent
6ecbe0f771
commit
8c90b491e5
@ -181,8 +181,14 @@ func ApproveWorkflowRun(ctx *context.APIContext) {
|
||||
return
|
||||
}
|
||||
|
||||
// GitHub-compatible: return 200 if already approved (idempotent)
|
||||
if !run.NeedApproval {
|
||||
ctx.APIError(http.StatusBadRequest, "Run does not require approval")
|
||||
convertedRun, err := convert.ToActionWorkflowRun(ctx, ctx.Repo.Repository, run, nil)
|
||||
if err != nil {
|
||||
ctx.APIErrorInternal(err)
|
||||
return
|
||||
}
|
||||
ctx.JSON(http.StatusOK, convertedRun)
|
||||
return
|
||||
}
|
||||
|
||||
@ -195,14 +201,13 @@ func ApproveWorkflowRun(ctx *context.APIContext) {
|
||||
return
|
||||
}
|
||||
|
||||
// Reload run to reflect post-approval state.
|
||||
updatedRun, has, err := db.GetByID[actions_model.ActionRun](ctx, runID)
|
||||
if err != nil || !has {
|
||||
ctx.APIErrorInternal(err)
|
||||
return
|
||||
}
|
||||
// Update known-changed fields on the run object in memory.
|
||||
// Note: the overall run status is updated asynchronously by the notifier,
|
||||
// so the status field may still reflect the pre-approval state.
|
||||
run.NeedApproval = false
|
||||
run.ApprovedBy = ctx.Doer.ID
|
||||
|
||||
convertedRun, err := convert.ToActionWorkflowRun(ctx, ctx.Repo.Repository, updatedRun, nil)
|
||||
convertedRun, err := convert.ToActionWorkflowRun(ctx, ctx.Repo.Repository, run, nil)
|
||||
if err != nil {
|
||||
ctx.APIErrorInternal(err)
|
||||
return
|
||||
|
||||
@ -199,15 +199,18 @@ func ListRuns(ctx *context.APIContext, ownerID, repoID int64) {
|
||||
return
|
||||
}
|
||||
|
||||
res.Entries = make([]*api.ActionWorkflowRun, len(runs))
|
||||
res.Entries = make([]*api.ActionWorkflowRun, 0, len(runs))
|
||||
for i := range runs {
|
||||
if runs[i].Repo == nil {
|
||||
continue // skip runs whose repository has been deleted
|
||||
}
|
||||
// TODO: load run attempts in batch
|
||||
convertedRun, err := convert.ToActionWorkflowRun(ctx, runs[i].Repo, runs[i], nil)
|
||||
if err != nil {
|
||||
ctx.APIErrorInternal(err)
|
||||
return
|
||||
}
|
||||
res.Entries[i] = convertedRun
|
||||
res.Entries = append(res.Entries, convertedRun)
|
||||
}
|
||||
ctx.SetLinkHeader(total, listOptions.PageSize)
|
||||
ctx.SetTotalCountHeader(total)
|
||||
|
||||
@ -15,6 +15,7 @@ import (
|
||||
repo_model "code.gitea.io/gitea/models/repo"
|
||||
"code.gitea.io/gitea/modules/actions"
|
||||
"code.gitea.io/gitea/modules/httplib"
|
||||
"code.gitea.io/gitea/modules/log"
|
||||
"code.gitea.io/gitea/modules/util"
|
||||
"code.gitea.io/gitea/services/context"
|
||||
)
|
||||
@ -61,11 +62,12 @@ func DownloadActionsRunAllJobLogs(ctx *context.Base, ctxRepo *repo_model.Reposit
|
||||
|
||||
// Add each job's logs to the zip
|
||||
for _, job := range runJobs {
|
||||
if job.TaskID == 0 {
|
||||
taskID := job.EffectiveTaskID()
|
||||
if taskID == 0 {
|
||||
continue // Skip jobs that haven't started
|
||||
}
|
||||
|
||||
task, err := actions_model.GetTaskByID(ctx, job.TaskID)
|
||||
task, err := actions_model.GetTaskByID(ctx, taskID)
|
||||
if err != nil {
|
||||
return fmt.Errorf("GetTaskByID for job %d: %w", job.ID, err)
|
||||
}
|
||||
@ -74,7 +76,7 @@ func DownloadActionsRunAllJobLogs(ctx *context.Base, ctxRepo *repo_model.Reposit
|
||||
continue
|
||||
}
|
||||
|
||||
// Create file in zip with job name and task ID; sanitize to prevent Zip Slip
|
||||
// Create file in zip with job name and task ID; sanitize job names for safe zip entry paths
|
||||
safeJobName := strings.NewReplacer("/", "-", `\`, "-", "..", "__").Replace(job.Name)
|
||||
fileName := fmt.Sprintf("%s-%s-%d.log", safeWorkflowName, safeJobName, task.ID)
|
||||
|
||||
@ -93,7 +95,8 @@ func DownloadActionsRunAllJobLogs(ctx *context.Base, ctxRepo *repo_model.Reposit
|
||||
_, err = io.Copy(zipFile, reader)
|
||||
return err
|
||||
}(); err != nil {
|
||||
return fmt.Errorf("job %d: %w", job.ID, err)
|
||||
log.Error("Failed to add logs for job %d to zip: %v", job.ID, err)
|
||||
continue
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -258,9 +258,8 @@ func ToActionTask(ctx context.Context, t *actions_model.ActionTask) (*api.Action
|
||||
}
|
||||
|
||||
func ToActionWorkflowRun(ctx context.Context, repo *repo_model.Repository, run *actions_model.ActionRun, attempt *actions_model.ActionRunAttempt) (*api.ActionWorkflowRun, error) {
|
||||
if run.Repo == nil {
|
||||
run.Repo = repo
|
||||
}
|
||||
// caller-provided repo is the single source of truth for URL construction
|
||||
run.Repo = repo
|
||||
if err := run.LoadTriggerUser(ctx); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
@ -15,10 +15,10 @@ import (
|
||||
actions_model "code.gitea.io/gitea/models/actions"
|
||||
auth_model "code.gitea.io/gitea/models/auth"
|
||||
"code.gitea.io/gitea/models/db"
|
||||
"code.gitea.io/gitea/models/perm"
|
||||
repo_model "code.gitea.io/gitea/models/repo"
|
||||
"code.gitea.io/gitea/models/unittest"
|
||||
user_model "code.gitea.io/gitea/models/user"
|
||||
"code.gitea.io/gitea/modules/json"
|
||||
api "code.gitea.io/gitea/modules/structs"
|
||||
"code.gitea.io/gitea/modules/timeutil"
|
||||
|
||||
@ -63,9 +63,7 @@ func TestAPIActionsGetWorkflowRun(t *testing.T) {
|
||||
AddTokenAuth(token)
|
||||
resp := MakeRequest(t, req, http.StatusOK)
|
||||
|
||||
var jobList api.ActionWorkflowJobsResponse
|
||||
err = json.Unmarshal(resp.Body.Bytes(), &jobList)
|
||||
require.NoError(t, err)
|
||||
jobList := DecodeJSON(t, resp, &api.ActionWorkflowJobsResponse{})
|
||||
|
||||
job198Idx := slices.IndexFunc(jobList.Entries, func(job *api.ActionWorkflowJob) bool { return job.ID == 198 })
|
||||
require.NotEqual(t, -1, job198Idx, "expected to find job 198 in run 795 jobs list")
|
||||
@ -144,9 +142,7 @@ func testAPIActionsDeleteRunListArtifacts(t *testing.T, repo *repo_model.Reposit
|
||||
req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/runs/795/artifacts", repo.FullName())).
|
||||
AddTokenAuth(token)
|
||||
resp := MakeRequest(t, req, http.StatusOK)
|
||||
var listResp api.ActionArtifactsResponse
|
||||
err := json.Unmarshal(resp.Body.Bytes(), &listResp)
|
||||
assert.NoError(t, err)
|
||||
listResp := DecodeJSON(t, resp, &api.ActionArtifactsResponse{})
|
||||
assert.Len(t, listResp.Entries, artifacts)
|
||||
}
|
||||
|
||||
@ -154,9 +150,7 @@ func testAPIActionsDeleteRunListTasks(t *testing.T, repo *repo_model.Repository,
|
||||
req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/tasks", repo.FullName())).
|
||||
AddTokenAuth(token)
|
||||
resp := MakeRequest(t, req, http.StatusOK)
|
||||
var listResp api.ActionTaskResponse
|
||||
err := json.Unmarshal(resp.Body.Bytes(), &listResp)
|
||||
assert.NoError(t, err)
|
||||
listResp := DecodeJSON(t, resp, &api.ActionTaskResponse{})
|
||||
findTask1 := false
|
||||
findTask2 := false
|
||||
for _, entry := range listResp.Entries {
|
||||
@ -199,9 +193,7 @@ func TestAPIActionsRerunWorkflowRun(t *testing.T) {
|
||||
AddTokenAuth(writeToken)
|
||||
resp := MakeRequest(t, req, http.StatusCreated)
|
||||
|
||||
var rerunResp api.ActionWorkflowRun
|
||||
err := json.Unmarshal(resp.Body.Bytes(), &rerunResp)
|
||||
require.NoError(t, err)
|
||||
rerunResp := DecodeJSON(t, resp, &api.ActionWorkflowRun{})
|
||||
assert.Equal(t, int64(795), rerunResp.ID)
|
||||
assert.Equal(t, "queued", rerunResp.Status)
|
||||
assert.Equal(t, "c2d72f548424103f01ee1dc02889c1e2bff816b0", rerunResp.HeadSha)
|
||||
@ -250,7 +242,11 @@ func TestAPIActionsCancelWorkflowRun(t *testing.T) {
|
||||
t.Run("Success", func(t *testing.T) {
|
||||
req := NewRequest(t, "POST", fmt.Sprintf("/api/v1/repos/%s/actions/runs/793/cancel", repo.FullName())).
|
||||
AddTokenAuth(ownerToken)
|
||||
MakeRequest(t, req, http.StatusOK)
|
||||
resp := MakeRequest(t, req, http.StatusOK)
|
||||
cancelledRun := DecodeJSON(t, resp, &api.ActionWorkflowRun{})
|
||||
assert.Equal(t, int64(793), cancelledRun.ID)
|
||||
assert.Equal(t, "completed", cancelledRun.Status)
|
||||
assert.Equal(t, "cancelled", cancelledRun.Conclusion)
|
||||
})
|
||||
|
||||
t.Run("NotFound", func(t *testing.T) {
|
||||
@ -357,20 +353,32 @@ jobs:
|
||||
assert.Equal(t, actions_model.StatusWaiting, job.Status)
|
||||
}
|
||||
|
||||
// Test approve already approved run (idempotency)
|
||||
// Test approve already approved run (idempotency — returns 200 like GitHub)
|
||||
req = NewRequest(t, "POST", fmt.Sprintf("/api/v1/repos/%s/actions/runs/%d/approve", baseRepo.FullName(), run.ID)).
|
||||
AddTokenAuth(user2Token)
|
||||
MakeRequest(t, req, http.StatusBadRequest)
|
||||
resp = MakeRequest(t, req, http.StatusOK)
|
||||
idempotentRun := DecodeJSON(t, resp, &api.ActionWorkflowRun{})
|
||||
assert.NotEqual(t, "waiting", idempotentRun.Status, "already-approved run should not be blocked")
|
||||
|
||||
// Test approve non-existent run
|
||||
req = NewRequest(t, "POST", fmt.Sprintf("/api/v1/repos/%s/actions/runs/999999/approve", baseRepo.FullName())).
|
||||
AddTokenAuth(user2Token)
|
||||
MakeRequest(t, req, http.StatusNotFound)
|
||||
|
||||
// Test approve by non-owner (user4 should get forbidden)
|
||||
// Test approve by non-owner (user4 should get forbidden before being added as collaborator)
|
||||
req = NewRequest(t, "POST", fmt.Sprintf("/api/v1/repos/%s/actions/runs/%d/approve", baseRepo.FullName(), run.ID)).
|
||||
AddTokenAuth(user4Token)
|
||||
MakeRequest(t, req, http.StatusForbidden)
|
||||
|
||||
// Add user4 as a collaborator with write access
|
||||
doAPIAddCollaborator(user2APICtx, user4.Name, perm.AccessModeWrite)(t)
|
||||
|
||||
// Test approve by writer-but-non-admin (user4 should now succeed)
|
||||
req = NewRequest(t, "POST", fmt.Sprintf("/api/v1/repos/%s/actions/runs/%d/approve", baseRepo.FullName(), run.ID)).
|
||||
AddTokenAuth(user4Token)
|
||||
resp = MakeRequest(t, req, http.StatusOK)
|
||||
approvedRun := DecodeJSON(t, resp, &api.ActionWorkflowRun{})
|
||||
assert.NotEqual(t, "waiting", approvedRun.Status, "approved run should not be blocked")
|
||||
})
|
||||
}
|
||||
|
||||
@ -400,9 +408,7 @@ func TestAPIActionsRerunWorkflowJob(t *testing.T) {
|
||||
AddTokenAuth(writeToken)
|
||||
resp := MakeRequest(t, req, http.StatusCreated)
|
||||
|
||||
var rerunResp api.ActionWorkflowJob
|
||||
err := json.Unmarshal(resp.Body.Bytes(), &rerunResp)
|
||||
require.NoError(t, err)
|
||||
rerunResp := DecodeJSON(t, resp, &api.ActionWorkflowJob{})
|
||||
job199Rerun := getLatestAttemptJobByTemplateJobID(t, 795, 199)
|
||||
assert.Equal(t, job199Rerun.ID, rerunResp.ID)
|
||||
assert.Equal(t, "queued", rerunResp.Status)
|
||||
@ -450,9 +456,7 @@ func TestAPIActionsListUserWorkflows(t *testing.T) {
|
||||
t.Run("Runs", func(t *testing.T) {
|
||||
req := NewRequest(t, "GET", "/api/v1/user/actions/runs").AddTokenAuth(token)
|
||||
resp := MakeRequest(t, req, http.StatusOK)
|
||||
var runs api.ActionWorkflowRunsResponse
|
||||
err := json.Unmarshal(resp.Body.Bytes(), &runs)
|
||||
require.NoError(t, err)
|
||||
runs := DecodeJSON(t, resp, &api.ActionWorkflowRunsResponse{})
|
||||
|
||||
assert.Positive(t, runs.TotalCount)
|
||||
assert.NotEmpty(t, runs.Entries)
|
||||
@ -468,9 +472,7 @@ func TestAPIActionsListUserWorkflows(t *testing.T) {
|
||||
t.Run("Jobs", func(t *testing.T) {
|
||||
req := NewRequest(t, "GET", "/api/v1/user/actions/jobs").AddTokenAuth(token)
|
||||
resp := MakeRequest(t, req, http.StatusOK)
|
||||
var jobs api.ActionWorkflowJobsResponse
|
||||
err := json.Unmarshal(resp.Body.Bytes(), &jobs)
|
||||
require.NoError(t, err)
|
||||
jobs := DecodeJSON(t, resp, &api.ActionWorkflowJobsResponse{})
|
||||
|
||||
assert.Positive(t, jobs.TotalCount)
|
||||
assert.NotEmpty(t, jobs.Entries)
|
||||
@ -492,9 +494,7 @@ func TestAPIActionsListRepoWorkflows(t *testing.T) {
|
||||
|
||||
req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/runs", repo.FullName())).AddTokenAuth(token)
|
||||
resp := MakeRequest(t, req, http.StatusOK)
|
||||
var runs api.ActionWorkflowRunsResponse
|
||||
err := json.Unmarshal(resp.Body.Bytes(), &runs)
|
||||
require.NoError(t, err)
|
||||
runs := DecodeJSON(t, resp, &api.ActionWorkflowRunsResponse{})
|
||||
|
||||
assert.Positive(t, runs.TotalCount)
|
||||
assert.NotEmpty(t, runs.Entries)
|
||||
@ -517,7 +517,28 @@ func TestAPIActionsGetWorkflowRunLogs(t *testing.T) {
|
||||
t.Run("Success", func(t *testing.T) {
|
||||
req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/runs/795/logs", repo.FullName())).
|
||||
AddTokenAuth(token)
|
||||
MakeRequest(t, req, http.StatusOK)
|
||||
resp := MakeRequest(t, req, http.StatusOK)
|
||||
assert.Equal(t, "application/zip", resp.Header().Get("Content-Type"))
|
||||
assert.Contains(t, resp.Header().Get("Content-Disposition"), "attachment")
|
||||
assert.Contains(t, resp.Header().Get("Content-Disposition"), ".zip")
|
||||
body := resp.Body.Bytes()
|
||||
require.NotEmpty(t, body)
|
||||
assert.Equal(t, "PK", string(body[:2]), "response should be a valid zip file")
|
||||
})
|
||||
|
||||
t.Run("RerunJobLogs", func(t *testing.T) {
|
||||
// Rerun the workflow so latest-attempt jobs have SourceTaskID instead of TaskID
|
||||
req := NewRequest(t, "POST", fmt.Sprintf("/api/v1/repos/%s/actions/runs/795/rerun", repo.FullName())).
|
||||
AddTokenAuth(token)
|
||||
MakeRequest(t, req, http.StatusCreated)
|
||||
|
||||
// Download logs for the latest attempt — should include rerun job logs via EffectiveTaskID
|
||||
req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/runs/795/logs", repo.FullName())).
|
||||
AddTokenAuth(token)
|
||||
resp := MakeRequest(t, req, http.StatusOK)
|
||||
body := resp.Body.Bytes()
|
||||
require.NotEmpty(t, body)
|
||||
assert.Equal(t, "PK", string(body[:2]), "response should be a valid zip file")
|
||||
})
|
||||
|
||||
t.Run("NotFound", func(t *testing.T) {
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user