mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2026-05-28 11:14:54 -04:00
fix: do not hide previous attempts without task for latest attempt (#12779)
If a Forgejo Actions job was run more than once, Forgejo would not display previous attempts if no `ActionTask` existed for the latest attempt. That is the case when a job is cancelled or skipped before having been dispatched to a runner or while it is waiting for a runner. This is fixed by always loading all existing attempts. Resolves #12626. ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests for Go changes - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I ran... - [x] `make pr-go` before pushing ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [x] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change. - [ ] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change. *The decision if the pull request will be shown in the release notes is up to the mergers / release team.* The content of the `release-notes/<pull request number>.md` file will serve as the basis for the release notes. If the file does not exist, the title of the pull request will be used instead. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12779 Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org> Reviewed-by: 0ko <0ko@noreply.codeberg.org>
This commit is contained in:
Andreas Ahlenstorf
Mathieu Fenniak
parent
81a5b0d840
commit
fa5a2501d0
9 changed files with 84 additions and 67 deletions
|
|
@ -153,6 +153,21 @@ func (job *ActionRunJob) CanBeRerun(ctx context.Context) (bool, error) {
|
|||
return job.Status.IsDone(), nil
|
||||
}
|
||||
|
||||
// GetAllAttempts retrieve all the attempts of this job. Limited fields are queried to avoid loading the LogIndexes blob
|
||||
// when not needed.
|
||||
func (job *ActionRunJob) GetAllAttempts(ctx context.Context) ([]*ActionTask, error) {
|
||||
var attempts []*ActionTask
|
||||
err := db.GetEngine(ctx).
|
||||
Cols("id", "attempt", "status", "started").
|
||||
Where("job_id=?", job.ID).
|
||||
Desc("attempt").
|
||||
Find(&attempts)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return attempts, nil
|
||||
}
|
||||
|
||||
func GetRunJobByID(ctx context.Context, id int64) (*ActionRunJob, error) {
|
||||
var job ActionRunJob
|
||||
has, err := db.GetEngine(ctx).Where("id=?", id).Get(&job)
|
||||
|
|
|
|||
|
|
@ -45,7 +45,7 @@ func TestActionRunJob_HTMLURL(t *testing.T) {
|
|||
}{
|
||||
{
|
||||
id: 192,
|
||||
expected: "https://try.gitea.io/user5/repo4/actions/runs/187/jobs/0/attempt/1",
|
||||
expected: "https://try.gitea.io/user5/repo4/actions/runs/187/jobs/0/attempt/3",
|
||||
},
|
||||
{
|
||||
id: 393,
|
||||
|
|
@ -502,3 +502,22 @@ func TestActionRunJob_CanBeRerun(t *testing.T) {
|
|||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestActionTask_GetAllAttempts(t *testing.T) {
|
||||
require.NoError(t, unittest.PrepareTestDatabase())
|
||||
|
||||
job2 := unittest.AssertExistsAndLoadBean(t, &ActionRunJob{ID: 192})
|
||||
|
||||
allAttempts, err := job2.GetAllAttempts(t.Context())
|
||||
require.NoError(t, err)
|
||||
|
||||
require.Len(t, allAttempts, 3)
|
||||
assert.EqualValues(t, 47, allAttempts[0].ID, "ordered by attempt, 1")
|
||||
assert.EqualValues(t, 53, allAttempts[1].ID, "ordered by attempt, 2")
|
||||
assert.EqualValues(t, 52, allAttempts[2].ID, "ordered by attempt, 3")
|
||||
|
||||
// GetAllAttempts doesn't populate all fields; so check expected fields from one of the records
|
||||
assert.EqualValues(t, 3, allAttempts[0].Attempt, "read Attempt field")
|
||||
assert.Equal(t, StatusRunning, allAttempts[0].Status, "read Status field")
|
||||
assert.Equal(t, timeutil.TimeStamp(1683636528), allAttempts[0].Started, "read Started field")
|
||||
}
|
||||
|
|
|
|||
|
|
@ -161,21 +161,6 @@ func (task *ActionTask) UpdateToken(ctx context.Context) error {
|
|||
return UpdateTask(ctx, task, "token_hash", "token_salt", "token_last_eight")
|
||||
}
|
||||
|
||||
// Retrieve all the attempts from the same job as the target `ActionTask`. Limited fields are queried to avoid loading
|
||||
// the LogIndexes blob when not needed.
|
||||
func (task *ActionTask) GetAllAttempts(ctx context.Context) ([]*ActionTask, error) {
|
||||
var attempts []*ActionTask
|
||||
err := db.GetEngine(ctx).
|
||||
Cols("id", "attempt", "status", "started").
|
||||
Where("job_id=?", task.JobID).
|
||||
Desc("attempt").
|
||||
Find(&attempts)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return attempts, nil
|
||||
}
|
||||
|
||||
func GetTaskByID(ctx context.Context, id int64) (*ActionTask, error) {
|
||||
var task ActionTask
|
||||
has, err := db.GetEngine(ctx).Where("id=?", id).Get(&task)
|
||||
|
|
|
|||
|
|
@ -6,35 +6,12 @@ package actions
|
|||
import (
|
||||
"testing"
|
||||
|
||||
"forgejo.org/models/db"
|
||||
"forgejo.org/models/unittest"
|
||||
"forgejo.org/modules/timeutil"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func TestActionTask_GetAllAttempts(t *testing.T) {
|
||||
require.NoError(t, unittest.PrepareTestDatabase())
|
||||
|
||||
var task ActionTask
|
||||
has, err := db.GetEngine(t.Context()).Where("id=?", 47).Get(&task)
|
||||
require.NoError(t, err)
|
||||
require.True(t, has, "load ActionTask from fixture")
|
||||
|
||||
allAttempts, err := task.GetAllAttempts(t.Context())
|
||||
require.NoError(t, err)
|
||||
require.Len(t, allAttempts, 3)
|
||||
assert.EqualValues(t, 47, allAttempts[0].ID, "ordered by attempt, 1")
|
||||
assert.EqualValues(t, 53, allAttempts[1].ID, "ordered by attempt, 2")
|
||||
assert.EqualValues(t, 52, allAttempts[2].ID, "ordered by attempt, 3")
|
||||
|
||||
// GetAllAttempts doesn't populate all fields; so check expected fields from one of the records
|
||||
assert.EqualValues(t, 3, allAttempts[0].Attempt, "read Attempt field")
|
||||
assert.Equal(t, StatusRunning, allAttempts[0].Status, "read Status field")
|
||||
assert.Equal(t, timeutil.TimeStamp(1683636528), allAttempts[0].Started, "read Started field")
|
||||
}
|
||||
|
||||
func TestActionTask_GetTaskByJobAttempt(t *testing.T) {
|
||||
require.NoError(t, unittest.PrepareTestDatabase())
|
||||
|
||||
|
|
@ -58,7 +35,7 @@ func TestActionTask_CreatePlaceholderTask(t *testing.T) {
|
|||
|
||||
assert.NotEqualValues(t, 0, task.ID)
|
||||
assert.Equal(t, job.ID, task.JobID)
|
||||
assert.EqualValues(t, 0, task.Attempt)
|
||||
assert.EqualValues(t, 1, task.Attempt)
|
||||
assert.NotEqualValues(t, 0, task.Started)
|
||||
assert.NotEqualValues(t, 0, task.Stopped)
|
||||
assert.Equal(t, job.Status, task.Status)
|
||||
|
|
|
|||
|
|
@ -6,7 +6,7 @@
|
|||
commit_sha: c2d72f548424103f01ee1dc02889c1e2bff816b0
|
||||
is_fork_pull_request: false
|
||||
name: job_2
|
||||
attempt: 1
|
||||
attempt: 3
|
||||
job_id: job_2
|
||||
task_id: 47
|
||||
status: 1
|
||||
|
|
@ -199,7 +199,7 @@
|
|||
commit_sha: 985f0301dba5e7b34be866819cd15ad3d8f508ee
|
||||
is_fork_pull_request: false
|
||||
name: job_2
|
||||
attempt: 0
|
||||
attempt: 1
|
||||
job_id: job_2
|
||||
task_id: null
|
||||
status: 5
|
||||
|
|
|
|||
|
|
@ -857,6 +857,7 @@
|
|||
"actions.runs.delete.button": "Delete run",
|
||||
"actions.runs.delete.error": "Could not delete the workflow run.",
|
||||
"actions.runs.delete.confirm_action": "Do you really want to delete this workflow run?",
|
||||
"actions.jobs.not_started": "not started",
|
||||
"actions.workflow.job_parsing_error": "Unable to parse jobs in workflow: %v",
|
||||
"actions.workflow.event_detection_error": "Unable to parse supported events in workflow: %v",
|
||||
"actions.workflow.persistent_incomplete_matrix": "Unable to evaluate `strategy.matrix` of job %[1]s due to a `needs` expression that was invalid. It may reference a job that is not in it's 'needs' list (%[2]s), or an output that doesn't exist on one of those jobs.",
|
||||
|
|
|
|||
|
|
@ -349,6 +349,39 @@ func getViewResponse(ctx *app_context.Context, req *ViewRequest, runIndex, jobIn
|
|||
Branch: branch,
|
||||
}
|
||||
|
||||
taskAttempts, err := current.GetAllAttempts(ctx)
|
||||
if err != nil {
|
||||
ctx.Error(http.StatusInternalServerError, err.Error())
|
||||
return nil
|
||||
}
|
||||
|
||||
var allAttempts []*TaskAttempt
|
||||
// If the latest attempt has not been taken up yet by a runner, there is no task that could be displayed. As a
|
||||
// stopgap, inject a phantom task that provides the necessary information until a real tasks is created, if ever.
|
||||
if len(taskAttempts) == 0 || taskAttempts[0].Attempt != current.Attempt {
|
||||
taskAttempt := &TaskAttempt{
|
||||
Number: current.Attempt,
|
||||
Status: current.Status.String(),
|
||||
Started: template.HTML(ctx.Locale.TrString("actions.jobs.not_started")),
|
||||
StatusDiagnostics: statusDiagnostics(current.Status, current, ctx.Locale),
|
||||
}
|
||||
allAttempts = append(allAttempts, taskAttempt)
|
||||
}
|
||||
for _, actionTask := range taskAttempts {
|
||||
taskAttempt := &TaskAttempt{
|
||||
Number: actionTask.Attempt,
|
||||
Started: templates.TimeSince(actionTask.Started),
|
||||
Status: actionTask.Status.String(),
|
||||
StatusDiagnostics: statusDiagnostics(actionTask.Status, current, ctx.Locale),
|
||||
}
|
||||
allAttempts = append(allAttempts, taskAttempt)
|
||||
}
|
||||
|
||||
resp.State.CurrentJob.Title = current.Name
|
||||
resp.State.CurrentJob.Details = statusDiagnostics(current.Status, current, ctx.Locale)
|
||||
resp.State.CurrentJob.Steps = make([]*ViewJobStep, 0) // marshal to '[]' instead of 'null' in json
|
||||
resp.State.CurrentJob.AllAttempts = allAttempts
|
||||
|
||||
var task *actions_model.ActionTask
|
||||
// TaskID will be set only when the ActionRunJob has been picked by a runner, resulting in an ActionTask being
|
||||
// created representing the specific task. If current.TaskID is not set, then the user is attempting to view a job
|
||||
|
|
@ -369,29 +402,9 @@ func getViewResponse(ctx *app_context.Context, req *ViewRequest, runIndex, jobIn
|
|||
}
|
||||
}
|
||||
|
||||
resp.State.CurrentJob.Title = current.Name
|
||||
resp.State.CurrentJob.Details = statusDiagnostics(current.Status, current, ctx.Locale)
|
||||
|
||||
resp.State.CurrentJob.Steps = make([]*ViewJobStep, 0) // marshal to '[]' instead of 'null' in json
|
||||
resp.Logs.StepsLog = make([]*ViewStepLog, 0) // marshal to '[]' instead of 'null' in json
|
||||
resp.Logs.StepsLog = make([]*ViewStepLog, 0) // marshal to '[]' instead of 'null' in json
|
||||
// As noted above with TaskID; task will be nil when the job hasn't be picked yet...
|
||||
if task != nil {
|
||||
taskAttempts, err := task.GetAllAttempts(ctx)
|
||||
if err != nil {
|
||||
ctx.Error(http.StatusInternalServerError, err.Error())
|
||||
return nil
|
||||
}
|
||||
allAttempts := make([]*TaskAttempt, len(taskAttempts))
|
||||
for i, actionTask := range taskAttempts {
|
||||
allAttempts[i] = &TaskAttempt{
|
||||
Number: actionTask.Attempt,
|
||||
Started: templates.TimeSince(actionTask.Started),
|
||||
Status: actionTask.Status.String(),
|
||||
StatusDiagnostics: statusDiagnostics(actionTask.Status, task.Job, ctx.Locale),
|
||||
}
|
||||
}
|
||||
resp.State.CurrentJob.AllAttempts = allAttempts
|
||||
|
||||
steps := actions.FullSteps(task)
|
||||
for _, v := range steps {
|
||||
resp.State.CurrentJob.Steps = append(resp.State.CurrentJob.Steps, &ViewJobStep{
|
||||
|
|
|
|||
|
|
@ -340,7 +340,14 @@ func TestActionsViewViewPost(t *testing.T) {
|
|||
// in-sync with the RepoActionView 'view non-picked action run job' test.
|
||||
resp.State.CurrentJob.Details = []template.HTML{"actions.status.diagnostics.waiting"}
|
||||
resp.State.CurrentJob.Steps = []*ViewJobStep{}
|
||||
resp.State.CurrentJob.AllAttempts = nil
|
||||
resp.State.CurrentJob.AllAttempts = []*TaskAttempt{
|
||||
{
|
||||
Number: 1,
|
||||
Started: template.HTML("actions.jobs.not_started"),
|
||||
Status: "waiting",
|
||||
StatusDiagnostics: []template.HTML{"actions.status.diagnostics.waiting"},
|
||||
},
|
||||
}
|
||||
},
|
||||
},
|
||||
}
|
||||
|
|
|
|||
|
|
@ -147,8 +147,8 @@ func TestActionViewsView(t *testing.T) {
|
|||
url: "/user5/repo4/actions/runs/187",
|
||||
runIndex: 187,
|
||||
jobIndex: 0,
|
||||
attempt: 1,
|
||||
expectedJSON: "{\"state\":{\"run\":{\"preExecutionError\":\"\",\"link\":\"/user5/repo4/actions/runs/187\",\"title\":\"update actions\",\"titleHTML\":\"update actions\",\"status\":\"success\",\"canCancel\":false,\"canDelete\":false,\"canApprove\":false,\"canRerun\":false,\"canDeleteArtifact\":false,\"description\":\"Commit <a href=\\\"/user5/repo4/commit/c2d72f548424103f01ee1dc02889c1e2bff816b0\\\">c2d72f5484</a> pushed by <a href=\\\"/user1\\\">user1</a>\",\"done\":true,\"jobs\":[{\"id\":192,\"name\":\"job_2\",\"status\":\"success\",\"canRerun\":false,\"duration\":\"_duration_\"}],\"commit\":{\"localeWorkflow\":\"Workflow\",\"localeAllRuns\":\"all runs\",\"shortSHA\":\"c2d72f5484\",\"link\":\"/user5/repo4/commit/c2d72f548424103f01ee1dc02889c1e2bff816b0\",\"pusher\":{\"displayName\":\"user1\",\"link\":\"/user1\"},\"branch\":{\"name\":\"master\",\"link\":\"/user5/repo4/src/branch/master\",\"isDeleted\":false}}},\"currentJob\":{\"title\":\"job_2\",\"details\":[\"Success\"],\"steps\":[{\"summary\":\"Set up job\",\"duration\":\"_duration_\",\"status\":\"success\"},{\"summary\":\"Complete job\",\"duration\":\"_duration_\",\"status\":\"success\"}],\"allAttempts\":[{\"number\":3,\"time_since_started_html\":\"_time_\",\"status\":\"running\",\"status_diagnostics\":[\"Running\"]},{\"number\":2,\"time_since_started_html\":\"_time_\",\"status\":\"success\",\"status_diagnostics\":[\"Success\"]},{\"number\":1,\"time_since_started_html\":\"_time_\",\"status\":\"success\",\"status_diagnostics\":[\"Success\"]}]}},\"logs\":{\"stepsLog\":[]}}\n",
|
||||
attempt: 3,
|
||||
expectedJSON: "{\"state\":{\"run\":{\"preExecutionError\":\"\",\"link\":\"/user5/repo4/actions/runs/187\",\"title\":\"update actions\",\"titleHTML\":\"update actions\",\"status\":\"success\",\"canCancel\":false,\"canDelete\":false,\"canApprove\":false,\"canRerun\":false,\"canDeleteArtifact\":false,\"description\":\"Commit <a href=\\\"/user5/repo4/commit/c2d72f548424103f01ee1dc02889c1e2bff816b0\\\">c2d72f5484</a> pushed by <a href=\\\"/user1\\\">user1</a>\",\"done\":true,\"jobs\":[{\"id\":192,\"name\":\"job_2\",\"status\":\"success\",\"canRerun\":false,\"duration\":\"_duration_\"}],\"commit\":{\"localeWorkflow\":\"Workflow\",\"localeAllRuns\":\"all runs\",\"shortSHA\":\"c2d72f5484\",\"link\":\"/user5/repo4/commit/c2d72f548424103f01ee1dc02889c1e2bff816b0\",\"pusher\":{\"displayName\":\"user1\",\"link\":\"/user1\"},\"branch\":{\"name\":\"master\",\"link\":\"/user5/repo4/src/branch/master\",\"isDeleted\":false}}},\"currentJob\":{\"title\":\"job_2\",\"details\":[\"Success\"],\"steps\":[{\"summary\":\"Set up job\",\"duration\":\"_duration_\",\"status\":\"running\"},{\"summary\":\"Complete job\",\"duration\":\"_duration_\",\"status\":\"waiting\"}],\"allAttempts\":[{\"number\":3,\"time_since_started_html\":\"_time_\",\"status\":\"running\",\"status_diagnostics\":[\"Running\"]},{\"number\":2,\"time_since_started_html\":\"_time_\",\"status\":\"success\",\"status_diagnostics\":[\"Success\"]},{\"number\":1,\"time_since_started_html\":\"_time_\",\"status\":\"success\",\"status_diagnostics\":[\"Success\"]}]}},\"logs\":{\"stepsLog\":[]}}\n",
|
||||
expectedArtifacts: "{\"artifacts\":[{\"name\":\"multi-file-download\",\"size\":2048,\"status\":\"completed\"}]}\n",
|
||||
},
|
||||
{
|
||||
|
|
@ -230,7 +230,7 @@ func TestActionViewsViewAttemptOutOfRange(t *testing.T) {
|
|||
re = regexp.MustCompile(pattern)
|
||||
actualClean = re.ReplaceAllString(actualClean, `"time_since_started_html":"_time_"`)
|
||||
|
||||
assert.JSONEq(t, "{\"state\":{\"run\":{\"preExecutionError\":\"\",\"link\":\"/user5/repo4/actions/runs/190\",\"title\":\"job output\",\"titleHTML\":\"job output\",\"status\":\"success\",\"canCancel\":false,\"canDelete\":false,\"canApprove\":false,\"canRerun\":false,\"canDeleteArtifact\":false,\"description\":\"Commit <a href=\\\"/user5/repo4/commit/c2d72f548424103f01ee1dc02889c1e2bff816b0\\\">c2d72f5484</a> pushed by <a href=\\\"/user1\\\">user1</a>\",\"done\":false,\"jobs\":[{\"id\":396,\"name\":\"job_2\",\"status\":\"waiting\",\"canRerun\":false,\"duration\":\"_duration_\"}],\"commit\":{\"localeWorkflow\":\"Workflow\",\"localeAllRuns\":\"all runs\",\"shortSHA\":\"c2d72f5484\",\"link\":\"/user5/repo4/commit/c2d72f548424103f01ee1dc02889c1e2bff816b0\",\"pusher\":{\"displayName\":\"user1\",\"link\":\"/user1\"},\"branch\":{\"name\":\"test\",\"link\":\"/user5/repo4/src/branch/test\",\"isDeleted\":true}}},\"currentJob\":{\"title\":\"job_2\",\"details\":[\"Waiting for a runner with the following label: fedora\"],\"steps\":[],\"allAttempts\":null}},\"logs\":{\"stepsLog\":[]}}\n", actualClean)
|
||||
assert.JSONEq(t, "{\"state\":{\"run\":{\"preExecutionError\":\"\",\"link\":\"/user5/repo4/actions/runs/190\",\"title\":\"job output\",\"titleHTML\":\"job output\",\"status\":\"success\",\"canCancel\":false,\"canDelete\":false,\"canApprove\":false,\"canRerun\":false,\"canDeleteArtifact\":false,\"description\":\"Commit <a href=\\\"/user5/repo4/commit/c2d72f548424103f01ee1dc02889c1e2bff816b0\\\">c2d72f5484</a> pushed by <a href=\\\"/user1\\\">user1</a>\",\"done\":false,\"jobs\":[{\"id\":396,\"name\":\"job_2\",\"status\":\"waiting\",\"canRerun\":false,\"duration\":\"_duration_\"}],\"commit\":{\"localeWorkflow\":\"Workflow\",\"localeAllRuns\":\"all runs\",\"shortSHA\":\"c2d72f5484\",\"link\":\"/user5/repo4/commit/c2d72f548424103f01ee1dc02889c1e2bff816b0\",\"pusher\":{\"displayName\":\"user1\",\"link\":\"/user1\"},\"branch\":{\"name\":\"test\",\"link\":\"/user5/repo4/src/branch/test\",\"isDeleted\":true}}},\"currentJob\":{\"title\":\"job_2\",\"details\":[\"Waiting for a runner with the following label: fedora\"],\"steps\":[],\"allAttempts\":[{\"number\":1,\"status\":\"waiting\",\"status_diagnostics\":[\"Waiting for a runner with the following label: fedora\"],\"time_since_started_html\":\"not started\"}]}},\"logs\":{\"stepsLog\":[]}}\n", actualClean)
|
||||
})
|
||||
htmlDoc.AssertAttrEqual(t, selector, "data-initial-artifacts-response", "{\"artifacts\":[]}\n")
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue