Allow authors of components to review PRs to accelerate maintainer approval

[CodeMonkeyLeet]

Describe the proposal

Per dapr/dapr#3378 (comment), to make the PR review and approval process more efficient with the currently limited number of maintainers, it would be helpful to have the authors/owners of components review PRs first with their domain expertise, and only require a final approval from maintainers to merge an approved change so that they maintain their gatekeeping authority. This can be achieved with existing GitHub mechanisms as follows:

1. Update the branch protection policy to require code owner approval if that's not already the case.
2. Add more reviewers to dapr project as collaborators so that they can review and approve PRs, but do not add them to CODEOWNERS yet. This allows reviewers to iterate with the PR contributors until there's an initial approval and takes some load off the existing maintainers.
   • It doesn't change the requirement that maintainers are still needed to approve the PR for merge, but potentially reduces the wait time for when a PR is submitted and is getting feedback from a reviewer at least.
   • The CODEOWNERS file current includes both @dapr/maintainers-components-contrib and @dapr/approvers-components-contrib with significant overlap in membership between the two.
     • If the desire is that PRs must have maintainer approval to be merged, then @dapr/approvers-components-contrib membership should be reconciled against the maintainers group and dropped from CODEOWNERS.

The downside is that this is an unintuitive use of CODEOWNERS, and approvers not included in CODEOWNERS will not be automatically notified of PRs that require their review, though that can be worked around with GitHub apps and bots. It also precludes using the built-in CODEOWNERS file for automatic folder-level assignment of PR reviews, but ideally, this would only be temporary and approvers should be added to the CODEOWNERS file specifically for the folder they are allowed to merge without maintainer intervention after some probation period.

An additional consideration would be to have a cron job in GitHub workflows that checks for merge access in CODEOWNERS and report to maintainers if there are any approvers that have not been active in a long time.

[CodeMonkeyLeet]

@artursouza As requested, adding this proposal for your reference.

[tanvigour]

What would be the criteria to add someone as a collaborator to the Dapr project? Is that something additional to them having read/write permission to the repo- for instance demonstration of interest/expertise in the repo area?

[yaron2]

This seems ok-ish and needs further discussion, however I do not approve of this:

specifically for the folder they are allowed to merge without maintainer intervention after some probation period.

Granular code owners has been tried and tested in a great many projects and has proven to become an extremely difficult practice, especially in components contrib as you'd have the number of collaborators potentially equal to the number of components. This usually works for very static parts of a single repository.

[yaron2]

Actually, thinking about this more, this concept of collaborators feels like a hack "during times of panic" that we wouldn't normally do if we had an adequate number of approvers and maintainers, which to me raises a flag.

If the problem we are trying to solve is help accelerate maintainer approval on components-contrib (which I whole heartedly agree with), I propose to solve this via the existing and established membership patterns instead of coming up with GitHub membership hacks to allow for some sort of new "in-between" state that's applicable to a single repository to solve a point in time issue that may not be needed once more approvers/maintainers onboard.

More specifically, I recommend we establish organization level practices and recommendations to help deliver better efficiency.

/cc @artursouza

[CodeMonkeyLeet]

What would be the criteria to add someone as a collaborator to the Dapr project? Is that something additional to them having read/write permission to the repo- for instance demonstration of interest/expertise in the repo area?

@tanvigour To clarify, this would be anyone designated as an approver according to the existing community membership guidelines. The GitHub collaborator status is a mechanism for access to actions I would expect from an approver which generally encompasses read/write privileges without needing to be a project member, and could be replaced with a proper access groups (such as membership in @dapr/approvers-components-contrib, which is preferable if the groups are appropriately factored).

[artursouza]

I think we can have a concept of component owner to allow maintainers and approvers to consider their review first. IMO, this should be a recommendation and not an authoritative reviewer.

[CodeMonkeyLeet]

More specifically, I recommend we establish organization level practices and recommendations to help deliver better efficiency.

@yaron2 I think that's a great idea; taking a step back from the mechanical details of the proposal, let me try to characterize what I'm hearing as the problem to address from @artursouza, which was not captured in this issue:

• The current bar for moving from Contributor to Approver is high, and Dapr is not promoting enough approvers to keep up with PR volume.
• The issue is particularly prominent in components-contrib where the burden of thorough technical reviews spanning numerous technologies is high when existing Approvers/Maintainers may not be fluent in those technologies.
• It would be useful to be able to distribute the responsibility of PRs to a broader pool of community component experts, who can contribute to core technical reviews, and allow the Approvers/Maintainers to focus on architectural/ecosystem/conformance issues at the dapr level.

One thing that the original proposal misses based on #1046 (comment) is the suggestion that we should define a new category of contributors, Component Owners, in addition to Approvers and Maintainers (I was under the impression that Component Owners should always become Approvers over time and that is not the expectation.) A couple of properties I'm inferring from the discussion:

• Component Owners are a concept narrowly defined for components-contrib for the purposes of this discussion (though it could be expanded if it proves useful to other repos as feature/area owners).
• Component Owners could be the author(s) of the component or any contributor(s) currently maintaining it.
  • Technically, we already have a concept of a component maintainer as defined for a GA component, who may not be a more general Dapr Approver/Maintainer, it would be good to reconcile that definition with a set of GitHub rights and responsibilities that are being discussed for a Component Owner.
• Component Owners should be the first line of review for component PRs, and preferably have the ability to indicate this in GitHub by approving the PR.
  • Component Owners may not be Approvers and do not have the ability to merge PRs. An Approver/Maintainer approval is still required for the PR to be merged.
  • Component Owners should be assigned and notified of PRs in their area by automation.

The top level question is: Does Dapr want to introduce the concept of a Component Owner (someone invested in a component or area but not Dapr more broadly and has specific ownership responsibilities) for this purpose? In other projects, this has analogs to members of Special Interest Groups (SIGs) that can be community lead.

If the answer is yes, then the next question is a refinement of this proposal, which suggests a way to use GitHub features to provide guardrails and automation around the Component Owner role. For example, if we don't want to use the GitHub collaborator mechanism as a hack:

• Component Owners can be defined as a their own security group (e.g. @dapr/component-owners) with read/write access (write access is necessary to approve PRs).
  • Corollary: Component Owners must be members of the Dapr org so that they can be added to the group.
• @dapr/component-owners are excluded from the CODEOWNERS file so their PR approval is insufficient for the PR to be merged.
• dapr-bot is modified or a new workflow is added to assign & notify Component Owners when a PR for their component is pushed.

[artursouza]

Some thoughts on the proposal above:

• Yes, I think Dapr needs a concept of Component Owners and it should be explained in dapr/community.
• No, I don't think Component Owners should have any special access to the repo.
• Yes, component-owners should not be in CODEOWNERS file because their approval is informational to maintainers and not authoritative.
• Yes, Dapr Bot can be changed to understand some sort of special file for component owners to add them as reviewers (COMPONENT_OWNERS file under each component?).
• Yes, maintainers are still the ones approving and merging the PRs in the end.

[yaron2]

Thanks for the expanded explanation @CodeMonkeyLeet.

It would be useful to be able to distribute the responsibility of PRs to a broader pool of community component experts, who can contribute to core technical reviews, and allow the Approvers/Maintainers to focus on architectural/ecosystem/conformance issues at the dapr level.

We can, should and sometimes do utilize Dapr members in general for reviewers in PRs. Every maintainer or approver should be able to mention Dapr Members to review PRs and help. This should not be specific to contrib, but can help there the most right now.

Component Owners should be the first line of review for component PRs

That's what we'd want to happen, but in the majority of cases since Component Owners are not Approvers or Maintainers, they do not have any obligation to check or respond to reviews. If an Approver or Maintainer have bandwidth to review a component PR, they should not wait for the Component Owner to review.

In other projects, this has analogs to members of Special Interest Groups (SIGs) that can be community lead.

SIGs are usually centered around cross-repository concepts. In the Dapr world, there would probably be a pub/sub SIG and a state SIG, centered around building blocks, not around components. But I get what you're saying.

Component Owners should be assigned and notified of PRs in their area by automation.

The thing is, we can't expect someone contributing a component to become a Component Owner. So the best we can do is mention them as FYI for any change to the component they authored.

[CodeMonkeyLeet]

• No, I don't think Component Owners should have any special access to the repo.

To clarify, @artursouza you mean that we don't need the @dapr/components-owner permissions group, correct? I believe you need to at least be a Member to mark a PR as approved (read perms to just leave reviews, write perms if we expect the approval to matter for merging to branches), so the baseline expectation is just that a Component Owner is already a Dapr Member.

[CodeMonkeyLeet]

The thing is, we can't expect someone contributing a component to become a Component Owner. So the best we can do is mention them as FYI for any change to the component they authored.

I don't think the suggestion is that the author of a component is automatically designated as the Component Owner, though we could do that. Reiterating:

• Component Owners could be the author(s) of the component or any contributor(s) currently maintaining it.
  • Technically, we already have a concept of a component maintainer as defined for a GA component, who may not be a more general Dapr Approver/Maintainer, it would be good to reconcile that definition with a set of GitHub rights and responsibilities that are being discussed for a Component Owner.

Echoing #1046 (comment), the concept of a Component Owner would need to be defined and circulated to the community, which includes defining a new set of criteria. As with Approvers and Maintainers, people need to understand how to volunteer for this, and we need to make the bar significantly lower than for becoming an Approver. Since each component needs a component maintainer to reach GA, I would propose making that role synonymous with being a Component Owner discussed here. For example:

• The author of a component or a contributor who has contributed 3 or more fixes can volunteer to be a Component Owner.
• There may be more than one active Component Owner.
• A Component Owner may also be an Approver or Maintainer. They are orthogonal roles.
• An inactive Component Owner may be removed or replaced (e.g. component accumulates > X outstanding stale issues or Y unreviewed PRs).
• A component without a Component Owner is ineligible for GA.
• A GA component that has lost all Component Owners with no replacement may be downgraded to Abandoned/Deprecated.

And yes, I agree it's important to keep in mind that we're not talking about granting community members privileges, we're asking for community members to take on responsibilities, and that's hard. Rather than saying we shouldn't expect folks to step up though, I think that the Dapr org can do more to incentivize that: we need to set clear expectations, reduce the barriers to entry, reduce the friction of performing their task, and provide the recognition for that task. Having a clearly defined role matters for visibility and scope in an open source project, and I think this is a low risk experiment to move towards more community investment in Dapr: in the worst case, it just ends up being the existing case, everything being maintained by a small group of Approvers and Maintainers.

[artursouza]

• No, I don't think Component Owners should have any special access to the repo.

To clarify, @artursouza you mean that we don't need the @dapr/components-owner permissions group, correct? I believe you need to at least be a Member to mark a PR as approved (read perms to just leave reviews, write perms if we expect the approval to matter for merging to branches), so the baseline expectation is just that a Component Owner is already a Dapr Member.

Anyone can approve a PR - if their approval is authoritative or not, is a matter of config in GitHub. I don't think we should add a formal concept of component owner for the reasons Yaron also mentioned here. I do think we can add component owners as reviewers to PRs as a best effort thing. We might even be able to extract that automatically based on the files being modified.

[dapr-bot]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged (pinned, good first issue, help wanted or triaged/resolved) or other activity occurs. Thank you for your contributions.

[dapr-bot]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as pinned, good first issue, help wanted or triaged/resolved. Thank you for your contributions.