From 44e1efc22190504b8428d82b338012297153b256 Mon Sep 17 00:00:00 2001 From: Harvey Sanders Date: Mon, 21 Aug 2023 10:29:58 -0400 Subject: [PATCH 1/6] Fetch all jobs on a gh view run --json --- pkg/cmd/run/shared/shared.go | 22 +++++++++++++---- pkg/cmd/run/shared/test.go | 18 ++++++++++++++ pkg/cmd/run/view/view_test.go | 45 +++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/run/shared/shared.go b/pkg/cmd/run/shared/shared.go index a755ce93c..9fcdb5df3 100644 --- a/pkg/cmd/run/shared/shared.go +++ b/pkg/cmd/run/shared/shared.go @@ -424,7 +424,8 @@ func preloadWorkflowNames(client *api.Client, repo ghrepo.Interface, runs []Run) } type JobsPayload struct { - Jobs []Job + TotalCount int `json:"total_count"` + Jobs []Job } func GetJobs(client *api.Client, repo ghrepo.Interface, run *Run) ([]Job, error) { @@ -432,10 +433,23 @@ func GetJobs(client *api.Client, repo ghrepo.Interface, run *Run) ([]Job, error) return run.Jobs, nil } var result JobsPayload - if err := client.REST(repo.RepoHost(), "GET", run.JobsURL, nil, &result); err != nil { - return nil, err + perPage := 100 + v := url.Values{} + v.Set("per_page", fmt.Sprintf("%d", perPage)) + + // Fetch all the pages until total fetched jobs == result.TotalCount + maxPages := 10 + for p := 1; p < maxPages; p++ { + v.Set("page", fmt.Sprintf("%d", p)) + jobsPath := fmt.Sprintf("%s?%s", run.JobsURL, v.Encode()) + if err := client.REST(repo.RepoHost(), "GET", jobsPath, nil, &result); err != nil { + return run.Jobs, err + } + run.Jobs = append(run.Jobs, result.Jobs...) + if len(run.Jobs) >= result.TotalCount { + break + } } - run.Jobs = result.Jobs return result.Jobs, nil } diff --git a/pkg/cmd/run/shared/test.go b/pkg/cmd/run/shared/test.go index 0e37572e3..e44debaa4 100644 --- a/pkg/cmd/run/shared/test.go +++ b/pkg/cmd/run/shared/test.go @@ -5,6 +5,7 @@ import ( "time" workflowShared "github.com/cli/cli/v2/pkg/cmd/workflow/shared" + "github.com/cli/cli/v2/pkg/iostreams" ) var TestRunStartTime, _ = time.Parse("2006-01-02 15:04:05", "2021-02-23 04:51:00") @@ -122,3 +123,20 @@ var TestWorkflow workflowShared.Workflow = workflowShared.Workflow{ Name: "CI", ID: 123, } + +type TestExporter struct { + fields []string + writeHandler func(io *iostreams.IOStreams, data interface{}) error +} + +func MakeTestExporter(fields []string, wh func(io *iostreams.IOStreams, data interface{}) error) *TestExporter { + return &TestExporter{fields: fields, writeHandler: wh} +} + +func (t *TestExporter) Fields() []string { + return t.fields +} + +func (t *TestExporter) Write(io *iostreams.IOStreams, data interface{}) error { + return t.writeHandler(io, data) +} diff --git a/pkg/cmd/run/view/view_test.go b/pkg/cmd/run/view/view_test.go index 3d4fc4cf0..e2a2a2456 100644 --- a/pkg/cmd/run/view/view_test.go +++ b/pkg/cmd/run/view/view_test.go @@ -1242,6 +1242,51 @@ func TestViewRun(t *testing.T) { }, wantOut: "\nX trunk CI ยท 123\nTriggered via push about 59 minutes ago\n\nX This run likely failed because of a workflow file issue.\n\nFor more information, see: https://github.com/runs/123\n", }, + { + name: "Fetches all of a run's jobs with --json flag", + opts: &ViewOptions{ + RunID: "3", + Exporter: shared.MakeTestExporter( + []string{"jobs"}, + func(io *iostreams.IOStreams, data interface{}) error { + run, ok := data.(*shared.Run) + if !ok { + return fmt.Errorf("expected data type *shared.Run") + } + fmt.Fprintf(io.Out, "fetched %d jobs\n", len(run.Jobs)) + return nil + }, + ), + }, + httpStubs: func(reg *httpmock.Registry) { + jobs := []shared.Job{} + for i := 0; i < 115; i++ { + jobs = append(jobs, shared.Job{ + ID: int64(i), + Name: fmt.Sprintf("job %d", i), + }) + } + + limit := 100 + firstPage := jobs[:limit] + secondPage := jobs[limit:] + + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/actions/runs/3"), + httpmock.JSONResponse(shared.SuccessfulRun)) + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/123"), + httpmock.JSONResponse(shared.TestWorkflow)) + reg.Register( + httpmock.REST("GET", "runs/3/jobs"), + httpmock.JSONResponse(shared.JobsPayload{TotalCount: len(jobs), Jobs: firstPage})) + // /runs/:runID/jobs endpoint should be called once more to fetch the remaining jobs + reg.Register( + httpmock.REST("GET", "runs/3/jobs"), + httpmock.JSONResponse(shared.JobsPayload{TotalCount: len(jobs), Jobs: secondPage})) + }, + wantOut: "fetched 115 jobs\n", + }, } for _, tt := range tests { From a4547481d6041554ea9c610087f8efbea75c90c8 Mon Sep 17 00:00:00 2001 From: Harvey Sanders Date: Sat, 2 Sep 2023 01:01:29 -0400 Subject: [PATCH 2/6] Refactor GetJobs to use RESTWithNext --- pkg/cmd/run/shared/shared.go | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/pkg/cmd/run/shared/shared.go b/pkg/cmd/run/shared/shared.go index 9fcdb5df3..3bbfb3726 100644 --- a/pkg/cmd/run/shared/shared.go +++ b/pkg/cmd/run/shared/shared.go @@ -4,6 +4,7 @@ import ( "archive/zip" "errors" "fmt" + "net/http" "net/url" "reflect" "strings" @@ -432,25 +433,26 @@ func GetJobs(client *api.Client, repo ghrepo.Interface, run *Run) ([]Job, error) if run.Jobs != nil { return run.Jobs, nil } - var result JobsPayload - perPage := 100 - v := url.Values{} - v.Set("per_page", fmt.Sprintf("%d", perPage)) - // Fetch all the pages until total fetched jobs == result.TotalCount - maxPages := 10 - for p := 1; p < maxPages; p++ { - v.Set("page", fmt.Sprintf("%d", p)) - jobsPath := fmt.Sprintf("%s?%s", run.JobsURL, v.Encode()) - if err := client.REST(repo.RepoHost(), "GET", jobsPath, nil, &result); err != nil { + v := url.Values{} + v.Set("per_page", "100") + + jobsPath := fmt.Sprintf("%s?%s", run.JobsURL, v.Encode()) + + for jobsPath != "" { + var resp JobsPayload + var err error + jobsPath, err = client.RESTWithNext(repo.RepoHost(), http.MethodGet, jobsPath, nil, &resp) + if err != nil { return run.Jobs, err } - run.Jobs = append(run.Jobs, result.Jobs...) - if len(run.Jobs) >= result.TotalCount { - break + + run.Jobs = append(run.Jobs, resp.Jobs...) + if len(run.Jobs) >= resp.TotalCount { + return run.Jobs, nil } } - return result.Jobs, nil + return run.Jobs, nil } func GetJob(client *api.Client, repo ghrepo.Interface, jobID string) (*Job, error) { From acc362f9d8977412386b6724f7444768be92bcd8 Mon Sep 17 00:00:00 2001 From: Harvey Sanders Date: Sun, 3 Sep 2023 19:55:27 -0400 Subject: [PATCH 3/6] Fix pagination resp with "Link" header --- pkg/cmd/run/view/view_test.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/run/view/view_test.go b/pkg/cmd/run/view/view_test.go index e2a2a2456..1bef02fc3 100644 --- a/pkg/cmd/run/view/view_test.go +++ b/pkg/cmd/run/view/view_test.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "net/http" + "net/url" "testing" "time" @@ -1278,9 +1279,12 @@ func TestViewRun(t *testing.T) { httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/123"), httpmock.JSONResponse(shared.TestWorkflow)) reg.Register( - httpmock.REST("GET", "runs/3/jobs"), - httpmock.JSONResponse(shared.JobsPayload{TotalCount: len(jobs), Jobs: firstPage})) - // /runs/:runID/jobs endpoint should be called once more to fetch the remaining jobs + httpmock.QueryMatcher("GET", "runs/3/jobs", url.Values{"per_page": []string{"100"}}), + httpmock.WithHeader( + httpmock.JSONResponse(shared.JobsPayload{TotalCount: len(jobs), Jobs: firstPage}), + "Link", + `; rel="next", ; rel="last"`), + ) reg.Register( httpmock.REST("GET", "runs/3/jobs"), httpmock.JSONResponse(shared.JobsPayload{TotalCount: len(jobs), Jobs: secondPage})) From d5d10a6f38879225bcdeb020b39da2515c6cf8aa Mon Sep 17 00:00:00 2001 From: Harvey Sanders Date: Sun, 3 Sep 2023 20:32:08 -0400 Subject: [PATCH 4/6] Use "Link" header for pagination completion check --- pkg/cmd/run/shared/shared.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/pkg/cmd/run/shared/shared.go b/pkg/cmd/run/shared/shared.go index 3bbfb3726..d5a918776 100644 --- a/pkg/cmd/run/shared/shared.go +++ b/pkg/cmd/run/shared/shared.go @@ -448,9 +448,6 @@ func GetJobs(client *api.Client, repo ghrepo.Interface, run *Run) ([]Job, error) } run.Jobs = append(run.Jobs, resp.Jobs...) - if len(run.Jobs) >= resp.TotalCount { - return run.Jobs, nil - } } return run.Jobs, nil } From 137da73642fc66165abf42faa44269c816af08ac Mon Sep 17 00:00:00 2001 From: Harvey Sanders Date: Sun, 3 Sep 2023 20:32:41 -0400 Subject: [PATCH 5/6] Simplify mock jobs response --- pkg/cmd/run/view/view_test.go | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/pkg/cmd/run/view/view_test.go b/pkg/cmd/run/view/view_test.go index 1bef02fc3..36d563eee 100644 --- a/pkg/cmd/run/view/view_test.go +++ b/pkg/cmd/run/view/view_test.go @@ -1260,18 +1260,6 @@ func TestViewRun(t *testing.T) { ), }, httpStubs: func(reg *httpmock.Registry) { - jobs := []shared.Job{} - for i := 0; i < 115; i++ { - jobs = append(jobs, shared.Job{ - ID: int64(i), - Name: fmt.Sprintf("job %d", i), - }) - } - - limit := 100 - firstPage := jobs[:limit] - secondPage := jobs[limit:] - reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/runs/3"), httpmock.JSONResponse(shared.SuccessfulRun)) @@ -1281,15 +1269,15 @@ func TestViewRun(t *testing.T) { reg.Register( httpmock.QueryMatcher("GET", "runs/3/jobs", url.Values{"per_page": []string{"100"}}), httpmock.WithHeader( - httpmock.JSONResponse(shared.JobsPayload{TotalCount: len(jobs), Jobs: firstPage}), + httpmock.StringResponse(`{"jobs":[{},{},{}]}`), "Link", `; rel="next", ; rel="last"`), ) reg.Register( httpmock.REST("GET", "runs/3/jobs"), - httpmock.JSONResponse(shared.JobsPayload{TotalCount: len(jobs), Jobs: secondPage})) + httpmock.StringResponse(`{"jobs":[{},{}]}`)) }, - wantOut: "fetched 115 jobs\n", + wantOut: "fetched 5 jobs\n", }, } From 6f0f5a89e2538c20cb02b3e1951026f08ea1e082 Mon Sep 17 00:00:00 2001 From: Harvey Sanders Date: Tue, 5 Sep 2023 11:36:33 -0400 Subject: [PATCH 6/6] Use consistent variable naming --- pkg/cmd/run/shared/shared.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/run/shared/shared.go b/pkg/cmd/run/shared/shared.go index d5a918776..cd9d481b8 100644 --- a/pkg/cmd/run/shared/shared.go +++ b/pkg/cmd/run/shared/shared.go @@ -434,17 +434,17 @@ func GetJobs(client *api.Client, repo ghrepo.Interface, run *Run) ([]Job, error) return run.Jobs, nil } - v := url.Values{} - v.Set("per_page", "100") - - jobsPath := fmt.Sprintf("%s?%s", run.JobsURL, v.Encode()) + query := url.Values{} + query.Set("per_page", "100") + jobsPath := fmt.Sprintf("%s?%s", run.JobsURL, query.Encode()) for jobsPath != "" { var resp JobsPayload var err error jobsPath, err = client.RESTWithNext(repo.RepoHost(), http.MethodGet, jobsPath, nil, &resp) if err != nil { - return run.Jobs, err + run.Jobs = nil + return nil, err } run.Jobs = append(run.Jobs, resp.Jobs...)