chore(prlint): apply needs-maintainer-review label when PR is assigned #27770
Conversation
github-actions
bot
added
the
star-contributor
PRs can only be assigned by members with write access. If a PR has been assigned to someone, remove the `needs-community-review` label (since it implies a maintainer is already working on that PR) and apply the `needs-maintainer-review` label. In order to facilitate this, the `pull_request_target` event should also be run when a PR is assigned/unassigned.
laurelmay
force-pushed
the
prlint-maintainer-review-for-assigned
branch
from
2634115 to
15415b8
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
aws-cdk-automation
added
the
pr/needs-community-review
There was a problem hiding this comment.
Hi @kylelaker -- thanks for your interest in a niche part of our tooling system :). I'm not 100% sold on the implication that assigning a maintainer to a PR means we are not open to a community review. This necessitates a conversation on what a maintainer assignment implies. I argue that if I assign myself to a PR I am committing to reviewing the PR, but I wouldn't be against having a community review to help me out. (I also don't really assign myself to PRs anymore, and there's not currently an internal meaning attached to assignment beyond "dibs"). My worry is that if a maintainer assigns themself to a PR and then forgets about it, then suddenly that PR is ineligible for community review because of an oversight. That being said, I can see how assignment also means that a maintainer review is imminent.
Is this PR in response to an instance or two where your effort was wasted due to an assignment? I'm inclined to just approve this PR but I would love to be convinced further by a clear need.
| // 2) is already community approved | ||
| // 3) is authored by a core team member | ||
| if (readyForReview && (fixesP1 || communityApproved || pr.labels.some(label => label.name === 'contribution/core'))) { | ||
| // 1) is assigned |
There was a problem hiding this comment.
| // 1) is assigned | |
| // 1) is assigned to core team member |
Honestly I've just been seeing a few in https://github.com/aws/aws-cdk/pulls?q=is%3Apr+is%3Aopen+label%3Apr%2Fneeds-community-review that are assigned and didn't want to step on toes 😄 So if this isn't wanted or provides unclear value, definitely happy to close it! I meant to start a conversation around it before opening the PR but for some reason ran |
|
Hey @kylelaker, got some feedback internally that there is no reason to take the label off in these cases. We still welcome our trusted community input, and the risk of PRs falling through the cracks due to this mechanism is not low. In general though, I am very happy to entertain ideas on how to streamline our processes, which we can discuss over on cdk.dev as well. Closing this for now. |
PRs can only be assigned by members with write access. If a PR has been assigned to someone, remove the
needs-community-reviewlabel (since it implies a maintainer is already working on that PR) and apply theneeds-maintainer-reviewlabel. In order to facilitate this, thepull_request_targetevent should also be run when a PR is assigned/unassigned.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license