-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
Mentions don't work on GitHub according to github/markup#209 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
to summarize here what we've discussed with Basti: There are now 3 new teams:
|
Otherwise the rules are looking good and are also approved by @gavofyork |
Looks reasonable to me. |
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. |
@@ -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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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