review dismissal question

[devinburnette]

Hello,

I think I'm currently hitting an edge case introduced in: #577

As described in the bottom of that PR description:

If a single approval satisfies rules A and B where A is invalidated by pushes and B is not, pushing a new commit will not dismiss the review (because it still satisfies B). This is the intended behavior, but it could confuse people. I think in practice conflicting rules like this are behind predicates.

Most of our rules are "reviews with comments" type of policies where the github review could potentially contain a comment satisfying multiple policy rules. So it is a bit surprising to us to see that a new commit being pushed post approval makes the policy go back to pending after the re-evaluation, but doesn't actually dismiss the github review. This is where in the original implementation, I was thinking the "reason" field could clearly state which candidate is the cause of the review being dismissed.

Example policy:

policy:
  approval:
      - and:
          - domain review
          - platform review

approval_rules:
- name: domain review
  requires:
    count: 1
    teams:
    - "some-org/engineering"
  options:
    ignore_edited_comments: true
    invalidate_on_push: true
    allow_non_author_contributor: true
    methods:
      comments: []
      github_review: true
      github_review_comment_patterns:
        - '(?i)\bdomain\s*lgtm\b'
- name: platform review
  requires:
    count: 1
    teams:
    - "some-org/platform-reviewers"
  options:
    ignore_edited_comments: true
    invalidate_on_push: false
    allow_non_author_contributor: true
    methods:
      comments: []
      github_review: true
      github_review_comment_patterns:
        - '(?i)\bplatform\s*lgtm\b'

Is there a way to have it still dismiss the github review commenting the reason with this set up? You mention using predicates, is there a way to refactor this policy to accomplish what we want with predicates?

[bluekeyes]

To make sure I understand the problem, is this accurate?

1. User A leaves a review that says domain lgtm
2. User B leaves a review that says platform lgtm
3. The PR author pushes a new commit
4. Policy Bot moves the status to pending, but does not dismiss the review from User A

[devinburnette]

User A leaves a review that says domain lgtm
User B leaves a review that says platform lgtm
The PR author pushes a new commit
Policy Bot moves the status to pending, but does not dismiss the review from User A

Almost, that scenario is fine I think. The scenario where the edge case lives is:

1. User A leaves a single review that says both domain lgtm && platform lgtm
2. PR author pushes a new commit
3. Policy Bot moves the status to pending, but does not dismiss the review from User A

[bluekeyes]

Ah, thanks. I don't think there's a way to solve this with predicates, since those are about the state of the pull request, not the state of the approvals.

I'm not sure there's a way to make this work for all cases: it seems like a fundamental conflict where there are arguments for either resolution:

• The current implementation picks the invariant that Policy Bot dismissing a review should never change the approval state of a rule. In your example, because the approval still satisfies the platform review rule, we can't dismiss it without changing the state of that rule, even though it wouldn't change the overall state of the policy
• It sounds like your desired behavior is that Policy Bot should dismiss a review when it becomes invalid for any rule that it previously approved. In other words, if the review no longer satisfies all of the rules it did at the time the user left it, Policy Bot should dismiss the review because it no longer matches the user's original intent.

Right now, I don't see a way to reconcile these positions, but maybe its something we could allow as an option either in the server or in the policy.