Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(merge-on-green)!: remove restrictions for checking reviews for merge on green #5231

Merged
merged 9 commits into from
Jun 3, 2024

Conversation

sofisl
Copy link
Contributor

@sofisl sofisl commented Sep 19, 2023

When we initially created merge-on-green, we weren't sure whether repos had set up branch protection adequately, so we wrote additional logic to make sure PRs had reviews. However, we've since instituted org-wide protection, this seems like an additional piece of logic now that causes a lot of confusion, see: #5193

Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

Main change seems ok, but a nit about the test

@@ -422,7 +422,7 @@ describe('merge-on-green wrapper logic', () => {

const [reducedPRs] = handler.maybeReducePRList(prs);

assert.strictEqual(reducedPRs.length, 25);
assert.ok(reducedPRs.length === 24 || reducedPRs.length === 25);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be confident in this value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's OK if we're not confident. It's produced by a random value, and the only requirement is that it has to be under 25 (the chunk size):

const index = Math.round(Math.random() * prs.length);
. I've updated the test to reflect that.

@sofisl sofisl requested a review from chingor13 October 20, 2023 19:06
@sofisl sofisl changed the title fix!: remove restrictions for checking reviews for merge on green fix(merge-on-green)!: remove restrictions for checking reviews for merge on green Nov 21, 2023
@sofisl
Copy link
Contributor Author

sofisl commented Dec 4, 2023

Friendly ping @chingor13

@googleapis googleapis deleted a comment from Iben1993 Jun 3, 2024
@googleapis googleapis deleted a comment from Iben1993 Jun 3, 2024
@chingor13 chingor13 enabled auto-merge (squash) June 3, 2024 17:33
@chingor13 chingor13 merged commit 9040399 into main Jun 3, 2024
20 checks passed
@chingor13 chingor13 deleted the mergeOnGreenRemoveReviews branch June 3, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants