Check doesn't get removed when base branch changes

[palantir]

Check doesn't get removed when base branch changes

[bluekeyes]

Thinking about it more, this could be fixed if we finish up and merge #46: the Checks API has additional states, so we might be able to set the checks for other branches to the neutral status without introducing the security issue. I need to do more research into how the Check API states interact with required status checks for branch protection and how they render in the UI.

[bluekeyes]

This is a limitation of the GitHub API: status checks are attached to commits and I can't find a way to delete them once created. Since the branch still has the same commit, it shows the old status as well as any new statuses.

The branch names are necessary to avoid a different problem. Without them, you could create a PR into a branch you control with a policy that requires no approval, then change the base branch to be develop, and then race policy-bot to merge the PR before it can update the status to reflect the policy on the new target branch.

Is the main problem that this makes it look like some statuses are still pending even if they aren't required? Setting checks to success when switching branches would fix that, but I think it reintroduces the security issue above where you can use branch switching to skip the real policy.

[serhalp]

FWIW this bug makes policy-bot nearly unusable with certain git workflows (if you frequently open PRs against the feature branch your feature branch is branched off, to get early feedback, then change the base branch once your base branch is merged).

[bluekeyes]

To confirm, problem the is specifically that once you change the base branch, the summary status of the PR still appears as pending or failed even though all required checks are successful, which wrongly implies the PR is not ready to merge?

As I mentioned above, we're kind of stuck here because you can't delete status checks once created and overwriting a statuses to success when the policy hasn't actually passed introduces a way to merge PRs without approval.

Maybe we could solve this with a top-level option specifying the branches to check? That way, you could only protect your trunk/release branches and policy-bot would only post statuses for commits that are part of PRs targeting those whitelisted branches?

A similar workaround is currently possible, but involves a more complicated policy and requires that you follow a branch naming pattern:

policy:
  approval:
    - or:
      - change targets a feature branch
      - change is approved

approval_rules: 
  - name: change targets a feature branch
    if:
      targets_branch:
        pattern: "^feature/.*$"
    requires:
      count: 0
  - name: change is approved
    requires:
      count: 1
      teams: ["org/approvers"]

You'd still have the extra status appear, but they'd all be successful and wouldn't impact the overall state of the PR.

[serhalp]

I think you're on to something - to rephrase, what we really want is to only enable policy-bot checks for protected branches (in our case, just master). So if a PR is opened against another branch, ideally I'd want no check reported from policy-bot, but an always-true check is close enough.

So yes, I'll give something like this a try (slight reworking of your example):

policy:
  approval:
    - or:
      - <real approval policy>
      - change does not target a protected branch

approval_rules: 
  - name: <real approval policy>
  # real approval rules here
  - name: change does not target a protected branch
    if:
      targets_branch:
        pattern: "???"
    requires:
      count: 0

... although writing this out made me realize because re2 doesn't support negative lookahead/lookbehind and policy-bot doesn't support logical negation, there isn't really any proper way to write a predicate for "doesn't match the exact string 'master'".

And really what I'd want is a built-in predicate that returns true for protected branches and false otherwise. How difficult is this?

... or could policy-bot be configured to only report a check on a PR against a target branch for which it is configured as a required status check? That would work too.

[pkmetski]

@serhalp this policy does what you want:

policy:
  approval:
  - or:
    - 0 approvers for non-master
    - 2 non-contributors approvers for master

approval_rules:
- name: 0 approvers for non-master
  if:
    targets_branch:
      pattern: "^(?:[^m]+|m(?:$|[^a]|a(?:$|[^s]|s(?:$|[^t]|t(?:$|[^e]|e(?:$|[^r]))))))*$"
  requires:
    count: 0
- name: 2 non-contributors approvers for master
  if:
    targets_branch:
      pattern: "^(master)$"
  requires:
    count: 2