Rework review & locks rules

[mordamax]

• Parity code should in general require 3 Parity people to know it prior to merge. This includes the author, therefore we generally want 2 reviews.
• Locked code (i.e. code marked with a 🔒) or changes to production runtimes (i.e. Kusama & Polkadot relay and their system parachains) should require 2 senior devs (Fellowship rank >= 4) to know the changes prior to merge. This includes the author, so if the author happens to be a senior dev already then this condition only necessitates one further senior dev review.
• Any parts of the codebase may also have their own lists of reviewers. These can continue, but individual reviews are allowed to contribute to multiple sub-conditions. It should still be based around "knowers-of-code", which means the author should count towards the requirements.

May 25

Gav: can we ensure that the author is also counted against the number of special reviewers? I.e. where we need 2 reviewers from a particular set, if the author of the PR is in this set, then of the two reviewers, we should only require one of them to be from that set. The point of having these privileged sets is not to hold up the review cycle or increase the reviews requried: it's to ensure particular eyes have seen code changes. It doesn't matter if those eyes saw it in the authoring process or in the review process. I asked for this a while back but just checking that it has been implemented: the locks system should take into account the initiator of the PR, not just the reviewers. Also, a review by someone for one subcondition should also contribute to another subcondition.

Upd Jun 18:

i'm having a bit of trouble interpreting the CI log [here](https://github.com/paritytech/polkadot/actions/runs/5303962201/jobs/9599941536#step:3:37) i authored the PR, kian reviewed it. it says 2 reviews are needed for the change to runtime files and that there's only 1: Rule "Runtime files" needs in total 2 DISTINCT approvals, but 1 were given it mentions kian's username: combinationApprovedByMostPeopleOverall: Map(1) { 'Runtime files[1]' => Set(1) { 'kianenigma' } } yet the other required reviewers it mentions are all the people in locks-review group excluding me (which kian is not a member of): usersToAskForReview Map(4) { 'andresilva' => { teams: Set(1) { 'locks-review' } }, 'eskimor' => { teams: Set(1) { 'locks-review' } }, 'bkchr' => { teams: Set(1) { 'locks-review' } }, 'rphmeier' => { teams: Set(1) { 'locks-review' } } } so i don't get it - is the script working properly and figured out that i as the author am a member of locks-review (but just hasn't mentioned it in the dump)? Gav ok - definitely seems like it's not doing what i want yet. seems like it's not allowing reviewers enabling locks-review to pass to count in polkadot-review: Rule "Runtime files" needs in total 2 DISTINCT approvals, but 1 were given. Users whose approvals counted towards one criterion are excluded from other criteria. For example: even if a user belongs multiple teams, their approval will only count towards one of them; or even if a user is referenced in multiple subconditions, their approval will only count towards one subcondition. Subcondition "Runtime files[1]" does not have approval from the following users: andresilva (team: polkadot-review), eskimor (team: polkadot-review), athei (team: polkadot-review), stefan-sopic (team: polkadot-review), ordian (team: polkadot-review), bkchr (team: polkadot-review), rphmeier (team: polkadot-review), sandreim (team: polkadot-review). This is a utterly bare minimum change to make the system developed actually usable. I'd also like to see integration into a chat bot so that for PRs which have otherwise enough reviews but are blocked on locks-reviews, the relevant individuals are pinged in chat with a link and request to approve the PR And with the planned move of the runtimes to the fellowship, we should also alter the locks reviews so that they take into account the fellowship rank via a smoldot query.

[Bullrich]

I would like to rewrite this project and prepare it for the Fellowship migration.

The current state of the code is too messy, there is no abstraction at all which makes this so difficult to implement.

And the configuration files are not readable at all:

exclude: ^\.gitlab-ci\.yml|^scripts/ci/.*|^\.github/.*|^\.config/nextest.toml|^frame/(?!.*(nfts/.*|uniques/.*|babe/.*|grandpa/.*|beefy|merkle-mountain-range/.*|contracts/.*|election|nomination-pools/.*|staking/.*|aura/.*))
include: ^frame/(?!.*(nfts/.*|uniques/.*|babe/.*|grandpa/.*|beefy|merkle-mountain-range/.*|contracts/.*|election|nomination-pools/.*|staking/.*|aura/.*))
include: ^\.gitlab-ci\.yml|^scripts/ci/.*|^\.github/.*|^\.config/nextest.toml
exclude: ^runtime/(kusama|polkadot)/src/[^/]+\.rs$|^\.gitlab-ci\.yml|^(?!.*\.dic$|.*spellcheck\.toml$)scripts/ci/.*|^\.github/.*

[Bullrich]

So, I have been researching the code: Most rules here and AND DISTINCT rule here set the reviews here.

We need to add an extra review by the user itself and let the system check if it counts.

[mordamax]

Parity code should in general require 3 Parity people to know it prior to merge. This includes the author, therefore we generally want 2 reviews.

@gavofyork do I understand correctly, considering that we will start to count PR initiator in according team condition, that we should also increase min_approvals to 3 for core-devs condition?

a) Let's say one of core-devs created PR in:

1. non-locked place:

   • 3 min_approvals for core-devs: they still need 2 approvals from core-devs counting himself as "knower-of-code"
   • 2 min_approvals for core-devs: only 1 core-dev approval to merge

2. locked place:

   • 3 min approvals for core-devs: if for example Gav (locks-reviews) and ordian (polkadot-reviews) approved - then green to merge
   • 2 min approvals for core-devs: only Gav or (Gav and ordian) approved, - then green to merge (as they are also members of core-devs team, + counting initiator as "knower-of-code")

b) Non-core-dev creates PR in:

1. non locked place:
   • 3 min_approvals for core-devs: 3 approvals required (core-devs)
   • 2 min_approvals for core-devs: 2 approvals required (core-devs)
2. locked place:
   • 3 min_approvals for core-devs: 3 approvals required (polkadot-review, locks-review, core-devs)
   • 2 min_approvals for core-devs: 3 approvals required (polkadot-review, locks-review, core-devs)

[mordamax]

Reopened for now, until we test the fixes

[Bullrich]

Fixes had been tested.

Current staging server instance is running with the new update.

As me and @mordamax are away until Friday, we decided to postpone the production release as we won't be able to monitor it during our absence.

[bkchr]

@gavofyork do I understand correctly, considering that we will start to count PR initiator in according team condition, that we should also increase min_approvals to 3 for core-devs condition?

As I understood the request, people that are being part of "special groups" (aka not core-devs) count towards the "approval" count. So, we always require two core-devs and the author is not counting towards these minimum number of two. However, when the pr also requires lock-review and being part of this group, it counts towards these two.

[mordamax]

@bkchr As I understood Any parts of the codebase may also have their own lists of reviewers. These can continue, but individual reviews are allowed to contribute to multiple sub-conditions. It should still be based around "knowers-of-code", which means the author should count towards the requirements. any part, including for core-devs and Parity code should in general require 3 Parity people to know it prior to merge. This includes the author, therefore we generally want 2 reviews. to me would mean that regardless of lock-review the "knowers-of-code" is always the rule (which makes sense).

min_approvals to 3 for core-devs condition

If not "knower" would contribute - that would be these 3 Parity people to know it if "knower" -> then generally want 2 reviews would be, discounting the "knower" themself

just wanted to confirm

[bkchr]

But then an external contributor will require three reviews. Not sure that we want this. core-devs is some special group and we should just "ignore", IMO.

[mordamax]

@bkchr, yes, and it seems to be corresponding to this requirement

Parity code should in general require 3 Parity people to know it prior to merge

I'm curious, whether it's more often happen that core-devs open PRs daily or external contributors? To what i see in PR list - it's more often core-dev authors, so in most cases this change is unnoticeable for people in parity. Thinking what kind of a problem for external devs (or us) that is going to become if +1 core-dev (out of 70+) would have to approve this?

[bkchr]

Just because people are part of core-devs, doesn't mean that they have any more knowledge than an external contributor. But yeah, let's try it. If not we can revisit.

[mordamax]

Ok, i've merged bump to 3 core-devs in polkadot. so this should be closed now