Merge CODEOWNERS Part 2

[chevdor]

Description

This PR:

• merge the CODEOWNERS from each former repos
• add catch-alls and default owners

[ggwpez]

This actually closes #1164.
There are more similar ones in the Polkadot-SDK (Repo) or Monorepo projects.

[ordian]

hmm, I think this was previously only needed for runtime files, which are going to be migrated to https://github.com/polkadot-fellows/runtimes

#1239 (comment)

[chevdor]

Are you suggesting using core-devs for /polkadot as catch-all as well ?

[ordian]

yeah, probably start with not too strict rules and enforce other rules via PRCR

[chevdor]

Yes the idea here is not to be too strict but have "full" coverage for the relevant files (ie all files but things like .gitignore, etc...)

[franciscoaguirre]

This is definitely too strict

[bkchr]

Please stop introducing random rules that weren't present before.

[bkchr]

Suggested change

/cumulus @paritytech/cumulus-locks-review @paritytech/core-devs

[bkchr]

Suggested change

/polkadot @paritytech/polkadot-review

[bkchr]

Suggested change

/substrate @paritytech/SubstrateTeamLeads

[franciscoaguirre]

We could add a new rule /polkadot/xcm/ and tag the XCM team, or Keith, Just and me

[franciscoaguirre]

This is definitely too strict

[chevdor]

@bkchr, the idea is to have some fallback/catch-all for:

• /substrate
• /polkadot
• /cumulus

Ideally, those catch-all never trigger because we have more accurate rules to cover the content of the folders mentioned above. They should be covered nonetheless in case CODEOWNERS rules end up not covering everything* (that can easily happen as the project grows and new content is added).

• with the current state of the PR, I did not try 100% coverage, but those top level folder should be covered.
  This is why I would rather not keep those empty.

We could add a new rule /polkadot/xcm/ and tag the XCM team, or Keith, Just and me

@franciscoaguirre Yes, totally and we should.
I think it is better to use teams but we may not have all those we needs defined.
In your case with xcm, there is https://github.com/orgs/paritytech/teams/xcm. So if you 3 are fine, we can add that rule. Just let me know and I will add it or feel free to commit into the PR.

[bkchr]

Why do we need these fallback catch all? For what exactly?
Who told you that we need this?

[chevdor]

Having full coverage with the CODEOWNERS allows matching any relevant file to an owner (or a set of ~).

CODEOWNERS is mainly used to define who should review but also helps notify people about relevant content. As a side effect, it helps figuring out who is the right contact for a given piece of code, documentation, script, etc...
For regular contributors, it is often trivial and they know who to contact. Looking at the history of changes can also help but reflects only the past. For new contributors and automation however, a proper coverage via CODEOWNERS is very helpful.

Without a catch-all in place, we will only match those patterns that are defined for as long as they are valid.

Currently we have quite such rules for substrate but not much for the rest and none of the code for polkadot

If my initial proposal for the top level owner can be improved, please propose better choices but keep in mind that those are only cath-all and should have no effect if we have good rules for the content.

On that last point, I see already quite some suggestions in this thread but I recon it may take a while to make it complete.

I can split this PR into 2, in order to deal with #1164 first and deal with further discussions in a follow-up PR:

• simple PR with what we currently have without the new patterns
• this PR that adds more rules

[franciscoaguirre]

@chevdor I added the XCM team to the file but it's showing an error.
It might be the XCM team needs write access to polkadot-sdk? Right now under repositories I only see paritytech/xcm

[chevdor]

@franciscoaguirre

It might be the XCM team needs write access to polkadot-sdk? Right now under repositories I only see paritytech/xcm

yes indeed, this is mentioned here:

The people you choose as code owners must have write permissions for the repository. When the code owner is a team, that team must be visible and it must have write permissions, even if all the individual members of the team already have write permissions directly, through organization membership, or through another team membership.

[chevdor]

I extracted the simple merge as #1472

[franciscoaguirre]

@chevdor Yes, I think I don't have permissions to add it. Do you? If not, I'll ask around

[chevdor]

@franciscoaguirre no I don't, sorry

[bkchr]

For regular contributors, it is often trivial and they know who to contact. Looking at the history of changes can also help but reflects only the past. For new contributors and automation however, a proper coverage via CODEOWNERS is very helpful.

Not sure how a catch all should be helpful to know who to contact. I'm against this pr. I don't see any reason for this. Having individual parts beinged covered by CODEOWNERS is fine, while this also already isn't working. People are getting pinged any mainly ignore it. I'm going to close this issue. If someone finds this ultra important, the discussion with a proper proposal and issue description can continue in an issue.