palantir / policy-bot

A GitHub App that enforces approval policies on pull requests
Apache License 2.0
780 stars 108 forks source link

Rebase invalidates approval #795

Open paradisfm opened 4 months ago

paradisfm commented 4 months ago

When master updates between the time a PR is created and when it is merged, a rebase/"update master" commit is obviously required. However, if an approval is present at the time of the rebase, the rebase commit will invalidate the approve since it is a "new commit" despite the code in the PR not changing. Please investigate as this is quite cumbersome.

bluekeyes commented 4 months ago

I've though about this before, and it's a challenging problem to solve in all cases. While as a PR author, it's clear that your rebase did not change anything, from Policy Bot's perspective, the commits on the PR are completely different after a rebase because Git generates new commit objects as part of the operation. These new commits may also contain different content (trees) in the case of automatically resolved merge conflicts. Auto-resolved conflicts are probably safe to accept, but it's hard to distinguish between those and manually resolved conflicts, which should be reviewed.

I have yet to find a way to verify all of this using the tools provided by the GitHub API, but if you have any ideas, I'm interested to hear them!

For now, here are some strategies that might minimize this problem:

  1. Consider if you actually need to require that branches are up to date before merging. Internally, we've decided that the (small) risk of semantic merge conflicts is an acceptable tradeoff to avoid the extra CI time and work required to keep PRs up to date, even in large monorepos with hundreds of contributors. But every organization will have a different opinion here, so I understand if it is not an option for you.
  2. Instead of rebasing, use GitHub's "Update branch" button to create a merge commit and then enable the ignore_update_merges option for the relevant rules
  3. Consider removing the invalidate_on_push requirement from rules, if allowed by your organization's security/compliance posture