Skip to content

feat: Look at 100 recent builds instead of just 30. - #73

Merged
coreyja merged 1 commit into
coreyja:masterfrom
temochka:moar-builds
Mar 7, 2020
Merged

feat: Look at 100 recent builds instead of just 30.#73
coreyja merged 1 commit into
coreyja:masterfrom
temochka:moar-builds

Conversation

@temochka

@temochka temochka commented Mar 5, 2020

Copy link
Copy Markdown
Contributor

Hey @coreyja! 👋 thank you for sleet :)

CircleCI limits the number of returned builds to 30 by default, but the
endpoint supports up to 100. When using CircleCI workflows, it's
entirely possible for a single push to trigger more than 30 builds,
which can result in a behavior where sleet will be unable to find some
of the "overflow" builds.

Bumping this to 100 keeps it still possible but much less likely.

A more future-proof solution would be to paginate the builds until a match is found or some artificial limit is reached (probably don’t want this to be infinite), but the existing structure makes this approach a little more difficult to implement without dropping caching. Wonder if you have any thoughts about this.

CircleCI limits the number of returned builds to 30 by default, but the
endpoint supports up to 100. When using CircleCI workflows, it's
entirely possible for a single push to trigger more than 30 builds,
which can result in a behavior where sleet will be unable to find some
of the "overflow" builds.

Bumping this to 100 makes it still possible but much less likely.
@coreyja

coreyja commented Mar 6, 2020

Copy link
Copy Markdown
Owner

Thanks for the PR! Also happy to see that people are using and enjoying Sleet!

This change makes so much sense! I will get this merged and a version cut this evening!

Comment thread lib/sleet/branch.rb
def url
"https://circleci.com/api/v1.1/project/github/#{github_user}/#{github_repo}/tree/#{branch}?filter=completed"
"https://circleci.com/api/v1.1/project/github/#{github_user}/#{github_repo}/tree/#{branch}" \
'?filter=completed&limit=100'

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird that I decided to just throw query params in the URL lol
Always fun to see the decisions you made a few years ago!

Like you breaking the query params to a new line!

@coreyja coreyja left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me, thanks again for the contribution!

@coreyja coreyja changed the title Look at 100 recent builds instead of just 30. feat: Look at 100 recent builds instead of just 30. Mar 7, 2020
@coreyja
coreyja merged commit 1c2fff9 into coreyja:master Mar 7, 2020
@coreyja

coreyja commented Mar 7, 2020

Copy link
Copy Markdown
Owner

@temochka This is out and released as version 0.4.3

Thanks again for the contribution!

@temochka

temochka commented Mar 9, 2020

Copy link
Copy Markdown
Contributor Author

The pleasure is mine. Thank you for the quick turnaround ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants