palantir / policy-bot

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

File-based policies fail when file is removed #854

Closed iainlane closed 1 month ago

iainlane commented 1 month ago

Perhaps this is a duplicate of #246 - no problem closing if so.

This is one of our rules:

  - name: Workflow .github/workflows/linters.yml succeeded or skipped
    requires:
      conditions:
        has_workflow_result:
          conclusions:
            - skipped
            - success
          workflows:
            - .github/workflows/linters.yml

If we ever delete or rename .github/workflows/linters.yml, the policy won't pass and the PR will not be able to be merged.

I'm wondering about a file_exists predicate, or perhaps supporting {ADDED, CHANGED, MODIFIED, REMOVED} on changed_files and no_changed_files. So it'd look something like:

  - name: Workflow .github/workflows/linters.yml succeeded or skipped
    if:
      no_changed_files:
        paths:
          - path: .github/workflows/linter.yml
            types: [REMOVED]
    requires:
      conditions:
        has_workflow_result:
          conclusions:
            - skipped
            - success
          workflows:
            - .github/workflows/linters.yml

or

  - name: Workflow .github/workflows/linters.yml succeeded or skipped
    if:
      file_exists:
        - github/workflows/linter.yml
    requires:
      conditions:
        has_workflow_result:
          conclusions:
            - skipped
            - success
          workflows:
            - .github/workflows/linters.yml
bluekeyes commented 1 month ago

I'm a bit unclear on the situation you are trying to handle here, which means I'm not sure if this is the same as #246 or not. Which of these are you concerned about?

  1. PR X removes the linters.yml workflow, so GitHub does not run this workflow for the PR. Even if PR X removes (or fixes) this condition in the policy, it cannot merge because the policy comes from the target branch.
  2. PR X removes the linters.yml workflow without modifying the policy. Afterwards, PR Y and PR Z should still be able to merge even though the workflow doesn't run anymore.
  3. Something else

If it's the first case (making sure you can merge PRs that change workflows and also update the policy at the same time), then I think this is basically the same request as in #246: a condition on addition or removal of files as part of the current PR.

Another options that is sometimes useful is to have some override rule that can approve in all situations but is normally unused. Whether this is viable depends on your team and policy structure and how often these changes happen, but if they're rare enough, getting approval from say 2-3 senior engineers/managers/admins to unstick cases like this might work.

iainlane commented 1 month ago

It's the first case, yep.

We have such an override rule so this is not a blocker. It's just a little bit awkward in our case since the repository in question is a monorepo with workflows from many teams which run at various points, and having a step where they have to find people to help them override the check is a bit clunky. As far as possible, I want the policy bot check to feel smooth to people, not something they sometimes have to do obscure things to work with.

(This isn't massively urgent as removing workflows is not very common.)

I'll duplicate this to #246 though and we can decide what is best over there, then I'll implement whatever that is.

iainlane commented 1 month ago

Duplicate of #246