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

Conversation

@joao-paulo-parity
Copy link
Contributor

This PR fixes the situation in https://github.com/paritytech/polkadot/runs/5978667098?check_suite_focus=true#step:2:40

Given the approvals:

 {
  { id: 937326948, user: 'rphmeier', isApproval: true },
  { id: 937330726, user: 'eskimor', isApproval: true },
  { id: 938487338, user: 'bkchr', isApproval: true }
}

The check is failing because bkchr counts towards locks-review but then doesn't count towards polkadot-review since it's excluded from the latter.

This PR fixes the scenario by exiting the subconditions once ruleApprovedBy reaches rule.min_approvals (which is the sum of each individual min_approvals throughout all subconditions for AndDistinctRule).

@joao-paulo-parity joao-paulo-parity requested a review from a team as a code owner April 12, 2022 11:18
@bkchr
Copy link
Member

bkchr commented Apr 12, 2022

Can we not directly fix it properly and let one person count towards all the team the person is part of?

@joao-paulo-parity
Copy link
Contributor Author

Can we not directly fix it properly and let one person count towards all the team the person is part of?

@bkchr That's already possible and it's part of a different kind of rule, unrelated to this PR

@bkchr
Copy link
Member

bkchr commented Apr 12, 2022

So it means, the linked job would have succeeded without my approval and with this pr being applied?

@joao-paulo-parity
Copy link
Contributor Author

joao-paulo-parity commented Apr 12, 2022

So it means, the linked job would have succeeded without my approval and with this pr being applied?

https://github.com/paritytech/polkadot/runs/5978667098?check_suite_focus=true#step:2:40 would succeed with your approval and this PR applied because it has approvals from 2 different developers which are on polkadot-review and locks-review. This is a bug fix for how it currently is supposed to work, not a change in the intended behavior.

For your suggestion please see the comment in https://github.com/paritytech/polkadot/pull/5253/files#r848353059.

@TriplEight
Copy link
Contributor

As I see it, @bkchr suggests a feature: when a reviewer is a member of both teams, count their review as two.

Even though it's valid from the logical standpoint, I suggest leaving it as a feature request because it contradicts with what we've decided before in #67 and may be counterintuitive from reading the rules.

@joao-paulo-parity joao-paulo-parity merged commit 7227ae4 into paritytech:master Apr 12, 2022
@joao-paulo-parity joao-paulo-parity deleted the intersection branch April 12, 2022 12:14
@bkchr
Copy link
Member

bkchr commented Apr 12, 2022

As I see it, @bkchr suggests a feature: when a reviewer is a member of both teams, count their review as two.

Even though it's valid from the logical standpoint, I suggest leaving it as a feature request because it contradicts with what we've decided before in #67 and may be counterintuitive from reading the rules.

I read the rules exactly like this in #67. I even made the comment here: #67 (comment)

@joao-paulo-parity
Copy link
Contributor Author

@bkchr I just want to be extra clear that we're not being dismissive of your suggestion. I tried to implement the requirements following exactly what's written in https://github.com/paritytech/pr-custom-review/blob/da1d81b9fd39705cc8b37f59235283801c818708/rules.md, as explained in #67 (comment). While it's possible that I've interpreted the text wrong, I'm not in a position to go against what's exactly written there.

That being said, your suggestion is already supported by pr-custom-review's features. If it's clearly demonstrated that your suggestion is aligned with the stakeholders', please let us know and we can change the behavior promptly.

@bkchr
Copy link
Member

bkchr commented Apr 12, 2022

Ty @joao-paulo-parity

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants