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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

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

@Iben1993
Copy link

Amikor kezdetben az egyesületet--zöldre hoztuk létre, nem voltunk biztosak abban, hogy a repos megfelelően létrehozta-e a fióktelepek védelmét, ezért további logikát írtunk annak érdekében, hogy a PRok legyenek felülvizsgálatok. Azonban, azóta, hogy bevezettük az org-wide védelmet, ez úgy tűnik, mint egy további logika most, amely sok zűrzavart okoz, lásd: #5193

@Iben1993
Copy link

LICENSE

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

3 participants