feat(ci): utilize file_not_deleted predicate

[dblinkhorn]

When a workflow file that a policy rule depends on is deleted in a PR, the rule should be skipped rather than fail. This is achieved by adding the file_not_deleted predicate to each workflow-based policy rule.

Closes #178851.

[zerok]

Do you perhaps also have a test-case there that exactly covers the "workflow was deleted" scenario? Or did I just miss it? Am new to the policybot codebase 🙂

[dblinkhorn]

Not sure I fully understand. I think these tests just ensure the generated policy has the structure and values it is supposed to. In terms of the actual functionality of the predicates once part of a policy, there are tests for that in the policy-bot repo itself.

Would you mind clarifying? Thanks Horst.

[iainlane]

cool news: this should be able to be dropped now if we use palantir/policy-bot@6245c9d or newer 👍

[dblinkhorn]

Fixed. Thanks!

[iainlane]

It looks good to me! Can you confirm how you've tested this please? Definitely the kind of check which it's easy to subtly get wrong in a way that a review is hard to catch.

One way to do it would be to upgrade to a fixed version of policy-bot in our dev environment, and then try this new branch in a test repo writing a .policy.dev.yml (think this is right) file.

[dblinkhorn]

@iainlane the only testing I've done so far is generating a policy and making sure the file_not_deleted predicate was properly applied to workflow-based actions. I will do some actual testing on a repo as well to make sure everything is working as intended, and report back.

[dblinkhorn]

Okay I did some testing but...

First I tried to test this using the grafana/github-policy-bot-dev, but the generated file got an invalid policy error. I am guessing it's because the policy-bot GitHub App attached to this repo does not contain the new commit to palantir/policy-bot that defined the logic for the file_not_deleted predicate? In any case, I didn't feel confident messing around with any Grafana Org GitHub Apps, so I decided to test this on my own test repo using my own installation of a policy-bot GH App which based on my latest merged commit to palantir/policy-bot.

I created this repo and installed a policy-bot GitHub App, ran the policy-bot server locally using ngrok to create a webhook URL. As you can see in the repo I have two test workflows. I used grafana/generate-policy-bot-config to generate a policy config and it spit out:

policy:
  approval:
    - or:
        - and:
            - Workflow .github/workflows/test-a.yml succeeded or skipped
            - Workflow .github/workflows/test-b.yml succeeded or skipped
            - default to approval

...at the top-level. However, my understanding is that this setup says that each workflow must succeed in order for the PR to be approved. This negates the purpose of the file_not_deleted predicate in this setup. Also, when I tested this kind of configuration with an always-failing workflow, the policy ended up being approved because of "default to approval". When I tested this in a PR which deleted both the failing workflow and another one, both were skipped, but the PR was not approved. I ended up using this:

policy:
  approval:
    - and:
        - or:
            - Workflow .github/workflows/test-a.yml succeeded or skipped
        - or:
            - Workflow .github/workflows/test-b.yml succeeded or skipped

...to ensure that each workflow must succeed or be skipped (and a deleted workflow definition file will result in a skipped rule). However, in testing this I found that even when one rule succeeds and another is skipped, the final evaluation is stuck in a pending state indefinitely--not approved as I want. I feel like this is not the correct/optimal setup here. I still don't fully understand how different teams are using this tool, but I assume most start with a base policy config? Is it the intention of this tool/this change that individual teams set up this top-level logic sufficient to enable the functionality of the new file_not_deleted predicate?

Thanks for any clarification you can give!

[dblinkhorn]

With the policy-bot bug fixed (thanks @iainlane!), the PR I created which deletes the build-b definition file is now approved by policy-bot as expected. The rule for build-b was skipped:

Any further testing I should do?