Skip to content
This repository was archived by the owner on Jun 3, 2024. It is now read-only.

Discuss rules #67

Closed
joao-paulo-parity wants to merge 10 commits into from
Closed

Discuss rules #67

Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ab9456f
Create rules.md
joao-paulo-parity Mar 18, 2022
7d5ac1c
Update rules.md
joao-paulo-parity Mar 18, 2022
3b56b4d
Remove <!-- -->
joao-paulo-parity Mar 18, 2022
8482294
Update rules.md
joao-paulo-parity Mar 18, 2022
8e7e6c9
Apply suggestions from code review
TriplEight Mar 18, 2022
b7ba58e
Apply suggestions from code review
TriplEight Mar 18, 2022
056871b
Apply suggestions from code review
TriplEight Mar 18, 2022
e63b255
final requirements and group names
TriplEight Mar 21, 2022
d6d5572
review suggestions
TriplEight Mar 21, 2022
da1d81b
dvdplm's suggesions
TriplEight Mar 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions rules.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Substrate

- 🔒️ed lines are reviewed by 2 team leads, one of them must be in @paritytech/locks-review and the other in @paritytech/polkadot-review.
Copy link
Copy Markdown
Contributor Author

@joao-paulo-parity joao-paulo-parity Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@TriplEight TriplEight Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@bkchr bkchr Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Contributor Author

@joao-paulo-parity joao-paulo-parity Apr 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@bkchr bkchr Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't alternative 1 and 3 the same?

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

Copy link
Copy Markdown
Member

@bkchr bkchr Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@bkchr bkchr Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@joao-paulo-parity joao-paulo-parity Apr 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@bkchr bkchr Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then let's take alternative 1

Copy link
Copy Markdown

@jsdw jsdw Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- Normal PRs are reviewed by 2 of @paritytech/core-devs. This group should not be explicitly mentioned, but the check should be fail explaining this rule.

# Polkadot

- 🔒️ed lines are reviewed by 2 team leads, one of them must be in @paritytech/locks-review and the other in @paritytech/polkadot-review.
- The following files are reviewed by 2 team leads, one of them must be in @paritytech/locks-review and the other in @paritytech/polkadot-review.
- `/runtime/{kusama|polkadot}/src/*.rs`, except weight files.
- Normal PRs are reviewed by 2 of @paritytech/core-devs. This group should not be explicitly mentioned, but the check should be fail explaining this rule.

# Cumulus

- 🔒️ed lines are reviewed by 2 team leads, one of them must be in @paritytech/cumulus-locks-review and the other in @paritytech/polkadot-review.
- The following files are reviewed by 2 team leads, one of them must be in @paritytech/cumulus-locks-review and the other in @paritytech/polkadot-review.
- `/polkadot-parachains/{statemine|statemint}/src/*.rs`, except weight files.
- Normal PRs are reviewed by 2 of @paritytech/core-devs. This group should not be explicitly mentioned, but the check should be fail explaining this rule.