upsidr / merge-gatekeeper

Get better merge control
MIT License
85 stars 14 forks source link

Fix pagination for PRs having >100 checks #59

Closed markrmullan closed 1 year ago

markrmullan commented 1 year ago

The pagination logic for fetching checks on PRs is incorrect, and on PRs having >100 checks (aka the current maxStatusesPerPage value) merge-gatekeeper gets stuck in an infinite loop until it typically fails due to hitting Github's rate limit. This PR fixes that logic to paginate status check requests properly.

rytswd commented 1 year ago

I'm so sorry for getting back to this so late, and thanks for the PR @markrmullan! πŸ™

The change itself looks good, but would you be able to also add a test case to ensure this fix actually works as intended?

markrmullan commented 1 year ago

I'm so sorry for getting back to this so late, and thanks for the PR @markrmullan! πŸ™

The change itself looks good, but would you be able to also add a test case to ensure this fix actually works as intended?

No problem @rytswd , thanks for your response! Will look into adding some tests today. Golang is not my strength, so please feel free to rip them apart, no offense will be taken πŸ™‚

markrmullan commented 1 year ago

I'm so sorry for getting back to this so late, and thanks for the PR @markrmullan! πŸ™

The change itself looks good, but would you be able to also add a test case to ensure this fix actually works as intended?

Hey there @rytswd ! Would you mind taking a second look at this, time permitting on your end? Thanks again πŸ™

rytswd commented 1 year ago

Hi @markrmullan, absolutely, I've been looking into this just now πŸ˜‰ Give me some time to go through all ☺️

markrmullan commented 1 year ago

Thank you very much for your work! πŸ₯° This was meant to be something we added test cases to capture all possible scenarios, but it just looks like the test case was implemented incorrectly πŸ₯²

Also, as I understood from your comment that you are not so versed with Go, I have added some extra notes about Go coding for future references -- I hope you don't get offended πŸ™ƒπŸ™

I am not versed in Go, so your notes are highly appreciated, thank you for such a thorough review, and will address these comments today!

markrmullan commented 1 year ago

Thank you very much for your work! πŸ₯° This was meant to be something we added test cases to capture all possible scenarios, but it just looks like the test case was implemented incorrectly πŸ₯²

Also, as I understood from your comment that you are not so versed with Go, I have added some extra notes about Go coding for future references -- I hope you don't get offended πŸ™ƒπŸ™

Again, thank you very much for all of your help and patience with a Golang newb! This is ready for another passthrough, as time permits on your end πŸ™‚

rytswd commented 1 year ago

As all test cases are passing correctly with the fixed implementation, I'm merging this to get it into main branch. We usually have it sit in main branch while we use and test it internally -- if no issue found in a few days, we will cut a new release -- this one would be a patch release πŸ‘

Thanks again, @markrmullan! πŸ₯°

markrmullan commented 1 year ago

As all test cases are passing correctly with the fixed implementation, I'm merging this to get it into main branch. We usually have it sit in main branch while we use and test it internally -- if no issue found in a few days, we will cut a new release -- this one would be a patch release πŸ‘

Thanks again, @markrmullan! πŸ₯°

Thank you again for all of your work on this, @rytswd !

markrmullan commented 1 year ago

Hi there @rytswd ! Do you feel comfortable cutting a release for this change?

rytswd commented 1 year ago

@markrmullan Thanks for the reminder, I have just released the new version v1.2.1! πŸ₯°

markrmullan commented 1 year ago

@markrmullan Thanks for the reminder, I have just released the new version v1.2.1! πŸ₯°

Thank you very much, again!