Update of PR shouldn't always remove approval check

[DepickereSven]

We have the following use case:

We have non-technical people checking a certain number of files that contain files that will generate documentation. So when they find that documentation looks fine they will approve it.

But whenever something gets pushed the bot will ask for approval again. Would it be possible to have something as follows:

• The non-technical people approve the changes.
• Technical people make a comment, to file outside of the scope of non-technical.
• Comment gets solved
• Technical people are asked again for review while non-technical people aren't asked to review again.

If there is a change in their scope they should be asked to review the changes of course.

Does something already exist? Or is possible to give some pointers so we can make PR for this feature?

[bluekeyes]

Most of this workflow is possible except for the final request: there is no way to selectively re-request reviews based on the files changed.

Because the invalidate_on_push option applies at the rule level, you can create a policy like this, where technical people have to review all changes on every push, while non-technical people only need to review once:

policy:
  approval:
    - docs changes are approved
    - code changes are approved

approval_rules:
  - name: docs changes are approved
    if:
      changed_files:
        paths:
          - "docs/.*"
    options:
      invalidate_on_push: false
    requires:
      count: 1
      teams: ["my-org/docs-reviewers"]
  - name: code changes are approved
    options:
      invalidate_on_push: true
    requires:
      count: 1
      teams: ["my-org/code-reviewers"]

This has a few disadvantages but these might be acceptable:

• Once the first rule is approved, that approval is never invalidated, even if documentation changes again
• The second rule is invalidated by any push, even if the update only touched documentation. Your technical reviewers must review all changes.

---

The main limitation for implementing this feature is that we only build modified file lists using the full pull request, rather than the changes in individual commits. If you want to try implementing this feature, I think you will need to:

1. Modify the GitHub interactions to load modified files from commits. While doing this, carefully consider how this changes the number of GitHub API requests required to evaluate a PR. You'll also need to make sure commits combine properly. For example, if a commit adds a file and a later commit removes it, the file should not influence the final evaluation.
2. Pass information about modified files back to the evaluation logic along with the existing commits
3. Modify the predicate evaluation so that you can evaluate file-based predicates on individual commits instead of only on full pull requests
4. Modify the invalidation logic to account for modified files. You will need to check if the combined modifications of all commits pushed after an approval modified any files selected by predicates of the current rule.

I think this also opens the question of if there are any other predicates that should have the same behavior. Overall, while possible, I believe this would require refactoring some of the more complicated parts of Policy Bot.

[DepickereSven]

@bluekeyes thank you for the clear explanation.

While evaluating this Policy-bot rules, we got some feedback:

Once the first rule is approved, that approval is never invalidated, even if documentation changes again This wouldn't be acceptable from the docs-reviewers, because they can push on-reviewed documentation once they got an approval.

The second rule is invalidated by any push, even if the update only touched documentation. Your technical reviewers must review all changes. This approach is what we're currently using, it's causing a lot of overhead for our docs-reviewers.

So would be open to develop this feature, if that's a feature that would be accepted for Policy-bot.

[bluekeyes]

I think it makes sense to limit the impact of invalidate_on_push to rules with no file predicates and rules with matching file predicates. From that perspective, I would accept a PR implementing this feature.

My main concerns are what I outlined earlier. I think this will touch some of the most complicated parts of Policy Bot, could increase the number of API calls made for each evaluation, and requires care because it modifies core logic. I don't want to discourage you from trying this, but I expect it will take a while to get an initial version working and then require iteration on the PR to get something we can merge and support.