Configuration for policy-bot's behaviour when no rules match

[justinwyer]

Currently policy-bot will mark it's status as error if no rules match:

	case common.StatusSkipped:
		statusState = "error"
		statusDescription = "All rules were skipped. At least one rule must match."

I would like to purpose configuration that will override this behavior. Ideally no status would be set for my use case, however I can see a world where pending could also work or be desired.

I am happy to implement this if the PR would be considered.

[asvoboda]

Hey @justinwyer ,

Can you elaborate on your use-case? Internally, we make policy-bot a required status check. Github will always mark a required status check as pending, which is perhaps what you want.

[justinwyer]

hey @asvoboda we are using policy-bot & bulldozer to drive automated PR closure for gitops related workflow where commits are raised by various automation rather than developers.

We could make policy-bot required / make an approval rule for a human approval, however in this case bulldozer will merge the PR as soon as policy-bot's status is marked as success which is not always what we want.

The PR list view becomes are to understand from a glace as a PR may have a red / yellow dot due to policy-bot or due to a failed / in-progress test.

Ideally, for our use-case, policy-bot would add no status when all rules were skipped.

[asvoboda]

Ah okay, so the way we handle this is by writing policies that look something like the following [1]. Effectively, bots require 0 approvals when they touch certain files (think dependabot updating build.gradle or version lock files like go.mod) but everything else requires a human approval. Then, policy-bot + bulldozer will play nicely with whatever external automation/bots/updaters you have set up.

[1]

policy:
  approval:
  - or:
    - a non-contributing team member has approved
    - bot only touched circle

  disapproval:
    requires:
      teams: ["palantir/devtools"]

approval_rules:

- name: bot only touched circle
  requires:
    count: 0
  if:
    has_author_in:
      users: [ "svc-bot-name" ]
    only_changed_files:
      paths:
        - "^\\.circleci/.*$"

- name: a non-contributing team member has approved
  options:
    invalidate_on_push: true
  requires:
    count: 1
    teams: ["palantir/devtools"]

[dominykas]

Effectively, bots require 0 approvals when they touch certain files [..] but everything else requires a human approval

This is almost exactly the use case we're looking into!

However, with the proposed config, what you end up with is that every PR [which requires human approval] is showing either an ❌ or a 🟡. Both of these, when looking at the list of PRs, could be misleading for humans. In practice, what I've seen is that humans avoid the PRs with an ❌ or a 🟡, because they've been trained that these mean that the PR is broken and not ready to be reviewed.

If there was an option to not add the ❌ or a 🟡 when no rules match, then it could be the solution, because it's not ✅ for automerge purposes, but ✅ for all other intents and purposes.

[asvoboda]

In practice it means PRs that don't match show up as pending (not failed), which I don't think is unreasonable. It means the check is pending approval.

If policy-bot doesn't add anything (and the check is required in Github) then Github still marks the check as pending anyway but without the extra status and context from policy-bot, which is even worse for humans.

[dominykas]

Yeah, if you're enforcing the check via GH, then you do get the yellow, but we aren't and we're trying to fit this into existing people's habits and workflows, so even where we're trialing it at small scale, people are coming back with questions 🤷 Hence looking to avoid more of these questions 😅

That said, are there any specific downsides to implementing this option? It seems that the change (as shown by OP), would need to live ~here:

policy-bot/server/handler/eval_context.go

Lines 144 to 146 in d4bf6b6

	case common.StatusSkipped:
	statusState = "error"
	statusDescription = "All rules were skipped. At least one rule must match."

- this could be made configurable - are there some pitfalls we've not covered?