Conversation
|
I think it is very favorable that this approach clarifies the criteria for which PRs are subject to maintainer review. However, I believe most users, upon seeing that a PR has already been opened, would assume it will be merged in due course and would not think to react to the issue. For users who are unaware of this rule, we need to find a way to communicate that PRs will not be reviewed unless the corresponding issue receives reactions. |
There was a problem hiding this comment.
Does this mean that community reviews would essentially be phased out / no longer serve their role?
There was a problem hiding this comment.
good point. i did not consider community reviews initially but you're right that this proposal disregards them.
in truth, i don't think community reviews has been a popular initiative besides a select few community members who are willing to spend some of their time reviewing. i want to still encourage and reward this behavior, but i'm not sure if the current community review setup is the best way to do that. @badmintoncryer if you have any ideas, i'm happy to hear them! otherwise i'll keep thinking on this.
There was a problem hiding this comment.
Thank you for your response.
As you pointed out, there are not many reviewers who actively engage in community reviews, and I feel that we are not achieving much throughput at the moment. I believe one of the reasons for this is that the benefits and motivation for conducting reviews are difficult to feel.
If this RFC is formalized, I expect the bar for reviews to rise and the number of community-sourced PRs that get merged to decrease. In such a situation, I felt that if PRs approved by community reviewers were made eligible for review without requiring five reactions, it would make it easier for reviewers to get their desired features merged, which in turn could help boost motivation to participate in reviews.
This is just one idea, but I will also talk with community reviewers around me to see if there are any other good suggestions.
There was a problem hiding this comment.
I felt that if PRs approved by community reviewers were made eligible for review without requiring five reactions
+1
PRs approved by a community reviewer should be eligible for maintainer merge regardless of the issue's interaction count. This would serve as a practical mitigation for the strict 5-reaction threshold, reduce the maintainer review burden, and give community reviewers a stronger incentive to stay engaged.
I believe one of the reasons for this is that the benefits and motivation for conducting reviews are difficult to feel.
On the motivation side, moving the community reviewer list from the wiki into a dedicated file in the repository (like CONTRIBUTORS.md) could help. Not many people check the wiki, so most contributors probably don't even know community reviewers exist. More visibility would naturally lead to more recognition and motivation.
There was a problem hiding this comment.
What is the point in reviewing code if its never going to end up in the library anyway. The fundemental issue is not how its labelled.
| Pull requests will inherit the labels of the issue they address. Pull requests without a linked issue | ||
| are by default labeled `incubating`. We will have heavy-handed communication in the Contributing Guide |
There was a problem hiding this comment.
For PRs that are not linked to an issue, would they be promoted to accepting-prs if the PR itself receives more than 5 reactions?
There was a problem hiding this comment.
yes this should be the case. and i hear you that for this proposal to work, we need to make it very clear that reactions on issues are being used to determine impact in this manner. it will take some time for the community to adjust to this new weight that interacting with issues holds
There was a problem hiding this comment.
it will take some time for the community to adjust to this new weight that interacting with issues holds
@kaizencc I reckon the proposal should have a plan for that. We know the RFC process doesn't fit here, but for technical designs we always have a rollout plan. this seems similar.
go-to-k
left a comment
There was a problem hiding this comment.
Thanks for this, @kaizencc. I think this is a good proposal. I like that it makes it clear which issues and PRs should be worked on and which shouldn't.
One concern though. Auto-closing PRs linked to issues that don't reach 5 reactions feels too strict. For example, when a new AWS service feature is released and someone wants to add it to an L2 construct, those issues rarely attract 5 reactions, but they're exactly the kind of contribution that keeps CDK current. My impression is that many of the L2 feature PRs merged in the last few months had fewer than 5 unique individuals on the linked issue.
Or, with CDK Mixins now available, is the intention for users to customize L2 constructs themselves instead?
If not, I think some form of mitigation would help. Also, not being reviewed by maintainers and being auto-closed are two different things. Does auto-closing need to be the default for these PRs?
| 2. A `maintainer-claimed` issue is awaiting maintainer action, a phrase which here means providing | ||
| a workaround or creating a PR. After 14 days of inaction, `maintainer-claimed` issues upgrade to | ||
| `maintainer-response`. |
There was a problem hiding this comment.
Does this mean community members should not submit PRs for maintainer-claimed issues? It would be helpful to state this explicitly to avoid wasted effort on either side, e.g., a community contributor spending time on a fix that conflicts with work a maintainer has already started.
There was a problem hiding this comment.
I felt that if PRs approved by community reviewers were made eligible for review without requiring five reactions
+1
PRs approved by a community reviewer should be eligible for maintainer merge regardless of the issue's interaction count. This would serve as a practical mitigation for the strict 5-reaction threshold, reduce the maintainer review burden, and give community reviewers a stronger incentive to stay engaged.
I believe one of the reasons for this is that the benefits and motivation for conducting reviews are difficult to feel.
On the motivation side, moving the community reviewer list from the wiki into a dedicated file in the repository (like CONTRIBUTORS.md) could help. Not many people check the wiki, so most contributors probably don't even know community reviewers exist. More visibility would naturally lead to more recognition and motivation.
This is a request for comments about {RFC_DESCRIPTION}. See #877 for
additional details.
APIs are signed off by @mrgrain.
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache-2.0 license