Skip to content

Commit f2cf2e6

Browse files
rossigeerossigee
committed
Address PR feedback: use run ID instead of run index and remove duplicate endpoint
Based on reviewer feedback, this commit makes the following changes: ## API Consistency Improvements - Replace getRunIndex() with getRunID() to align with existing Gitea API patterns - Update all endpoints to use run ID directly instead of run index - Follow the same pattern as GetWorkflowRun(): runID := ctx.PathParamInt64("run") and db.GetByID[actions_model.ActionRun](ctx, runID) - Update swagger documentation to reflect "run ID" instead of "run number" ## Remove Duplicate Endpoint - Remove GetWorkflowJobLogs function and corresponding route /actions/runs/{run}/jobs/{job_id}/logs - This eliminates duplication with existing /actions/jobs/{job_id}/logs endpoint - Remove corresponding test TestAPIActionsGetWorkflowJobLogs ## Functions Updated - RerunWorkflowRun: now uses run ID consistently - CancelWorkflowRun: now uses run ID consistently - ApproveWorkflowRun: now uses run ID consistently - RerunWorkflowJob: now uses run ID consistently - GetWorkflowRunLogs: now uses run ID consistently - GetWorkflowRunLogsStream: now uses run ID consistently ## Helper Functions Refactored - getRunIndex() → getRunID() with proper error handling - getRunJobsByIndex() → getRunJobsByRunID() with proper validation - getRunJobsAndCurrent() updated to use run ID parameter All existing tests continue to pass as they were already using run IDs correctly.
1 parent 69e35f7 commit f2cf2e6

File tree

3 files changed

+70
-134
lines changed

3 files changed

+70
-134
lines changed

‎routers/api/v1/api.go‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1293,10 +1293,7 @@ func Routes() *web.Router {
12931293
m.Post("/cancel", reqToken(), reqRepoWriter(unit.TypeActions), repo.CancelWorkflowRun)
12941294
m.Post("/approve", reqToken(), reqRepoWriter(unit.TypeActions), repo.ApproveWorkflowRun)
12951295
m.Get("/jobs", repo.ListWorkflowRunJobs)
1296-
m.Group("/jobs", func() {
1297-
m.Post("/{job_id}/rerun", reqToken(), reqRepoWriter(unit.TypeActions), repo.RerunWorkflowJob)
1298-
m.Get("/{job_id}/logs", repo.GetWorkflowJobLogs)
1299-
})
1296+
m.Post("/jobs/{job_id}/rerun", reqToken(), reqRepoWriter(unit.TypeActions), repo.RerunWorkflowJob)
13001297
m.Get("/logs", repo.GetWorkflowRunLogs)
13011298
m.Post("/logs", reqToken(), reqRepoReader(unit.TypeActions), repo.GetWorkflowRunLogsStream)
13021299
m.Get("/artifacts", repo.GetArtifactsOfRun)

‎routers/api/v1/repo/actions_run.go‎

Lines changed: 69 additions & 111 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ func RerunWorkflowRun(ctx *context.APIContext) {
9393
// required: true
9494
// - name: run
9595
// in: path
96-
// description: run number or "latest"
96+
// description: run ID or "latest"
9797
// type: string
9898
// required: true
9999
// responses:
@@ -111,13 +111,7 @@ func RerunWorkflowRun(ctx *context.APIContext) {
111111
return
112112
}
113113

114-
runIndex := getRunIndex(ctx)
115-
if runIndex == 0 {
116-
ctx.APIError(404, "Invalid run number")
117-
return
118-
}
119-
120-
run, err := actions_model.GetRunByIndex(ctx, ctx.Repo.Repository.ID, runIndex)
114+
_, run, err := getRunID(ctx)
121115
if err != nil {
122116
if errors.Is(err, util.ErrNotExist) {
123117
ctx.APIError(404, "Run not found")
@@ -184,7 +178,7 @@ func CancelWorkflowRun(ctx *context.APIContext) {
184178
// required: true
185179
// - name: run
186180
// in: path
187-
// description: run number or "latest"
181+
// description: run ID or "latest"
188182
// type: string
189183
// required: true
190184
// responses:
@@ -202,13 +196,17 @@ func CancelWorkflowRun(ctx *context.APIContext) {
202196
return
203197
}
204198

205-
runIndex := getRunIndex(ctx)
206-
if runIndex == 0 {
207-
ctx.APIError(404, "Invalid run number")
199+
runID, _, err := getRunID(ctx)
200+
if err != nil {
201+
if errors.Is(err, util.ErrNotExist) {
202+
ctx.APIError(404, "Run not found")
203+
} else {
204+
ctx.APIErrorInternal(err)
205+
}
208206
return
209207
}
210208

211-
jobs, err := getRunJobsByIndex(ctx, runIndex)
209+
jobs, err := getRunJobsByRunID(ctx, runID)
212210
if err != nil {
213211
ctx.APIErrorInternal(err)
214212
return
@@ -281,7 +279,7 @@ func ApproveWorkflowRun(ctx *context.APIContext) {
281279
// required: true
282280
// - name: run
283281
// in: path
284-
// description: run number or "latest"
282+
// description: run ID or "latest"
285283
// type: string
286284
// required: true
287285
// responses:
@@ -299,13 +297,17 @@ func ApproveWorkflowRun(ctx *context.APIContext) {
299297
return
300298
}
301299

302-
runIndex := getRunIndex(ctx)
303-
if runIndex == 0 {
304-
ctx.APIError(404, "Invalid run number")
300+
runID, _, err := getRunID(ctx)
301+
if err != nil {
302+
if errors.Is(err, util.ErrNotExist) {
303+
ctx.APIError(404, "Run not found")
304+
} else {
305+
ctx.APIErrorInternal(err)
306+
}
305307
return
306308
}
307309

308-
current, jobs, err := getRunJobsAndCurrent(ctx, runIndex, -1)
310+
current, jobs, err := getRunJobsAndCurrent(ctx, runID, -1)
309311
if err != nil {
310312
ctx.APIErrorInternal(err)
311313
return
@@ -375,7 +377,7 @@ func RerunWorkflowJob(ctx *context.APIContext) {
375377
// required: true
376378
// - name: run
377379
// in: path
378-
// description: run number or "latest"
380+
// description: run ID or "latest"
379381
// type: string
380382
// required: true
381383
// - name: job_id
@@ -398,16 +400,20 @@ func RerunWorkflowJob(ctx *context.APIContext) {
398400
return
399401
}
400402

401-
runIndex := getRunIndex(ctx)
402-
if runIndex == 0 {
403-
ctx.APIError(404, "Invalid run number")
403+
runID, _, err := getRunID(ctx)
404+
if err != nil {
405+
if errors.Is(err, util.ErrNotExist) {
406+
ctx.APIError(404, "Run not found")
407+
} else {
408+
ctx.APIErrorInternal(err)
409+
}
404410
return
405411
}
406412

407413
jobID := ctx.PathParamInt64("job_id")
408414

409415
// Get all jobs for the run to handle dependencies
410-
allJobs, err := getRunJobsByIndex(ctx, runIndex)
416+
allJobs, err := getRunJobsByRunID(ctx, runID)
411417
if err != nil {
412418
ctx.APIErrorInternal(err)
413419
return
@@ -427,12 +433,8 @@ func RerunWorkflowJob(ctx *context.APIContext) {
427433
return
428434
}
429435

430-
// Get run and check if workflow is disabled
431-
run, err := actions_model.GetRunByIndex(ctx, ctx.Repo.Repository.ID, runIndex)
432-
if err != nil {
433-
ctx.APIErrorInternal(err)
434-
return
435-
}
436+
// Get run from the job and check if workflow is disabled
437+
run := allJobs[0].Run
436438

437439
cfgUnit := ctx.Repo.Repository.MustGetUnit(ctx, unit.TypeActions)
438440
cfg := cfgUnit.ActionsConfig()
@@ -468,21 +470,39 @@ func RerunWorkflowJob(ctx *context.APIContext) {
468470
}
469471

470472
// Helper functions
471-
func getRunIndex(ctx *context.APIContext) int64 {
472-
// if run param is "latest", get the latest run index
473+
func getRunID(ctx *context.APIContext) (int64, *actions_model.ActionRun, error) {
474+
// if run param is "latest", get the latest run
473475
if ctx.PathParam("run") == "latest" {
474-
if run, _ := actions_model.GetLatestRun(ctx, ctx.Repo.Repository.ID); run != nil {
475-
return run.Index
476+
run, err := actions_model.GetLatestRun(ctx, ctx.Repo.Repository.ID)
477+
if err != nil {
478+
return 0, nil, err
476479
}
480+
if run == nil {
481+
return 0, nil, util.ErrNotExist
482+
}
483+
return run.ID, run, nil
477484
}
478-
return ctx.PathParamInt64("run")
485+
486+
// Otherwise get run by ID
487+
runID := ctx.PathParamInt64("run")
488+
run, has, err := db.GetByID[actions_model.ActionRun](ctx, runID)
489+
if err != nil {
490+
return 0, nil, err
491+
}
492+
if !has || run.RepoID != ctx.Repo.Repository.ID {
493+
return 0, nil, util.ErrNotExist
494+
}
495+
return runID, run, nil
479496
}
480497

481-
func getRunJobsByIndex(ctx *context.APIContext, runIndex int64) ([]*actions_model.ActionRunJob, error) {
482-
run, err := actions_model.GetRunByIndex(ctx, ctx.Repo.Repository.ID, runIndex)
498+
func getRunJobsByRunID(ctx *context.APIContext, runID int64) ([]*actions_model.ActionRunJob, error) {
499+
run, has, err := db.GetByID[actions_model.ActionRun](ctx, runID)
483500
if err != nil {
484501
return nil, err
485502
}
503+
if !has || run.RepoID != ctx.Repo.Repository.ID {
504+
return nil, util.ErrNotExist
505+
}
486506
run.Repo = ctx.Repo.Repository
487507
jobs, err := actions_model.GetRunJobsByRunID(ctx, run.ID)
488508
if err != nil {
@@ -494,8 +514,8 @@ func getRunJobsByIndex(ctx *context.APIContext, runIndex int64) ([]*actions_mode
494514
return jobs, nil
495515
}
496516

497-
func getRunJobsAndCurrent(ctx *context.APIContext, runIndex, jobIndex int64) (*actions_model.ActionRunJob, []*actions_model.ActionRunJob, error) {
498-
jobs, err := getRunJobsByIndex(ctx, runIndex)
517+
func getRunJobsAndCurrent(ctx *context.APIContext, runID, jobIndex int64) (*actions_model.ActionRunJob, []*actions_model.ActionRunJob, error) {
518+
jobs, err := getRunJobsByRunID(ctx, runID)
499519
if err != nil {
500520
return nil, nil, err
501521
}
@@ -588,7 +608,7 @@ func GetWorkflowRunLogs(ctx *context.APIContext) {
588608
// required: true
589609
// - name: run
590610
// in: path
591-
// description: run number or "latest"
611+
// description: run ID or "latest"
592612
// type: string
593613
// required: true
594614
// responses:
@@ -597,13 +617,7 @@ func GetWorkflowRunLogs(ctx *context.APIContext) {
597617
// "404":
598618
// "$ref": "#/responses/notFound"
599619

600-
runIndex := getRunIndex(ctx)
601-
if runIndex == 0 {
602-
ctx.APIError(404, "Invalid run number")
603-
return
604-
}
605-
606-
run, err := actions_model.GetRunByIndex(ctx, ctx.Repo.Repository.ID, runIndex)
620+
_, run, err := getRunID(ctx)
607621
if err != nil {
608622
if errors.Is(err, util.ErrNotExist) {
609623
ctx.APIError(404, "Run not found")
@@ -622,66 +636,6 @@ func GetWorkflowRunLogs(ctx *context.APIContext) {
622636
}
623637
}
624638

625-
func GetWorkflowJobLogs(ctx *context.APIContext) {
626-
// swagger:operation GET /repos/{owner}/{repo}/actions/runs/{run}/jobs/{job_id}/logs repository getWorkflowJobLogs
627-
// ---
628-
// summary: Download job logs
629-
// produces:
630-
// - application/zip
631-
// parameters:
632-
// - name: owner
633-
// in: path
634-
// description: owner of the repo
635-
// type: string
636-
// required: true
637-
// - name: repo
638-
// in: path
639-
// description: name of the repository
640-
// type: string
641-
// required: true
642-
// - name: run
643-
// in: path
644-
// description: run number or "latest"
645-
// type: string
646-
// required: true
647-
// - name: job_id
648-
// in: path
649-
// description: id of the job
650-
// type: integer
651-
// required: true
652-
// responses:
653-
// "200":
654-
// description: Job logs
655-
// "404":
656-
// "$ref": "#/responses/notFound"
657-
658-
runIndex := getRunIndex(ctx)
659-
if runIndex == 0 {
660-
ctx.APIError(404, "Invalid run number")
661-
return
662-
}
663-
664-
jobID := ctx.PathParamInt64("job_id")
665-
666-
run, err := actions_model.GetRunByIndex(ctx, ctx.Repo.Repository.ID, runIndex)
667-
if err != nil {
668-
if errors.Is(err, util.ErrNotExist) {
669-
ctx.APIError(404, "Run not found")
670-
} else {
671-
ctx.APIErrorInternal(err)
672-
}
673-
return
674-
}
675-
676-
if err = common.DownloadActionsRunJobLogsWithIndex(ctx.Base, ctx.Repo.Repository, run.ID, jobID); err != nil {
677-
if errors.Is(err, util.ErrNotExist) {
678-
ctx.APIError(404, "Job logs not found")
679-
} else {
680-
ctx.APIErrorInternal(err)
681-
}
682-
}
683-
}
684-
685639
func GetWorkflowRunLogsStream(ctx *context.APIContext) {
686640
// swagger:operation POST /repos/{owner}/{repo}/actions/runs/{run}/logs repository getWorkflowRunLogsStream
687641
// ---
@@ -703,7 +657,7 @@ func GetWorkflowRunLogsStream(ctx *context.APIContext) {
703657
// required: true
704658
// - name: run
705659
// in: path
706-
// description: run number or "latest"
660+
// description: run ID or "latest"
707661
// type: string
708662
// required: true
709663
// - name: job
@@ -758,9 +712,13 @@ func GetWorkflowRunLogsStream(ctx *context.APIContext) {
758712
// "404":
759713
// "$ref": "#/responses/notFound"
760714

761-
runIndex := getRunIndex(ctx)
762-
if runIndex == 0 {
763-
ctx.APIError(404, "Invalid run number")
715+
runID, _, err := getRunID(ctx)
716+
if err != nil {
717+
if errors.Is(err, util.ErrNotExist) {
718+
ctx.APIError(404, "Run not found")
719+
} else {
720+
ctx.APIErrorInternal(err)
721+
}
764722
return
765723
}
766724

@@ -776,7 +734,7 @@ func GetWorkflowRunLogsStream(ctx *context.APIContext) {
776734
req = LogRequest{LogCursors: []LogCursor{}}
777735
}
778736

779-
current, _, err := getRunJobsAndCurrent(ctx, runIndex, jobIndex)
737+
current, _, err := getRunJobsAndCurrent(ctx, runID, jobIndex)
780738
if err != nil {
781739
if errors.Is(err, util.ErrNotExist) {
782740
ctx.APIError(404, "Run or job not found")

‎tests/integration/api_actions_run_test.go‎

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -264,25 +264,6 @@ func TestAPIActionsGetWorkflowRunLogs(t *testing.T) {
264264
MakeRequest(t, req, http.StatusNotFound)
265265
}
266266

267-
func TestAPIActionsGetWorkflowJobLogs(t *testing.T) {
268-
defer prepareTestEnvActionsArtifacts(t)()
269-
270-
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2})
271-
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
272-
session := loginUser(t, user.Name)
273-
token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
274-
275-
// Test get job logs
276-
req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/runs/795/jobs/192/logs", repo.FullName())).
277-
AddTokenAuth(token)
278-
MakeRequest(t, req, http.StatusOK)
279-
280-
// Test get non-existent job logs
281-
req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/runs/795/jobs/999999/logs", repo.FullName())).
282-
AddTokenAuth(token)
283-
MakeRequest(t, req, http.StatusNotFound)
284-
}
285-
286267
func TestAPIActionsGetWorkflowRunLogsStream(t *testing.T) {
287268
defer prepareTestEnvActionsArtifacts(t)()
288269

0 commit comments

Comments
 (0)