CODEOWNERS: Refine ownership #125

Merged

fyrchik merged 1 commit from fyrchik/frostfs-contract:fix-codeowners into master

2024-11-07 09:16:50 +00:00

fyrchik commented

2024-11-07 06:24:03 +00:00

Owner

Signed-off-by: Evgenii Stratonikov e.stratonikov@yadro.com

Signed-off-by: Evgenii Stratonikov <e.stratonikov@yadro.com>

fyrchik added 1 commit 2024-11-07 06:24:03 +00:00

CODEOWNERS: Refine ownership

DCO action / DCO (pull_request) Failing after 41s

Details

Code generation / Generate wrappers (pull_request) Successful in 1m29s

Details

Tests / Tests (pull_request) Successful in 1m37s

Details

cc73970b1c

Signed-off-by: Evgenii Stratonikov <e.stratonikov@yadro.com>

fyrchik requested review from storage-core-committers 2024-11-07 06:24:14 +00:00

fyrchik requested review from storage-core-developers 2024-11-07 06:24:22 +00:00

fyrchik requested review from storage-services-committers 2024-11-07 06:24:23 +00:00

fyrchik requested review from storage-services-developers 2024-11-07 06:24:24 +00:00

fyrchik force-pushed fix-codeowners from cc73970b1c to 8b586081eb

2024-11-07 06:24:39 +00:00

Compare

acid-ant approved these changes 2024-11-07 06:28:03 +00:00

fyrchik requested review from potyarkin 2024-11-07 07:29:31 +00:00

potyarkin commented

2024-11-07 08:59:09 +00:00

Member

Replying to @fyrchik's questions from IRL conversation:

"Last matching line wins". That was the idea, and that's how it works on GitHub. Current Forgejo implementation is different and works as you wanted: all matching lines are active, there is no need to repeat reviewers for subdirectories. That also means that .* reviewers will always get pinged even on PRs they may not care about. And that the order of CODEOWNERS lines is irrelevant, at least until Forgejo decides to fix their code to match their deleted documentation.
- UPD: At least for now, Gitea works the same. And they have not deleted their documentation
I wanted to comment about moving .* line to the top of the file, but researching Forgejo code for the point above has made this one irrelevant.
Forgejo does not allow defining ad-hoc groups inside CODEOWNERS file. So it's either copy-paste or tweaking @org/teams defined by org administrator in web UI. CODEOWNERS parser in Forgeo is very basic, it parses each line without any awareness of previous state. Good news: see point #1, you don't have to repeat yourself :-D
CODEOWNERS just defines which people will automatically be asked to review the PR. Whether these unfinished reviews are blocking or not is decided by branch protection configuration:

☑️ Block merge on official review requests
Merging will not be possible when it has official review requests, even if there are enough approvals

Replying to @fyrchik's questions from IRL conversation: - "Last matching line wins". That was the idea, and that's [how it works on GitHub](https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners#codeowners-syntax). Current Forgejo [implementation](https://codeberg.org/forgejo/forgejo/src/commit/ed2d5f6b73216bdb24dc685ed22799f472fbce31/services/issue/pull.go#L100-L111) is different and works as you wanted: all matching lines are active, there is no need to repeat reviewers for subdirectories. That also means that `.*` reviewers will always get pinged even on PRs they may not care about. And that the order of CODEOWNERS lines is irrelevant, at least until Forgejo decides to fix their code to match their [deleted documentation](https://codeberg.org/forgejo/forgejo/src/commit/3bdd48016f659c440d6e8bb57386fab7ad7b357b/docs/content/doc/usage/code-owners.en-us.md). - UPD: At least for now, Gitea [works the same](https://github.com/go-gitea/gitea/blame/331e878e81d57235a53199383087c7649797a887/services/issue/pull.go#L98-L111). And they have not deleted their [documentation](https://docs.gitea.com/usage/code-owners) - I wanted to comment about moving `.*` line to the top of the file, but researching Forgejo code for the point above has made this one irrelevant. - Forgejo does not allow defining ad-hoc groups inside CODEOWNERS file. So it's either copy-paste or tweaking @org/teams defined by org administrator in web UI. CODEOWNERS parser in Forgeo is [very basic](https://codeberg.org/forgejo/forgejo/src/commit/ed2d5f6b73216bdb24dc685ed22799f472fbce31/models/issues/pull.go#L929), it parses each line without any awareness of previous state. Good news: see point #1, you don't have to repeat yourself :-D - CODEOWNERS just defines which people will automatically be asked to review the PR. Whether these unfinished reviews are blocking or not is decided by branch protection configuration: > ☑️ Block merge on official review requests > Merging will not be possible when it has official review requests, even if there are enough approvals

potyarkin approved these changes 2024-11-07 09:00:14 +00:00

fyrchik added this to the v0.21.0 milestone 2024-11-07 09:15:50 +00:00

fyrchik merged commit 8b586081eb into master

2024-11-07 09:16:50 +00:00

fyrchik deleted branch fix-codeowners

2024-11-07 09:16:51 +00:00

fyrchik referenced this pull request from a commit

2024-11-07 09:16:52 +00:00

[#125] CODEOWNERS: Refine ownership