Discuss rules

[joao-paulo-parity]

Rendered

---

Given the reasoning in paritytech/substrate#10951 (comment), I plan to use this pull request exclusively for discussing repository rules. It will not be merged.

Once a consensus has been reached, we'll implement what is defined in this PR without having to discuss again for each repository.

The plan for this PR is as follows:

Step 1: I'll collect the requirements with @TriplEight
Step 2: We'll invite one or more stakeholders to clarify specific details related to their expertise
Step 3: We'll request reviews from all stakeholders

#32, #35, #36, #38 can be closed after the discussion is finished

[bkchr]

For the team leads group, please create an empty team on github and then I will work with @dvdplm to fill this team :)

[TriplEight]

@bkchr

For the team leads group, please create an empty team on github and then I will work with @dvdplm to fill this team :)

Here you go: https://github.com/orgs/paritytech/teams/parachainleads

[TriplEight]

to summarize here what we've discussed with Basti:

There are now 3 new teams:
@paritytech/locks-review @paritytech/cumulus-locks-review and @paritytech/polkadot-review (yet to be populated).

• the rules about 🔒 s should ping 2 entire teams: @paritytech/polkadot-review AND one of @paritytech/cumulus-locks-review OR @paritytech/locks-review depending on the repo
• the rules about changed runtime files should ping 2 entire teams: @paritytech/polkadot-review AND one of @paritytech/cumulus-locks-review OR @paritytech/locks-review depending on the repo
• the rest regular PRs are reviewed by @paritytech/core-devs without pinging them. I assume this can be done by not assigning the team, but passively-aggressively waiting and failing a job until someone suitable reviews a pr. Details may be shown in the job's output.

[bkchr]

Otherwise the rules are looking good and are also approved by @gavofyork

[gavofyork]

Looks reasonable to me.

[TriplEight]

Here are the new teams' names we've agreed upon w Basti

[joao-paulo-parity]

FYI I plan to come back to this ASAP which is probably tomorrow or early next week. I've identified #68, #69, #70 and #71 as tasks to be worked on for us to support the requirements discussed here.

[joao-paulo-parity]

The preliminary PRs for the configuration have been submitted.

The application of those configurations through the GitHub workflow should be coordinated with a simultaneous update of processbot (paritytech/parity-processbot#373) since the current requirements are conflicting with processbot's own review requirements.

[joao-paulo-parity]

It seems like there's some disagreement about this requirement and others similar to it. I'll try to decompose the sentence according to my literal interpretation:

🔒️ed lines are reviewed by 2 team leads

Emphasis on "2".

one of them must be in ...

Let's call this agent DeveloperA.

and the other in ...

"the other" literally refers to another agent different from DeveloperA. Let's call this DeveloperB.

All of the above phrasing to me strongly suggests 2 different agents for this requirement.

Now it might be that someone is in both @paritytech/locks-review and @paritytech/polkadot-review. https://matrix.to/#/!gJeGMHCcDoIwsHIJri:matrix.parity.io/$5H7WC-VGc4Am5HuvivqpuqA-jR_ic2SmWiz0x4HK1cg raised the question:

why are you only counting towards one group if you are part of multiple?

If we'd make it so 1 person can fulfill the criteria by being in both groups, we'd effectively lower the requirement to a single approval (1 person) instead of 2 like I understood by the literal interpretation. If that's intentional, I suggest clarifying the phrasing as e.g.

🔒️ed requires 1 approval from @paritytech/locks-review and 1 approval from @paritytech/polkadot-review

That removes any implication of "2 team leads" since it only mentions 1 approval from each group, which might be given by the same person if they are in both groups. What's your opinion?

cc @bkchr @TriplEight

[TriplEight]

I agree that 1 person that is a member of two teams shall count as a review from both teams. But also it should count as one review of two requested.

[bkchr]

Yes that sounds good.

Again to write it in my own words:

🔒️ed lines are reviewed by 2 team leads, one of them must be in @paritytech/locks-review and the other in @paritytech/polkadot-review. Team leads that are being part of both groups are counted as one team lead that directly fulfills both groups. A second team lead from either team is then required to make the check succeed.

@TriplEight @joao-paulo-parity can you than please implement it that way? Ty!

[joao-paulo-parity]

A second team lead from either team is then required to make the check succeed.

"Either" team? That's different from how I was understanding your proposal.

I think it'll help to walk through an example scenario. Suppose the following structure

├── locks-review
│  ├── user1
│  └── user2
├── polkadot-review
│  ├── user1
│  └── user3

Alternative 1 (how it works currently): after user1 approves we'll still need an approval from user2 or user3, even though user1 is in both teams.

Suggested wording:

🔒 lines requires 1 approval from @paritytech/locks-review and 1 approval from @paritytech/polkadot-review. Each approval should come from a different user.

Note: I think adding "Each approval should come from a different user" is clearer than saying "2 team leads".

---

Alternative 2 (what I initially understood from your suggestion): user1's approval would fulfill the requirement because they are on both teams, therefore not requiring an approval from user2 or user3.

Suggested wording:

🔒 lines requires 1 approval from @paritytech/locks-review and 1 approval from @paritytech/polkadot-review

Note: avoids mentioning "2 team leads" in order to not provoke any ambiguity, since a single team lead is enough to fulfill both requirements. Also avoids mentioning "Each approval should come from a different user" since it's intentionally not that way.

---

Alternative 3 (how I understood from "either"): after user1 approves we'll need an approval from either user2 or user3.

Suggested wording:

🔒 lines requires 2 total approvals from users on either @paritytech/locks-review or @paritytech/polkadot-review. Each approval should come from a different user.

---

Which alternative are you suggesting? Also feel free to describe another alternative if none is matching

[bkchr]

Isn't alternative 1 and 3 the same?

And is alternative 1 really how it currently works? I doubt this.

[bkchr]

Okay, I think I may mixed something up. I'm now confused what the current status is. I just know that yesterday it wasn't alternative 1.

[bkchr]

I have now extended the polkadot-review team and now it should be okay.

[joao-paulo-parity]

Isn't alternative 1 and 3 the same?

In this specific example, when user1 approves, they result in the same outcome but their definitions are still different.

Consider if user2 were to approve. "Alternative 1" in that case would require that other approval comes from specifically polkadot-review (user2 isn't a part of it). However, if you say "total approvals from either team" then the second approval could come from the same team that user2 is in. In this reduced example with a few team members they might effectively result in the same conditions, but I think it would be different if you start adding more team members like the groups we have currently.

I just know that yesterday it wasn't alternative 1

If it's about https://github.com/paritytech/polkadot/runs/5978667098?check_suite_focus=true#step:2:43 it was misbehaving due to a bug, but it should be fine now.

[bkchr]

Then let's take alternative 1

[jsdw]

FWIW I thought that the initial "🔒️ed lines are reviewed by 2 team leads, one of them must be in @paritytech/locks-review and the other in @paritytech/polkadot-review." was precise enough to imply alternative 1 as-is, but if that is unclear, I'd go for something like:

🔒️ed lines must be reviewed by 2 team leads. One of them must be a member of @paritytech/locks-review and the other must be a member of @paritytech/polkadot-review.

If that is still unclear, then I'd add somehting like @bkchr said:

🔒️ed lines must be reviewed by 2 team leads. One of them must be a member of @paritytech/locks-review and the other must be a member of @paritytech/polkadot-review. If one is a member of both teams, then the other may be a member of either team.