feat(build:list): add --no-limit option and fix duplicate builds for …

[sbamaraa]

…non-divisible limits

Add --no-limit Option:

Users can now specify --no-limit to fetch all builds that match the provided criteria.
The command will continue fetching until all available builds are retrieved, making it easier to work with large datasets or when the total number of builds is unknown.

Fix for Non-Divisible Limits:

Previously, if the requested limit was not a multiple of the page size (e.g., 230 with a page size of 100), the command could return duplicate builds due to overlapping page boundaries.
The fetching logic has been corrected to ensure that only unique builds are returned, and the total count matches the requested limit.

Acknowledge logic from user when fetching more than certain number of requests.

When user fetches maxBuildLimit number of builds through the API, now it asks acknowledgement.

Loading spanner has more info

Now it shows how many raw builds have been fetched + how many actually matches the client side filter.

Builds are displayed as soon as it is fetched

Instead of displaying all the builds all at once after the command is finished, now it prints them immediately.

[JoeColeman95]

Hey @sbamaraa,

Thanks for the PR!

One thing that stands out here is the removal of page size on this – which may have a negative impact on the desired outcome of this PR.

By default, the Buildkite API will return 30 builds if no page size is specified, with a maximum of 100 builds per page. This can be found in our docs here: https://buildkite.com/docs/apis/rest-api#pagination

Looking into the PR, the flag in the helper states it sets this to 500, but I can't see this being done anywhere, with the page limit being stripped entirely, defaulting the results to 30 results per page when --no-limit is, or isn't used.

It may be worth simply tweaking the const for maxBuildLimit as that seems to be the main issue you're facing? (Other than dupes in results) The rationale would be to prevent any potential infinite loops here, and to also preserve some form of throttling.

Not opposed to --no-limit entirely, but I am concerned about the infinite loop in it's current state. It's just the page size being removed that I'm flagging primarily.

[eaugene]

@sbamaraa I don't know how difficult it is to implement
instead of --no-limit option , can we process to some MaxBuild records and then ask ack from user to get more data and process ?
@JoeColeman95 thoughts about this approach to avoid infinite looping fetching all builds

[sbamaraa]

@sbamaraa I don't know how difficult it is to implement instead of --no-limit option , can we process to some MaxBuild records and then ask ack from user to get more data and process ? @JoeColeman95 thoughts about this approach to avoid infinite looping fetching all builds.

Would be happy to implement that logic, again any thoughts on it @JoeColeman95?

[JoeColeman95]

@sbamaraa - apologies, I was out of office yesterday. I will take a further look at this PR for you today!

[JoeColeman95]

@sbamaraa - I've added pagination back with my suggestions and reworked the logic slightly to add infinite loop prevention measures, and removed the 500 page size entry on the flag. The bulk of your changes were already great, so they remain untouched.

With some testing on my end, I can see that this is achieving exactly what you set out for based off of the PR description, if you could ensure that you're happy with the suggested changes? If so, commit them and request a re-review and I'll approve.

Otherwise, feel free to make any change(s) and I can re-review

[sbamaraa]

I have made more changes to the PR :), additionally now it prints the results immediately after fetching. it helps debug much efficiently.

Also bunch of counter have been added to the "Loading spinner"

[sbamaraa]

I have made more changes to the PR :), additionally now it prints the results immediately after fetching. it helps debug much efficiently.

Also bunch of counter have been added to the "Loading spinner"

I really need to polish this PR before you reviewing it.

Update: has been polished now. Please review it.

[JoeColeman95]

Hey @sbamaraa,

Looks great! 🙂

The only thing we need here is the PageSize setting to 100 using the constant we have, as this will still use the default page size of 30 builds which should improve performance. We're getting a lint error due to the unchecked usage of Spinner:

pkg/cmd/build/list.go:320:15: Error return value of `io.SpinWhile` is not checked (errcheck)

[sbamaraa]

Thanks for the review! ✅

• The lint issue is resolved - the return value from io.Spinwhile is now checked
• In buildListOptionsFromFlags, PerPage is always set to pageSize constant which is 100 :)

If no more comments, we can merge it :)

[JoeColeman95]

Thanks for the contribution @sbamaraa 😄