Role based maintainers for recipes in meta-qcom-hwe

[sbanerjee-quic]

Identify Maintainers for specific recipe sub-directories.
The maintainers to be notified when PR is raised in specific recipe sub-directory.

[mynameistechno]

@sbanerjee-quic do these users need ability to merge PRs as well or just review and triage PRs and Issues?

I.e. for the dozen or so users identified, would it be sufficient to give them the triage role? This role can manage issues, discussions, and pull requests without write access. That is, they can be assigned PRs, assign them to others, review them, etc but not merge. Once they approve, then a smaller subset of maintainers such as yourself and Ricardo would be able to merge.

This is the new model we have been advocating for, i.e. fewer users with the Maintainer/Write role, and more users with Triage role. Let me know if any concerns.

cc: @ricardosalveti for thoughts

[mynameistechno]

I can look into the CODEOWNERS file so they get automatically assigned

[ricardosalveti]

I would prefer the merge to be done by the main repo maintainers, and having the tech (via sub-folders) maintainers be just extra reviewers (which the main repo maintainers would need to be aligned with from the changes pov). The main idea was to make sure the tech teams were involved as part of the review process as we move the project forward.

[ricardosalveti]

I can look into the CODEOWNERS file so they get automatically assigned

Yeah, that would be more aligned to what I was thinking as well.

[sbanerjee-quic]

@sbanerjee-quic do these users need ability to merge PRs as well or just review and triage PRs and Issues?

I.e. for the dozen or so users identified, would it be sufficient to give them the triage role? This role can manage issues, discussions, and pull requests without write access. That is, they can be assigned PRs, assign them to others, review them, etc but not merge. Once they approve, then a smaller subset of maintainers such as yourself and Ricardo would be able to merge.

These users need to participate for reviewing PRs and triaging issues only.
The merge rights should remain restricted to few repo owners.

This is the new model we have been advocating for, i.e. fewer users with the Maintainer/Write role, and more users with Triage role. Let me know if any concerns.

cc: @ricardosalveti for thoughts

[sbanerjee-quic]

@ricardosalveti , @quic-vkraleti , @mynameistechno
PR #32 is a workflow proposal to assign reviewers.

This worked on the forked repo sbanerje-quic/meta-qcom-hwe. But in this repo I see an error when the workflow runs.
error fetching organization teams: GraphQL: Resource not accessible by integration (organization.teams)

Any hints?

[sbanerjee-quic]

@ricardosalveti @mynameistechno
Here's proposal based on CODEOWNERS
#44

I am thinking the last line, i.e., rule with *, will help one of maintainers to be added as reviewers apart from the match that happens in previous lines. Thus, if there is a change in recipes-kernel/linux/ then reviewers will be assigned from both groups.
Need to test this though.

[mynameistechno]

@sbanerjee-quic the codeowners file will expand to include other recipes/tech areas as they are "forward-ported" to main?

I.e. do we still want to refrain from giving Write access to all the recipe/tech team members? If so we'll continue exploring my last idea here of leveraging codeowners with our own GH Action (vs native GH codeowners functionality)

[sbanerjee-quic]

@sbanerjee-quic the codeowners file will expand to include other recipes/tech areas as they are "forward-ported" to main?

Yes, the CODEOWNERS file will expand to include more reviewers who can be assigned technology specific reviews.

I.e. do we still want to refrain from giving Write access to all the recipe/tech team members? If so we'll continue exploring my last idea here of leveraging codeowners with our own GH Action (vs native GH codeowners functionality)

I created the native codeowners PR #44 , to test if the last catchall line in the file can act as a mandatory second reviewer. I am thinking if "Require a pull request review before merging" setting is enabled for the 'main branch then the following 2 rules will work together to ensure there are sufficient reviews in place before anyone with write access can merge a PR.
recipes-kernel/linux/* @NainaMehtaQUIC @quic-vkraleti @sbanerjee-quic
* @quic-vkraleti @ricardosalveti @sbanerjee-quic

Or otherwise waiting for 2 reviews per PR will be better than just merging with 1 review.

If no option works with native codeowners we can likely go to codeowners with our own GH Action

[quic-yocto-ci]

We should probably also set branch protection rules to avoid any accidental direct push. We can look at what's appropriate and how it interacts with CODEOWNERS.

[craigez]

Note to self, don't reply via e-mail to notifications sent to quic-yocto-ci!

[mynameistechno]

Yes I was just looking at this. Here are settings we should consider.

The 2 required approvals may get annoying for simple things like minor CI updates, but we could add a bypass list for the Maintainers if needed:

So here is one path to explore:

• Grant Triage team (i.e. tech team reviewers) Write access so CODEOWNERS works out of the box.
• We turn on branch protection for main requiring 2 approvers
• Add bypass list for Maintainers if needed
• Either via CODEOWNERS or another mechanism, ensure a Maintainer also get added as a reviewer along with tech team area reviewer

[sbanerjee-quic]

Yes I was just looking at this. Here are settings we should consider.

The 2 required approvals may get annoying for simple things like minor CI updates, but we could add a bypass list for the Maintainers if needed:

So here is one path to explore:

• Grant Triage team (i.e. tech team reviewers) Write access so CODEOWNERS works out of the box.
• We turn on branch protection for main requiring 2 approvers
• Add bypass list for Maintainers if needed
• Either via CODEOWNERS or another mechanism, ensure a Maintainer also get added as a reviewer along with tech team area reviewer

Yes, I agree, these are good point to start. The setting you shared makes sense for the code review.

One unrelated question, should we also consider enabling the "Require signed commits" setting?

[craigez]

Either via CODEOWNERS or another mechanism, ensure a Maintainer also get added as a reviewer along with tech team area reviewer

We could have a separate action that added Maintainers as reviewers in addition to CODEOWNERS when needed. I think that would be a simple action to write. Still we can't enforce them as a required reviewer due to the way GH reviewers work.

[craigez]

"signed commits" in this context is not the DCO, but rather signing the commit with a GPG key: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

[mynameistechno]

Either via CODEOWNERS or another mechanism, ensure a Maintainer also get added as a reviewer along with tech team area reviewer

We could have a separate action that added Maintainers as reviewers in addition to CODEOWNERS when needed. I think that would be a simple action to write. Still we can't enforce them as a required reviewer due to the way GH reviewers work.

In addition to a GH Action there is also a way to round robin auto-assign members of a team. (In the team settings). Unclear how it works in conjunction with codeowners. I will test this out elsewhere first.

And you’re right I can’t see a way to force a specific reviewer. So if there are two reviewers assigned via codeowners or someone with sufficient permissions assigns a second reviewer they could get around it.

In practice I assume folks won’t merge if they see a maintainer also assigned as a reviewer. You can always revert a PR that was prematurely merged, and damage will be minimal unless it goes unnoticed. We should definitely disable Force pushes to prevent real damage though.