diff --git a/pkg/cmd/build/list.go b/pkg/cmd/build/list.go index 72823962..61c01ba8 100644 --- a/pkg/cmd/build/list.go +++ b/pkg/cmd/build/list.go @@ -25,6 +25,9 @@ const ( pageSize = 100 ) +var DisplayBuildsFunc = displayBuilds +var ConfirmFunc = io.Confirm + type buildListOptions struct { pipeline string since string @@ -36,6 +39,7 @@ type buildListOptions struct { commit string message string limit int + noLimit bool } func NewCmdBuildList(f *factory.Factory) *cobra.Command { @@ -115,8 +119,10 @@ func NewCmdBuildList(f *factory.Factory) *cobra.Command { // Get pipeline from persistent flag opts.pipeline, _ = cmd.Flags().GetString("pipeline") - if opts.limit > maxBuildLimit { - return fmt.Errorf("limit cannot exceed %d builds (requested: %d)", maxBuildLimit, opts.limit) + if !opts.noLimit { + if opts.limit > maxBuildLimit { + return fmt.Errorf("limit cannot exceed %d builds (requested: %d); if you need more, use --no-limit", maxBuildLimit, opts.limit) + } } if opts.creator != "" && isValidEmail(opts.creator) { @@ -138,28 +144,21 @@ func NewCmdBuildList(f *factory.Factory) *cobra.Command { } org := f.Config.OrganizationSlug() - var builds []buildkite.Build - - err = io.SpinWhile("Loading builds", func() { - builds, err = fetchBuilds(cmd.Context(), f, org, opts, listOpts) - }) + builds, err := fetchBuilds(cmd, f, org, opts, listOpts, format) if err != nil { return fmt.Errorf("failed to list builds: %w", err) } - if opts.duration != "" || opts.message != "" { - builds, err = applyClientSideFilters(builds, opts) - if err != nil { - return fmt.Errorf("failed to apply filters: %w", err) - } - } - if len(builds) == 0 { fmt.Fprintln(cmd.OutOrStdout(), "No builds found matching the specified criteria.") return nil } - return displayBuilds(cmd, builds, format) + if format == output.FormatText { + return nil + } + + return DisplayBuildsFunc(cmd, builds, format, false) }, } @@ -177,6 +176,7 @@ func NewCmdBuildList(f *factory.Factory) *cobra.Command { cmd.Flags().StringVar(&opts.commit, "commit", "", "Filter by commit SHA") cmd.Flags().StringVar(&opts.message, "message", "", "Filter by message content") cmd.Flags().IntVar(&opts.limit, "limit", 50, fmt.Sprintf("Maximum number of builds to return (max: %d)", maxBuildLimit)) + cmd.Flags().BoolVar(&opts.noLimit, "no-limit", false, "Fetch all builds (overrides --limit)") output.AddFlags(cmd.Flags()) cmd.Flags().SortFlags = false @@ -256,20 +256,77 @@ func buildListOptionsFromFlags(opts *buildListOptions) (*buildkite.BuildsListOpt return listOpts, nil } -func fetchBuilds(ctx context.Context, f *factory.Factory, org string, opts buildListOptions, listOpts *buildkite.BuildsListOptions) ([]buildkite.Build, error) { +func fetchBuilds(cmd *cobra.Command, f *factory.Factory, org string, opts buildListOptions, listOpts *buildkite.BuildsListOptions, format output.Format) ([]buildkite.Build, error) { + ctx := cmd.Context() var allBuilds []buildkite.Build - for page := 1; len(allBuilds) < opts.limit; page++ { + // Track whether we've displayed any builds yet (for header logic) + printedAny := false + + // filtered builds added since last confirm (used when --no-limit) + filteredSinceConfirm := 0 + + // raw (unfiltered) build counters so progress messaging makes sense when client-side filters are active + rawTotalFetched := 0 + rawSinceConfirm := 0 + previousPageFirstBuildNumber := 0 + + for page := 1; ; page++ { + if !opts.noLimit && len(allBuilds) >= opts.limit { + break + } + listOpts.Page = page - listOpts.PerPage = min(pageSize, opts.limit-len(allBuilds)) var builds []buildkite.Build var err error + spinnerMsg := "Loading builds (" if opts.pipeline != "" { - builds, err = getBuildsByPipeline(ctx, f, org, opts.pipeline, listOpts) + spinnerMsg += fmt.Sprintf("pipeline %s, ", opts.pipeline) + } + filtersActive := opts.duration != "" || opts.message != "" + + // Show matching (filtered) counts and raw counts independently + if !opts.noLimit && opts.limit > 0 { + spinnerMsg += fmt.Sprintf("%d/%d matching, %d raw fetched", len(allBuilds), opts.limit, rawTotalFetched) } else { - builds, _, err = f.RestAPIClient.Builds.ListByOrg(ctx, org, listOpts) + spinnerMsg += fmt.Sprintf("%d matching, %d raw fetched", len(allBuilds), rawTotalFetched) + } + spinnerMsg += ")" + + if format == output.FormatText && rawSinceConfirm >= maxBuildLimit { + var confirmed bool + prompt := fmt.Sprintf("Fetched %d more builds (%d total). Continue?", rawSinceConfirm, rawTotalFetched) + if filtersActive { + prompt = fmt.Sprintf( + "Fetched %d raw builds (%d matching, %d matching total). Continue?", + rawSinceConfirm, filteredSinceConfirm, len(allBuilds), + ) + } + + if err := ConfirmFunc(&confirmed, prompt); err != nil { + return nil, err + } + + if !confirmed { + return allBuilds, nil + } + + filteredSinceConfirm = 0 + rawSinceConfirm = 0 + } + + spinErr := io.SpinWhile(spinnerMsg, func() { + if opts.pipeline != "" { + builds, err = getBuildsByPipeline(ctx, f, org, opts.pipeline, listOpts) + } else { + builds, _, err = f.RestAPIClient.Builds.ListByOrg(ctx, org, listOpts) + } + }) + + if spinErr != nil { + return nil, spinErr } if err != nil { @@ -280,9 +337,59 @@ func fetchBuilds(ctx context.Context, f *factory.Factory, org string, opts build break } - allBuilds = append(allBuilds, builds...) + // Track raw builds fetched before applying client-side filters + rawCountThisPage := len(builds) + rawTotalFetched += rawCountThisPage + rawSinceConfirm += rawCountThisPage + + // Detect duplicate first build number between pages to prevent infinite loop + if page > 1 && len(builds) > 0 { + currentPageFirstBuildNumber := builds[0].Number + if currentPageFirstBuildNumber == previousPageFirstBuildNumber { + return nil, fmt.Errorf("API returned duplicate results, stopping to prevent infinite loop") // We should never get here + } + } + + if len(builds) > 0 { + previousPageFirstBuildNumber = builds[0].Number + } + + builds, err = applyClientSideFilters(builds, opts) + if err != nil { + return nil, fmt.Errorf("failed to apply filters: %w", err) + } - if len(builds) < listOpts.PerPage { + // Decide which builds will actually be added (respect limit) + var buildsToAdd []buildkite.Build + addedThisPage := 0 + if !opts.noLimit { + remaining := opts.limit - len(allBuilds) + if remaining <= 0 { // safety, though we check at loop top + break + } + if len(builds) > remaining { + buildsToAdd = builds[:remaining] + addedThisPage = remaining + } else { + buildsToAdd = builds + addedThisPage = len(builds) + } + } else { + buildsToAdd = builds + addedThisPage = len(builds) + } + + // Stream only the builds we are about to add; header only once we actually print something + if format == output.FormatText && DisplayBuildsFunc != nil && len(buildsToAdd) > 0 { + showHeader := !printedAny + _ = DisplayBuildsFunc(cmd, buildsToAdd, format, showHeader) + printedAny = true + } + + allBuilds = append(allBuilds, buildsToAdd...) + filteredSinceConfirm += addedThisPage + + if rawCountThisPage < listOpts.PerPage { break } } @@ -380,7 +487,7 @@ func applyClientSideFilters(builds []buildkite.Build, opts buildListOptions) ([] return result, nil } -func displayBuilds(cmd *cobra.Command, builds []buildkite.Build, format output.Format) error { +func displayBuilds(cmd *cobra.Command, builds []buildkite.Build, format output.Format, withHeader bool) error { if format != output.FormatText { return output.Write(cmd.OutOrStdout(), builds, format) } @@ -399,23 +506,25 @@ func displayBuilds(cmd *cobra.Command, builds []buildkite.Build, format output.F var buf strings.Builder - header := lipgloss.NewStyle().Bold(true).Underline(true).Render("Builds") - buf.WriteString(header) - buf.WriteString("\n\n") - - headerRow := fmt.Sprintf("%-*s %-*s %-*s %-*s %-*s %-*s %s", - numberWidth, "Number", - stateWidth, "State", - messageWidth, "Message", - timeWidth, "Started (UTC)", - timeWidth, "Finished (UTC)", - durationWidth, "Duration", - "URL") - buf.WriteString(lipgloss.NewStyle().Bold(true).Render(headerRow)) - buf.WriteString("\n") - totalWidth := numberWidth + stateWidth + messageWidth + timeWidth*2 + durationWidth + columnSpacing - buf.WriteString(strings.Repeat("-", totalWidth)) - buf.WriteString("\n") + if withHeader { + header := lipgloss.NewStyle().Bold(true).Underline(true).Render("Builds") + buf.WriteString(header) + buf.WriteString("\n\n") + + headerRow := fmt.Sprintf("%-*s %-*s %-*s %-*s %-*s %-*s %s", + numberWidth, "Number", + stateWidth, "State", + messageWidth, "Message", + timeWidth, "Started (UTC)", + timeWidth, "Finished (UTC)", + durationWidth, "Duration", + "URL") + buf.WriteString(lipgloss.NewStyle().Bold(true).Render(headerRow)) + buf.WriteString("\n") + totalWidth := numberWidth + stateWidth + messageWidth + timeWidth*2 + durationWidth + columnSpacing + buf.WriteString(strings.Repeat("-", totalWidth)) + buf.WriteString("\n") + } for _, build := range builds { message := build.Message diff --git a/pkg/cmd/build/list_test.go b/pkg/cmd/build/list_test.go index 7a813553..89ed4df5 100644 --- a/pkg/cmd/build/list_test.go +++ b/pkg/cmd/build/list_test.go @@ -1,10 +1,21 @@ package build import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" "testing" "time" + "github.com/buildkite/cli/v3/internal/config" + "github.com/buildkite/cli/v3/pkg/cmd/factory" + "github.com/buildkite/cli/v3/pkg/output" buildkite "github.com/buildkite/go-buildkite/v4" + "github.com/spf13/afero" + "github.com/spf13/cobra" ) func TestFilterBuilds(t *testing.T) { @@ -44,3 +55,228 @@ func TestFilterBuilds(t *testing.T) { t.Errorf("Expected 1 build with 'Fast', got %d", len(filtered)) } } + +type fakeTransport struct { + pages [][]buildkite.Build + perPageRequested []int +} + +func (ft *fakeTransport) RoundTrip(req *http.Request) (*http.Response, error) { + q := req.URL.Query() + page := 1 + if p := q.Get("page"); p != "" { + fmt.Sscanf(p, "%d", &page) + } + perPage := 0 + if pp := q.Get("per_page"); pp != "" { + fmt.Sscanf(pp, "%d", &perPage) + } + ft.perPageRequested = append(ft.perPageRequested, perPage) + idx := page - 1 + var data []byte + if idx < 0 || idx >= len(ft.pages) { + data = []byte("[]") + } else { + b, _ := json.Marshal(ft.pages[idx]) + data = b + } + resp := &http.Response{ + StatusCode: 200, + Body: io.NopCloser(bytes.NewReader(data)), + Header: make(http.Header), + Request: req, + } + resp.Header.Set("Content-Type", "application/json") + return resp, nil +} + +func TestFetchBuildsLimitSlicing(t *testing.T) { + origDisplay := DisplayBuildsFunc + defer func() { DisplayBuildsFunc = origDisplay }() + type call struct { + buildCount int + withHeader bool + } + var calls []call + DisplayBuildsFunc = func(cmd *cobra.Command, builds []buildkite.Build, format output.Format, withHeader bool) error { + calls = append(calls, call{buildCount: len(builds), withHeader: withHeader}) + return nil + } + all := make([]buildkite.Build, 0, 250) + for i := 0; i < 250; i++ { + all = append(all, buildkite.Build{Number: i + 1}) + } + perPage := 100 + pages := [][]buildkite.Build{ + all[0:perPage], + all[perPage : 2*perPage], + all[2*perPage:], + } + + ft := &fakeTransport{pages: pages} + + apiClient, err := buildkite.NewOpts( + buildkite.WithBaseURL("https://api.example.buildkite"), + buildkite.WithHTTPClient(&http.Client{Transport: ft}), + ) + if err != nil { + t.Fatalf("failed to create api client: %v", err) + } + + fs := afero.NewMemMapFs() + conf := config.New(fs, nil) + conf.SelectOrganization("uber", true) + + f := &factory.Factory{Config: conf, RestAPIClient: apiClient} + + opts := buildListOptions{pipeline: "my-pipeline", limit: 230} + listOpts := &buildkite.BuildsListOptions{ListOptions: buildkite.ListOptions{PerPage: perPage}} + + cmd := &cobra.Command{} + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + cmd.SetContext(ctx) + builds, err := fetchBuilds(cmd, f, conf.OrganizationSlug(), opts, listOpts, output.FormatText) + if err != nil { + t.Fatalf("fetchBuilds returned error: %v", err) + } + if len(builds) != 230 { + t.Fatalf("expected %d builds, got %d", 230, len(builds)) + } + if builds[len(builds)-1].Number != 230 { + t.Fatalf("expected last build number 230, got %d", builds[len(builds)-1].Number) + } + + for i, page := range ft.pages { + if i < len(ft.pages)-1 && len(page) != perPage { + t.Errorf("page %d: expected %d items, got %d", i+1, perPage, len(page)) + } + } + + for i, pp := range ft.perPageRequested { + if pp != perPage { + t.Errorf("request %d: expected per_page=%d, got %d", i+1, perPage, pp) + } + } + + if len(calls) != len(pages) { + // In text format, display is called per page loaded + // Accept that last page may have been truncated by slicing limit + // So allow len(calls) to equal number of pages actually iterated (ft.perPageRequested) + if len(calls) != len(ft.perPageRequested) { + // fallback strict failure + t.Fatalf("expected display calls %d or %d, got %d", len(pages), len(ft.perPageRequested), len(calls)) + } + } + if len(calls) > 0 && !calls[0].withHeader { + t.Errorf("expected first display call to have header") + } + for i := 1; i < len(calls); i++ { + if calls[i].withHeader { + t.Errorf("expected only first display call to have header, call %d had header", i+1) + } + } +} + +func setupConfirmationTestEnv(t *testing.T) (*factory.Factory, *cobra.Command, *config.Config) { + t.Helper() + totalBuilds := maxBuildLimit*2 + 100 + all := make([]buildkite.Build, 0, totalBuilds) + for i := 0; i < totalBuilds; i++ { + all = append(all, buildkite.Build{Number: i + 1}) + } + pages := [][]buildkite.Build{} + for i := 0; i < len(all); i += pageSize { + end := i + pageSize + if end > len(all) { + end = len(all) + } + pages = append(pages, all[i:end]) + } + ft := &fakeTransport{pages: pages} + apiClient, err := buildkite.NewOpts( + buildkite.WithBaseURL("https://api.example.buildkite"), + buildkite.WithHTTPClient(&http.Client{Transport: ft}), + ) + if err != nil { + t.Fatalf("failed to create api client: %v", err) + } + fs := afero.NewMemMapFs() + conf := config.New(fs, nil) + conf.SelectOrganization("uber", true) + f := &factory.Factory{Config: conf, RestAPIClient: apiClient} + cmd := &cobra.Command{} + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + cmd.SetContext(ctx) + return f, cmd, conf +} + +func TestFetchBuildsConfirmationDeclineFirst(t *testing.T) { + origDisplay := DisplayBuildsFunc + DisplayBuildsFunc = func(cmd *cobra.Command, builds []buildkite.Build, format output.Format, withHeader bool) error { + return nil + } + t.Cleanup(func() { DisplayBuildsFunc = origDisplay }) + + f, cmd, conf := setupConfirmationTestEnv(t) + opts := buildListOptions{pipeline: "my-pipeline", noLimit: true} + listOpts := &buildkite.BuildsListOptions{ListOptions: buildkite.ListOptions{PerPage: pageSize}} + + confirmCalls := 0 + origConfirm := ConfirmFunc + ConfirmFunc = func(confirmed *bool, title string) error { + confirmCalls++ + *confirmed = false + return nil + } + t.Cleanup(func() { ConfirmFunc = origConfirm }) + + builds, err := fetchBuilds(cmd, f, conf.OrganizationSlug(), opts, listOpts, output.FormatText) + if err != nil { + t.Fatalf("fetchBuilds returned error: %v", err) + } + if confirmCalls != 1 { + t.Fatalf("expected 1 confirmation call, got %d", confirmCalls) + } + if len(builds) != maxBuildLimit { + t.Fatalf("expected %d builds when declined, got %d", maxBuildLimit, len(builds)) + } +} + +func TestFetchBuildsConfirmationAcceptThenDecline(t *testing.T) { + origDisplay := DisplayBuildsFunc + DisplayBuildsFunc = func(cmd *cobra.Command, builds []buildkite.Build, format output.Format, withHeader bool) error { + return nil + } + t.Cleanup(func() { DisplayBuildsFunc = origDisplay }) + + f, cmd, conf := setupConfirmationTestEnv(t) + opts := buildListOptions{pipeline: "my-pipeline", noLimit: true} + listOpts := &buildkite.BuildsListOptions{ListOptions: buildkite.ListOptions{PerPage: pageSize}} + + confirmCalls := 0 + origConfirm := ConfirmFunc + ConfirmFunc = func(confirmed *bool, title string) error { + confirmCalls++ + if confirmCalls == 1 { + *confirmed = true + } else { + *confirmed = false + } + return nil + } + t.Cleanup(func() { ConfirmFunc = origConfirm }) + + builds, err := fetchBuilds(cmd, f, conf.OrganizationSlug(), opts, listOpts, output.FormatText) + if err != nil { + t.Fatalf("fetchBuilds returned error: %v", err) + } + expected := maxBuildLimit * 2 + if len(builds) != expected { + t.Fatalf("expected %d builds after accepting once then declining, got %d", expected, len(builds)) + } + if confirmCalls != 2 { + t.Fatalf("expected 2 confirmation calls, got %d", confirmCalls) + } +}