search

finos / FDC3

An open standard for the financial desktop.
https://fdc3.finos.org
Other
203 stars 132 forks source link

Add CODEOWNERS #1253

Closed bingenito closed 2 months ago

bingenito commented 4 months ago

Enhancement Request

Add CODEOWNERS and enforce through branch protection. This will help facilitate the appropriate reviewers are automatically added based on contents of the PR and that their approvals are complete before PR can be merged.

Additional Information

https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners

### Tasks
- [x] Add CODEOWNERS
- [x] Require CODEOWNERS in branch protection
bingenito commented 4 months ago

Recommended content of initial CODEOWNERS would be to require fdc3-maintainers as blanket default and for /docs to include approvals from both a member of fdc3-editors and a member of fdc3-maintainers

* @finos/fdc3-maintainers 
/docs/ @finos/fdc3-editors  @finos/fdc3-maintainers
bingenito commented 4 months ago

I also suggest we add CODEOWNERS into /.github to prevent adding yet another file to the root of the project.

kriswest commented 4 months ago

Recommended content of initial CODEOWNERS would be to require fdc3-maintainers as blanket default and for /docs to include approvals from both a member of fdc3-editors and a member of fdc3-maintainers

* @finos/fdc3-maintainers 
/docs/ @finos/fdc3-editors  @finos/fdc3-maintainers

Where there are two owners, does it require both to review? No:

Note: When reviews from code owners are required, an approval from any of the owners is sufficient to meet this requirement.

Nevertheless, this would be a small improvement over the current setup (which just requires a review from someone with write access) and would automatically add reviewers to PRs. It will mean that FINOS admins can't approve changes alone - but generally they've always asked for a maintainers review anyway. However @maoo and @robmoffat should probably comment.

You may need protection for the CODEOWNERS file itself, perhaps:

/.github/CODEOWNERS @finos-admin

Which would mean FINOS has to approve any future changes to code ownership.

Finally, I think we need to have an additional conversation about release branches. We no longer release from main, but release/* branches are not protected so anyone with write access could do a release... I'm not yet sure how to govern that and wheter it would branch protection (for as yet unknown branches or a ruleset (as I think thats what those are for).

bingenito commented 4 months ago

Agreed in maintainer meeting that we would exclude the special case for /docs/ for now.

kriswest commented 2 months ago

THis is now complete, right @bingenito?