chore: add PR to R2 list only if there are 2 approvals #33283
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33283 +/- ##
=======================================
Coverage 80.83% 80.83%
=======================================
Files 236 236
Lines 14253 14253
Branches 2490 2490
=======================================
Hits 11522 11522
Misses 2446 2446
Partials 285 285
Flags with carried forward coverage won't be shown. Click here to find out more.
|
aws-cdk-automation
added
the
pr/needs-maintainer-review
| if (approvals.length < 2) { | ||
| console.log(`Observed approval count: ${approvals.length}. Skipping as it is less than 2...`); |
There was a problem hiding this comment.
Should we make it R3 if it only has one approval? Often I think if one community member is reviewing a PR then the others will leave it alone and wait for maintainer
| if (approvals.length < 2) { | ||
| console.log(`Observed approval count: ${approvals.length}. Skipping as it is less than 2...`); |
There was a problem hiding this comment.
I'm thinking should we consider checking approval based on a maintainer list instead of just counting approvals? This would prevent PRs from being added to R2 when they have multiple community approvals but no maintainer approval with failing checks.
We could use an approach similar to mergify configuration where:
- Maintain maintainer authors list in one common place
- Check if approval is from someone in this list
- Only add to R2 if a maintainer has approved and checks are failing
For approved PR's from community and needs maintainer review with failing status check will be added to R3 list if I'm not wrong.
Let me know if it makes sense.
There was a problem hiding this comment.
Thought about the same but as I went through a few PRs (very few as I was manually going through them), counting 2 approvals should cover most cases. Because of that, I didn't want to make this too complicated.
There was a problem hiding this comment.
Thanks for checking. Should be good then if most of the PRs falls under this behaviour.
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
aws-cdk-automation
removed
the
pr/needs-maintainer-review
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
This pull request has been removed from the queue for the following reason: The merge conditions cannot be satisfied due to failing checks You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. If you want to requeue this pull request, you need to post a comment with the text: |
|
@Mergifyio requeue |
✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
N/A
Description of changes
Add to R2 list if there are 2 or more approvals on the PR
Description of how you validated changes
unit tests
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license