From d0cdfe71132216b4c581d497af44eae43a27b72a Mon Sep 17 00:00:00 2001 From: bircni Date: Sun, 18 Oct 2026 17:43:11 +0200 Subject: [PATCH] fix: address verified review findings (actions-runs-1) Signed-off-by: bircni --- docs/migration-1.0.md | 1 + internal/app/action_runs.go | 62 ++++++++++++++++--- internal/app/action_runs_test.go | 95 +++++++++++++++++++++++++++-- internal/app/artifacts.go | 29 +++++++-- internal/app/artifacts_test.go | 38 +++++++++++- internal/app/target.go | 5 ++ internal/services/action_runs.go | 24 ++++++-- internal/services/artifacts.go | 24 ++++++-- internal/services/artifacts_test.go | 21 +++++++ 9 files changed, 267 insertions(+), 32 deletions(-) diff --git a/docs/migration-1.0.md b/docs/migration-1.0.md index 10f4b0db..04232232 100644 --- a/docs/migration-1.0.md +++ b/docs/migration-1.0.md @@ -258,6 +258,7 @@ Renamed flags that appear on many commands: | `--allow-login` (admin users edit) | `--prohibit-login=false` | | `--password-stdin`, `--no-must-change-password` (admin users) | `--password-file -`, `--must-change-password=false` | | `-u`, `-p`, `-e` (admin users create) | the username argument or `--username`, `--password`, `--email` | +| `--jobs=false` (`actions runs view`) | `run view --no-jobs` | | `--out` (man) | `--output-file` / `-o` | Other long names changed too, mostly on `admin user`, `repo fork`, `repo edit` and diff --git a/internal/app/action_runs.go b/internal/app/action_runs.go index ad891b9f..2039adc3 100644 --- a/internal/app/action_runs.go +++ b/internal/app/action_runs.go @@ -8,6 +8,8 @@ import ( "context" "errors" "fmt" + "io" + "path" "slices" "strconv" "strings" @@ -69,10 +71,15 @@ $ tea run list --since 24h --json id,status,conclusion,workflow`, opts.Actor = t.expandMe(opts.Actor) if opts.Workflow != "" { workflow, err := service.FindWorkflow(cmd.Context(), opts.Workflow) + // The server also lists the runs of a workflow file that only exists on another branch or was removed. + if notFound, ok := errors.AsType[*services.NotFoundError](err); ok && notFound.Kind == "workflow" && isWorkflowFile(opts.Workflow) { + err = nil + } else if err == nil { + opts.Workflow = workflow.ID + } if err != nil { return err } - opts.Workflow = workflow.ID } page, err := service.RunsPage(cmd.Context(), opts) if errors.Is(err, services.ErrRunScanExhausted) { @@ -230,6 +237,12 @@ $ tea run view 34 --json jobs --jq '.jobs[] | {name, conclusion}'`, return cmd } +// isWorkflowFile tells a workflow file name, which the server resolves itself, from a workflow name. +func isWorkflowFile(raw string) bool { + ext := path.Ext(raw) + return !strings.ContainsAny(raw, " /") && (ext == ".yml" || ext == ".yaml") +} + // runHints are the next steps shown under a run: watching it while it runs, and reading the logs // of its jobs once one started. func (a *application) runHints(t target, run services.Run) []string { @@ -250,9 +263,23 @@ func (a *application) suggestedRepo(t target) string { if strings.HasPrefix(t.RepoSource, "remote ") || strings.HasPrefix(t.RepoSource, "git config") { return "" } + // An instance that --host or the repository itself selected is not the one a copied command finds. + if (t.HostSource == "--host" || (t.RepoSource != "" && t.HostSource == t.RepoSource)) && !a.isDefaultHost(t.Host) { + return " -R " + t.Host + "/" + t.Repo + } return " -R " + t.Repo } +// isDefaultHost reports whether host is the instance that commands use without --host or a repository URL. +func (a *application) isDefaultHost(host string) bool { + s, err := a.settings() + if err != nil { + return false + } + var fallback target + return a.defaultHost(&fallback, s) == nil && fallback.Host == host +} + // renderRun shows a run with its jobs: on a terminal its state, trigger and jobs with their // steps, followed by hints and the run's URL; for scripts "key:\tvalue" lines and the jobs. func renderRun(run services.Run, d terminal.Display, hints []string) string { @@ -338,11 +365,15 @@ func jobLines(jobs []services.Job, now time.Time) [][]terminal.Cell { return rows } -// waitingForRunner explains a run that waits for a runner: none of its jobs started yet. +// waitingForRunner explains a run that waits for a runner: none of its jobs started yet. A run +// that needs approval, such as one from a first-time contributor's fork, has the status "waiting". func waitingForRunner(run services.Run) string { if run.Completed() || len(run.Jobs) == 0 || slices.ContainsFunc(run.Jobs, services.Job.Started) { return "" } + if run.Status == "waiting" { + return "Waiting for approval; approve the run in the web interface" + } return "Waiting for a runner" + runnerLabels(run) } @@ -515,7 +546,8 @@ func (a *application) watchRun(cmd *cobra.Command, t target, id int64, interval, if err != nil { if errors.Is(err, context.DeadlineExceeded) && run.ID != 0 { if a.options.json == "" { - _ = a.writeHuman(cmd, renderRun(run, a.display(), nil)) + // Not through the pager, which would stop the command at its end. + _, _ = io.WriteString(out, renderRun(run, a.display(), nil)) } return timeoutError(err, timeout, "waiting for run %d (%s)", id, stillRunning(run)) } @@ -525,12 +557,12 @@ func (a *application) watchRun(cmd *cobra.Command, t target, id int64, interval, if err := a.writeJSON(cmd, run); err != nil { return err } - } else if err := a.writeHuman(cmd, renderRun(run, a.display(), nil)); err != nil { + } else if _, err := io.WriteString(out, renderRun(run, a.display(), nil)); err != nil { return err } - conclusion := cmp.Or(run.Conclusion, run.Status) + conclusion := terminal.SafeLine(cmp.Or(run.Conclusion, run.Status)) if !services.SuccessfulRun(run) { - return fmt.Errorf("run %d (%s) completed with %s", run.ID, run.Workflow, conclusion) + return fmt.Errorf("run %d (%s) completed with %s", run.ID, terminal.SafeLine(run.Workflow), conclusion) } took := "" if d, ok := runElapsed(run.StartedAt, run.CompletedAt, a.deps.Now()); ok { @@ -557,10 +589,13 @@ func runProgress(run services.Run) string { // stillRunning says how far a run got when a wait for it expired, suspecting a missing runner // when no job started. func stillRunning(run services.Run) string { - if waitingForRunner(run) != "" { - return "still " + run.Status + "; is a runner registered" + runnerLabels(run) + "?" + if run.Status == "waiting" && waitingForRunner(run) != "" { + return "still waiting for approval in the web interface" } - return "still " + run.Status + if waitingForRunner(run) != "" { + return "still " + terminal.SafeLine(run.Status) + "; is a runner registered" + runnerLabels(run) + "?" + } + return "still " + terminal.SafeLine(run.Status) } func (a *application) runCancelCommand() *cobra.Command { @@ -580,11 +615,15 @@ $ tea run cancel 34 --force`, if err != nil { return err } - run, err := services.Actions{Client: t.client, Repository: t.Repo}.CancelRun(cmd.Context(), id, force) + service := services.Actions{Client: t.client, Repository: t.Repo} + run, err := service.CancelRun(cmd.Context(), id, force) if err != nil { return err } a.done(cmd, "Requested cancellation of run %d", id) + if a.options.json != "" { + run = service.NameRun(cmd.Context(), run) + } return a.jsonResult(cmd, run) }, } @@ -643,6 +682,9 @@ $ tea run rerun 34 --job build`, return err } a.done(cmd, "Requested rerun of run %d (attempt %d); watch it with: tea run watch %d%s", id, run.Attempt, id, a.suggestedRepo(t)) + if a.options.json != "" { + run = service.NameRun(cmd.Context(), run) + } return a.jsonResult(cmd, run) }, } diff --git a/internal/app/action_runs_test.go b/internal/app/action_runs_test.go index 458555e5..2c7c1c42 100644 --- a/internal/app/action_runs_test.go +++ b/internal/app/action_runs_test.go @@ -16,6 +16,7 @@ import ( "time" "gitea.dev/tea/internal/clierr" + "gitea.dev/tea/internal/services" "gitea.dev/tea/internal/terminal" "github.com/stretchr/testify/require" ) @@ -132,6 +133,22 @@ X 29 Nightly Refactor the deployment pipeline feature/x push } } +func TestRunListAcceptsWorkflowFilesMissingFromTheDefaultBranch(t *testing.T) { + server, requests := actionsAPI(t, map[string]string{ + "GET actions/workflows": workflowsFixture, + "GET actions/workflows/old.yml/runs": `{"total_count":1,"workflow_runs":[` + failedRunFixture + `]}`, + "GET actions/workflows/gone.yml/runs": `404 {"message":"workflow \"gone.yml\" not found"}`, + }) + deps, out, _ := actionsDeps(t, server.URL, false) + require.NoError(t, Execute(context.Background(), deps, []string{"run", "list", "-R", "alice/project", "--workflow", "old.yml", "--json", "id"})) + require.JSONEq(t, `[{"id":29}]`, out.String()) + err := Execute(context.Background(), deps, []string{"run", "list", "-R", "alice/project", "--workflow", "gone.yml"}) + require.EqualError(t, err, `workflow "gone.yml" not found in alice/project`) + err = Execute(context.Background(), deps, []string{"run", "list", "-R", "alice/project", "--workflow", "Missing name"}) + require.EqualError(t, err, `workflow "Missing name" not found in alice/project`) + require.NotContains(t, *requests, "GET actions/workflows/Missing name/runs") +} + func TestRunListResolvesWorkflowNamesAndReportsEmptyFilters(t *testing.T) { server, requests := actionsAPI(t, map[string]string{ "GET actions/workflows": workflowsFixture, @@ -290,6 +307,34 @@ func TestRunWatchExitStatusAndTimeout(t *testing.T) { } } +func TestRunWatchSummaryBypassesThePager(t *testing.T) { + server, _ := actionsAPI(t, map[string]string{ + "GET actions/workflows": workflowsFixture, + "GET actions/runs/7": strings.Replace(failedRunFixture, `"id":29`, `"id":7`, 1), + "GET actions/runs/7/jobs": failedJobsFixture, + }) + deps, out, _ := actionsDeps(t, server.URL, true) + pager := &recordingPager{} + deps.Pager = pager + deps.Getenv = serverEnv(server.URL, map[string]string{"TERM": "dumb", "PAGER": "fake-pager"}) + err := Execute(context.Background(), deps, []string{"run", "watch", "7", "-R", "alice/project", "--interval=1ms"}) + require.EqualError(t, err, "run 7 (Nightly) completed with failure") + require.Zero(t, pager.calls, "the pager would hold the command back after the run completed") + require.Contains(t, out.String(), "Run tests") +} + +func TestRunWatchErrorsDoNotCarryServerControlCharacters(t *testing.T) { + server, _ := actionsAPI(t, map[string]string{ + "GET actions/workflows": `{"workflows":[]}`, + "GET actions/runs/7": `{"id":7,"path":"\u001b]0;pwned\u0007ci.yml@refs/heads/main","status":"completed","conclusion":"failure", + "created_at":"2028-01-01T11:59:00Z","started_at":"2028-01-01T11:59:00Z","completed_at":"2028-01-01T11:59:30Z"}`, + "GET actions/runs/7/jobs": `{"jobs":[]}`, + }) + deps, _, _ := actionsDeps(t, server.URL, false) + err := Execute(context.Background(), deps, []string{"run", "watch", "7", "-R", "alice/project", "--interval=1ms"}) + require.EqualError(t, err, "run 7 (]0;pwnedci) completed with failure") +} + func TestRunWatchRedrawsOnATerminalAndSummarizes(t *testing.T) { server, _ := actionsAPI(t, map[string]string{ "GET actions/workflows": workflowsFixture, @@ -323,10 +368,17 @@ func TestRunMutationsReportWhatHappened(t *testing.T) { }, { "cancel returns the run", - []string{"run", "cancel", "47", "--json", "id,status"}, - map[string]string{"POST actions/runs/47/cancel": queuedRunFixture}, - []string{"POST actions/runs/47/cancel"}, - `{"id":47,"status":"queued"}`, "", nil, + []string{"run", "cancel", "47", "--json", "id,status,workflow"}, + map[string]string{"POST actions/runs/47/cancel": queuedRunFixture, "GET actions/workflows": workflowsFixture}, + []string{"POST actions/runs/47/cancel", "GET actions/workflows"}, + `{"id":47,"status":"queued","workflow":"CI"}`, "", nil, + }, + { + "cancel with fields the server may leave empty", + []string{"run", "cancel", "47", "--json", "id,jobs,attempt,event"}, + map[string]string{"POST actions/runs/47/cancel": `{"id":47}`, "GET actions/workflows": workflowsFixture}, + []string{"POST actions/runs/47/cancel", "GET actions/workflows"}, + `{"id":47,"jobs":null,"attempt":0,"event":""}`, "", nil, }, { "rerun", @@ -335,6 +387,13 @@ func TestRunMutationsReportWhatHappened(t *testing.T) { []string{"GET actions/runs/29", "POST actions/runs/29/rerun", "GET actions/runs/29"}, "", "āœ“ Requested rerun of run 29 (attempt 2); watch it with: tea run watch 29 -R alice/project\n", nil, }, + { + "rerun returns the run", + []string{"run", "rerun", "29", "--json", "id,workflow"}, + map[string]string{"GET actions/runs/29": completedRun, "POST actions/runs/29/rerun": completedRun, "GET actions/workflows": workflowsFixture}, + []string{"GET actions/runs/29", "POST actions/runs/29/rerun", "GET actions/runs/29", "GET actions/workflows"}, + `{"id":29,"workflow":"Nightly"}`, "", nil, + }, { "rerun a job by name", []string{"run", "rerun", "29", "--job", "TEST"}, @@ -395,3 +454,31 @@ func TestRunJobsListsStatusAndElapsedTime(t *testing.T) { require.NoError(t, Execute(context.Background(), deps, []string{"run", "jobs", "#29", "-R", "alice/project"})) require.Equal(t, "Showing 2 of 2 jobs in run 29\n\nSTATUS ID NAME ELAPSED\nāœ“ 30 build 45s\nX 31 test 27s\n", out.String()) } + +func TestRunsAwaitingApprovalAreNotReportedAsWaitingForARunner(t *testing.T) { + jobs := []services.Job{{ID: 1, Name: "build", Status: "waiting", Labels: []string{"ubuntu-latest"}}} + approval := services.Run{ID: 7, Status: "waiting", Jobs: jobs} + require.Equal(t, "Waiting for approval; approve the run in the web interface", waitingForRunner(approval)) + require.Equal(t, "still waiting for approval in the web interface", stillRunning(approval)) + queued := services.Run{ID: 7, Status: "queued", Jobs: jobs} + require.Equal(t, `Waiting for a runner for "ubuntu-latest"`, waitingForRunner(queued)) + require.Equal(t, `still queued; is a runner registered for "ubuntu-latest"?`, stillRunning(queued)) +} + +func TestRunSuggestionsNameTheInstanceThatWasSelectedExplicitly(t *testing.T) { + server, _ := actionsAPI(t, map[string]string{"GET actions/runs/47": queuedRunFixture}) + host := strings.TrimPrefix(server.URL, "https://") + for _, tc := range []struct { + args []string + want string + }{ + {[]string{"-R", "alice/project", "--host", server.URL}, " -R " + host + "/alice/project"}, + {[]string{"-R", host + "/alice/project", "--host", server.URL}, " -R " + host + "/alice/project"}, + } { + deps, _, _ := actionsDeps(t, server.URL, false) + // The instance that commands use by default is another one than the server. + deps.Getenv = serverEnv(server.URL, map[string]string{"TERM": "dumb", "GITEA_INSTANCE_URL": server.URL, "TEA_HOST": "https://default.test"}) + err := Execute(context.Background(), deps, append([]string{"run", "rerun", "47"}, tc.args...)) + require.EqualError(t, err, "run 47 is still queued; cancel it first: tea run cancel 47"+tc.want, tc.args) + } +} diff --git a/internal/app/artifacts.go b/internal/app/artifacts.go index aaaf86bc..9330f347 100644 --- a/internal/app/artifacts.go +++ b/internal/app/artifacts.go @@ -70,11 +70,10 @@ var artifactColumns = []terminal.Column{ func artifactRow(item services.Artifact) []terminal.Cell { expires := terminal.Cell{} - expiresAt := "" if item.ExpiresAt != nil { - expiresAt = *item.ExpiresAt - expires.Time = terminal.ParseTime(expiresAt) + expires.Time = terminal.ParseTime(*item.ExpiresAt) } + piped := terminal.Cell{Time: expires.Time} if item.Expired { expires = terminal.Cell{Text: "expired", Color: "red"} } @@ -84,7 +83,7 @@ func artifactRow(item services.Artifact) []terminal.Cell { {Text: terminal.Bytes(item.Size)}, {Text: strconv.FormatInt(item.Size, 10)}, expires, - {Text: expiresAt}, + piped, {Text: strconv.FormatBool(item.Expired)}, } } @@ -122,7 +121,7 @@ $ tea run download 34 --zip`, if err != nil { return err } - selected, err := selectArtifacts(artifacts, names, run) + selected, err := selectArtifacts(latestArtifacts(artifacts), names, run) if err != nil { return err } @@ -153,6 +152,24 @@ $ tea run download 34 --zip`, return cmd } +// latestArtifacts keeps the newest artifact of each name: after a rerun, the run lists the artifacts +// of every attempt, which may share names. +func latestArtifacts(artifacts []services.Artifact) []services.Artifact { + index := map[string]int{} + latest := make([]services.Artifact, 0, len(artifacts)) + for _, artifact := range artifacts { + if i, seen := index[artifact.Name]; seen { + if artifact.ID > latest[i].ID { + latest[i] = artifact + } + continue + } + index[artifact.Name] = len(latest) + latest = append(latest, artifact) + } + return latest +} + // selectArtifacts returns the artifacts of a run with the given names, or all of them. func selectArtifacts(artifacts []services.Artifact, names []string, run int64) ([]services.Artifact, error) { if len(artifacts) == 0 { @@ -167,7 +184,7 @@ func selectArtifacts(artifacts []services.Artifact, names []string, run int64) ( if index < 0 { available := make([]string, len(artifacts)) for i, artifact := range artifacts { - available[i] = artifact.Name + available[i] = terminal.SafeLine(artifact.Name) } return nil, fmt.Errorf("artifact %q not found in run %d (artifacts: %s)", name, run, strings.Join(available, ", ")) } diff --git a/internal/app/artifacts_test.go b/internal/app/artifacts_test.go index f0bc5c54..c9250b56 100644 --- a/internal/app/artifacts_test.go +++ b/internal/app/artifacts_test.go @@ -7,10 +7,12 @@ import ( "archive/zip" "bytes" "context" + "fmt" "net/http" "os" "path/filepath" "runtime" + "strconv" "strings" "testing" @@ -63,7 +65,6 @@ func TestRunDownloadExtractsArtifactsByName(t *testing.T) { "GET actions/runs/29/artifacts": `{"total_count":3,"artifacts":[{"id":1,"name":"build","size_in_bytes":3},{"id":2,"name":"coverage"},{"id":3,"name":"old","expired":true}]}`, "GET actions/artifacts/1/zip": zipOf(t, map[string]string{"bin/tea": "tea", "README": "read me"}), "GET actions/artifacts/2/zip": zipOf(t, map[string]string{"cover.out": "100%"}), - "GET actions/artifacts/4/zip": zipOf(t, map[string]string{"../escape": "x"}), }) for _, tc := range []struct { name string @@ -108,6 +109,39 @@ func TestRunDownloadExtractsArtifactsByName(t *testing.T) { } }) } + t.Run("newest artifact of a rerun name", func(t *testing.T) { + server, _ := actionsAPI(t, map[string]string{ + "GET actions/runs/29/artifacts": `{"total_count":3,"artifacts":[{"id":1,"name":"dist"},{"id":2,"name":"docs"},{"id":3,"name":"dist"}]}`, + "GET actions/artifacts/2/zip": zipOf(t, map[string]string{"index": "docs"}), + "GET actions/artifacts/3/zip": zipOf(t, map[string]string{"app": "new"}), + }) + dir := t.TempDir() + deps, _, _ := actionsDeps(t, server.URL, false) + require.NoError(t, Execute(context.Background(), deps, []string{"run", "download", "29", "-R", "alice/project", "-D", dir})) + data, err := os.ReadFile(filepath.Join(dir, "dist", "app")) + require.NoError(t, err) + require.Equal(t, "new", string(data)) + require.NoError(t, Execute(context.Background(), deps, []string{"run", "download", "29", "-R", "alice/project", "-D", dir, "-n", "dist", "--clobber"})) + }) + t.Run("unknown name lists artifacts without control characters", func(t *testing.T) { + server, _ := actionsAPI(t, map[string]string{"GET actions/runs/29/artifacts": `{"total_count":1,"artifacts":[{"id":4,"name":"build\u001b]0;pwned\u0007"}]}`}) + deps, _, _ := actionsDeps(t, server.URL, false) + err := Execute(context.Background(), deps, []string{"run", "download", "29", "-R", "alice/project", "-D", t.TempDir(), "-n", "docs"}) + require.EqualError(t, err, `artifact "docs" not found in run 29 (artifacts: build]0;pwned)`) + }) + t.Run("unsafe artifact names", func(t *testing.T) { + for _, name := range []string{"../evil", "..", "a/b", `a\b`, "/abs"} { + server, requests := actionsAPI(t, map[string]string{"GET actions/runs/29/artifacts": `{"total_count":1,"artifacts":[{"id":4,"name":` + strconv.Quote(name) + `}]}`}) + dir := t.TempDir() + deps, _, _ := actionsDeps(t, server.URL, false) + err := Execute(context.Background(), deps, []string{"run", "download", "29", "-R", "alice/project", "-D", filepath.Join(dir, "out")}) + require.ErrorContains(t, err, fmt.Sprintf("cannot use %q as a directory name", name)) + require.Equal(t, []string{"GET actions/runs/29/artifacts"}, *requests) + entries, err := os.ReadDir(dir) + require.NoError(t, err) + require.Empty(t, entries) + } + }) t.Run("existing files and unsafe paths", func(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "cover.out"), []byte("mine"), 0o600)) @@ -156,7 +190,7 @@ func TestExtractZipKeepsPermissionBits(t *testing.T) { func TestArtifactListShowsSizesAndExpiry(t *testing.T) { server, _ := actionsAPI(t, map[string]string{ - "GET actions/runs/29/artifacts": `{"total_count":2,"artifacts":[{"id":1,"name":"build","size_in_bytes":3145728,"expires_at":"2028-03-01T12:00:00Z"},{"id":3,"name":"old","size_in_bytes":13,"expired":true,"expires_at":"2027-12-01T12:00:00Z"}]}`, + "GET actions/runs/29/artifacts": `{"total_count":2,"artifacts":[{"id":1,"name":"build","size_in_bytes":3145728,"expires_at":"2028-03-01T14:00:00+02:00"},{"id":3,"name":"old","size_in_bytes":13,"expired":true,"expires_at":"2027-12-01T12:00:00Z"}]}`, }) for tty, want := range map[bool]string{ true: "Showing 2 of 2 artifacts of run 29\n\nID NAME SIZE EXPIRES\n1 build 3.0 MiB in about 2 months\n3 old 13 B expired\n", diff --git a/internal/app/target.go b/internal/app/target.go index f368eb28..e9c3d437 100644 --- a/internal/app/target.go +++ b/internal/app/target.go @@ -356,6 +356,11 @@ func (a *application) selectHost(t *target, s settings.Snapshot) error { } t.Host, t.HostSource = explicit, "--host" } + return a.defaultHost(t, s) +} + +// defaultHost picks the instance of a target that neither the repository nor --host named. +func (a *application) defaultHost(t *target, s settings.Snapshot) error { if t.Host == "" { for _, key := range []string{"TEA_HOST", "GITEA_INSTANCE_URL"} { if value := a.deps.Getenv(key); value != "" { diff --git a/internal/services/action_runs.go b/internal/services/action_runs.go index 5897280c..e025924a 100644 --- a/internal/services/action_runs.go +++ b/internal/services/action_runs.go @@ -35,11 +35,11 @@ type Run struct { Conclusion string `json:"conclusion"` URL string `json:"url"` Branch string `json:"branch"` - Event string `json:"event,omitempty"` - Actor string `json:"actor,omitempty"` - Commit string `json:"commit,omitempty"` - Attempt int64 `json:"attempt,omitempty"` - Jobs []Job `json:"jobs,omitempty"` + Event string `json:"event"` + Actor string `json:"actor"` + Commit string `json:"commit"` + Attempt int64 `json:"attempt"` + Jobs []Job `json:"jobs"` } // Completed reports whether the run finished, whatever its conclusion. @@ -193,7 +193,7 @@ func (s Actions) runsPage(ctx context.Context, opts RunListOptions) (Page[Run], if opts.Workflow != "" { endpoint = s.endpoint() + "workflows/" + url.PathEscape(opts.Workflow) + "/runs" } - return listPage(opts.Limit, opts.Page, func(limit, page int) ([]Run, http.Header, error) { + runs, err := listPage(opts.Limit, opts.Page, func(limit, page int) ([]Run, http.Header, error) { query.Set("limit", strconv.Itoa(limit)) query.Set("page", strconv.Itoa(page)) var response struct { @@ -207,6 +207,10 @@ func (s Actions) runsPage(ctx context.Context, opts RunListOptions) (Page[Run], } return runs, headers, err }) + if opts.Workflow != "" && IsNotFound(err) { + return Page[Run]{}, NotFound("workflow", strconv.Quote(opts.Workflow), s.Repository) + } + return runs, err } // nameRuns names runs after their workflows. @@ -240,6 +244,14 @@ func (s Actions) Run(ctx context.Context, id int64) (Run, error) { return response.result(), err } +// NameRun names a run that a mutation returned after its workflow; it stays as is when the +// workflows cannot be listed. +func (s Actions) NameRun(ctx context.Context, run Run) Run { + runs := []Run{run} + _ = s.nameRuns(ctx, runs) + return runs[0] +} + // RunDetails retrieves one run with its jobs, named after its workflow. func (s Actions) RunDetails(ctx context.Context, id int64) (Run, error) { names, err := s.workflowNames(ctx) diff --git a/internal/services/artifacts.go b/internal/services/artifacts.go index b17f3cf0..38dc2b93 100644 --- a/internal/services/artifacts.go +++ b/internal/services/artifacts.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "io" + "net/http" "strconv" gitea "gitea.dev/sdk" @@ -33,14 +34,29 @@ func artifactFrom(artifact *gitea.ActionsArtifact) Artifact { // Artifacts lists one page of artifacts belonging to a run, with their total number. func (s Actions) Artifacts(ctx context.Context, run int64, limit, page int) (Page[Artifact], error) { - response, err := repoCall(ctx, s.Client, s.Repository, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.ActionsArtifactsResponse, *gitea.Response, error) { - return c.Actions.ListRepoRunArtifacts(ctx, owner, repo, run, gitea.ListActionsArtifactsOptions{ListOptions: listOptions(limit, page)}) + owner, repo, err := splitRepository(s.Repository) + if err != nil { + return Page[Artifact]{}, err + } + result, err := listPage(limit, page, func(limit, page int) ([]Artifact, http.Header, error) { + response, resp, err := call(ctx, s.Client, func(ctx context.Context, c *gitea.Client) (*gitea.ActionsArtifactsResponse, *gitea.Response, error) { + return c.Actions.ListRepoRunArtifacts(ctx, owner, repo, run, gitea.ListActionsArtifactsOptions{ListOptions: listOptions(limit, page)}) + }) + if err != nil { + return nil, nil, err + } + // The server sends its count in the body only, and caps the page size below what AllArtifacts asks for; + // a count smaller than the page means the response lacks it. + headers := headerOf(resp).Clone() + if total := int(response.TotalCount); total >= len(response.Artifacts) { + headers.Set("X-Total-Count", strconv.Itoa(total)) + } + return convertAll(response.Artifacts, artifactFrom), headers, nil }) if err != nil { return Page[Artifact]{}, Classify(err, "run", strconv.FormatInt(run, 10), s.Repository) } - total := int(response.TotalCount) - return Page[Artifact]{Items: convertAll(response.Artifacts, artifactFrom), More: total > page*limit, Total: total, Counted: true}, nil + return result, nil } // AllArtifacts lists every artifact belonging to a run. diff --git a/internal/services/artifacts_test.go b/internal/services/artifacts_test.go index d48a8ceb..155e82e7 100644 --- a/internal/services/artifacts_test.go +++ b/internal/services/artifacts_test.go @@ -6,7 +6,10 @@ package services import ( "context" "encoding/json" + "fmt" "net/http" + "strconv" + "strings" "testing" gitea "gitea.dev/sdk" @@ -46,3 +49,21 @@ func TestArtifactFromMapsServerFields(t *testing.T) { CreatedAt: "2028-01-02T03:04:05Z", UpdatedAt: "2028-01-02T03:04:05Z", ExpiresAt: &expires, }, artifactFrom(artifact)) } + +func TestAllArtifactsReadsPastTheServersPageCap(t *testing.T) { + const total, serverCap = 130, 50 + client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + limit, _ := strconv.Atoi(r.URL.Query().Get("limit")) + page, _ := strconv.Atoi(r.URL.Query().Get("page")) + size := min(limit, serverCap) + items := []string{} + for id := (page-1)*size + 1; id <= min(page*size, total); id++ { + items = append(items, fmt.Sprintf(`{"id":%d,"name":"a%d"}`, id, id)) + } + _, _ = fmt.Fprintf(w, `{"total_count":%d,"artifacts":[%s]}`, total, strings.Join(items, ",")) + })) + artifacts, err := Actions{Client: client, Repository: "owner/repo"}.AllArtifacts(context.Background(), 7) + require.NoError(t, err) + require.Len(t, artifacts, total) + require.EqualValues(t, total, artifacts[total-1].ID) +}