search

upsidr / merge-gatekeeper

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

Support more than 30 status #48

Closed Jrc356 closed 1 year ago

Jrc356 commented 1 year ago

We have a monorepo that currently runs 44 jobs on each PR. This leads to a mismatch between the expected amount of statuses and the current completed amount which makes the gatekeeper pass earlier than expected. The issue is that the status API default size is 30. This PR bumps the PerPage size to 100 (max allowed) and adds the necessary page continuation logic to effectively allows for infinite statuses to be retrieved.

rytswd commented 1 year ago

Thank you @Jrc356! I believe this fixes #42. Let us check this further, as we would ideally want to have a test case to ensure this doesn't break the existing code, and also we can handle cases where we have exactly 100, more than 100, and more than 200. As I can see the test cases to be a bit annoying to work out, we may decide to go with this change without such coverage, but would like to get a closer look before deciding to merge this.

Jrc356 commented 1 year ago

Thank you @Jrc356! I believe this fixes #42. Let us check this further, as we would ideally want to have a test case to ensure this doesn't break the existing code, and also we can handle cases where we have exactly 100, more than 100, and more than 200. As I can see the test cases to be a bit annoying to work out, we may decide to go with this change without such coverage, but would like to get a closer look before deciding to merge this.

ah, yep, that was the exact issue I came across as well. Lemme know if/how I can help!

rytswd commented 1 year ago

I found the implementation easy enough to follow, so just need some test cases to prove it's doing what's expected - if you could help with that, I think we can merge this relatively quickly! 🥰

Jrc356 commented 1 year ago

@rytswd I've added some test cases, could you take another look? 🙂

rytswd commented 1 year ago

I'll leave this to @hlts2 to have a final check before merging it 👍

Jrc356 commented 1 year ago

@hlts2 would you be able to review today or Monday? We'd love to use this but can't without this feature

Jrc356 commented 1 year ago

@rytswd I see you created the last release. I'm curious, what is the release cadence for this? Looks like maybe once a month-ish or is there no fixed schedule?

rytswd commented 1 year ago

@Jrc356 As you noticed, we don't have a set schedule for this. We would at least keep the change in the main branch for some time before cutting a release, so that we can use the latest update in our internal repositories. After confirming there has been no degradation or unintended impact, we release it out as a patch/minor version bump.

As this is rather a critical fix for reliability, we will aim to release it sooner than usual - we should have sufficient runs in a day or two, and would cut a release if no issue found 👍

Having said that, I'm going ahead with the merge for this. Huge thanks for your contribution! 🥰