palantir/policy-bot

[Question] Is there a way to force rule checks?

Geoffrotism opened this issue · 3 comments

I would like to evaluate multiple PR title requirements using policy-bot but not on autogenerated PRs.
For example:

policy:
  approval:
    # Always allow whitesource and codegenie PRs
    - or:
      - Whitesource
      - CodeGenie
      - and:
        - PR Title starts correctly
        - PR Title ends correctly

# the list of rules
approval_rules:
  - name: PR Title starts correctly
    if:
      title:
        matches:
          - "^[a-zA-Z]+:"
    requires:
      count: 0
  - name: PR Title ends correctly
    if:
      title:
        matches:
          - "(\\[ENTPMTS-[0-9]+\\]\\s?)+$"
    requires:
      count: 0
  - name: CodeGenie
    if:
      has_author_in:
        users: ["codegenie[bot]"]
  - name: Whitesource
    if:
      has_author_in:
        users: ["whitesource[bot]"]

As per the documentation, it is my understanding that only 1 of the AND block needs to be evaluate for the entire policy to pass.
All I just want to show the status check as failed saying "Failed the end of the PR Title Format" or "Failed the beginning of the PR Title Format". Is this possible?

Policy Bot has a limited ability to post failures on pull requests by using disapproval policies. Unlike approval policies, disapproval only supports a single rule, so you can't opt-out specific bots. If you want to try this, you should use something like:

policy:
  disapproval:
    if:
      title:
        not_matches:
          - "^[a-zA-Z]+:"
          - "(\\[ENTPMTS-[0-9]+\\]\\s?)+$"

For the most part, Policy Bot is currently meant to implement review policies rather than arbitrary policies about PR details like titles or labels. If you want to "enforce" conditions, the strategy is to make rules that auto-approve (as you've started to do) and then have some fallback rule that is impossible to satisfy (e.g. because it requires too many reviews or approval from users that don't exist.) Note that Policy Bot must have at least one rule that is not skipped for every PR.

You'll also need to be careful with conditions. When a predicate is skipped because the condition is not true, the rule is removed from evaluation. In your example, the and block does not actually require that both patterns match because if one of the predicates is false (i.e. the pattern does not match), then the whole rule is removed from the block.

A policy like this might achieve what you want, but it can be kind of confusing:

policy:
  approval:
    - or:
      - Automated PR
      - PR title is correct
      - Other conditions are not met

approval_rules:
  - name: Automated PR
    if:
      has_author_in:
        users: ["codegenie[bot]", "whitesource[bot]"]
    requires:
      count: 0 # change this if you want to approve bot PRs

  - name: PR title is correct
    if:
      title:
        matches:
          - "^[a-zA-Z]+:.*(\\[ENTPMTS-[0-9]+\\]\\s?)+$"
    requires:
      count: 0

  - name: Other conditions are not met
    description: You must meet one of the other conditions to approve this PR
    requires:
      count: 9999 # an arbitrary large number that is impractical

thanks for the reply and suggestion! Not sure if it fits into the goals of policy-bot or is possible given the code, but would it be possible to:

  • add a required: true field on an approval block that tells policy-bot to not "skip" a rule.
  • If required:true and fails the if statement - This would then trigger the same functionality as disapprove block triggering (with the description populating the github check)
  • If required:true and passes the if statement - This would mark the block as Pass (like normal)
  • If required:false - Functionality of that approval block would run as it does right now.
  • If the required field is not provided (Default) - Functionality of that approval block would run as it does right now.

Again, I'm new to policy bot so I apologize if this goes against the principals or is impossible to implement.

If we were to implement something like this, I think one way to do it is to expand how disapproval works so that it can contain multiple rules and conditions, like approval policies. Disapproval rules probably would not have the concept of skipping, since it doesn't really make sense in that context.

Another option is to introduce a third type of policy, maybe conditions or preconditions, that allows you to use the existing predicates to enforce properties about the pull request. You could think of this as being the opposite of disapproval: preconditions specify things that must be true, while disapproval specifies things that must not be true. All of the preconditions must be true before Policy Bot evaluates the approval policy.

While your proposed required field would work, I think it adds too much complexity to approval rules and kind of contradicts the existing functionality by mixing in failure/disapproval with approval.

To set expectations, we don't currently have plans to expand disapproval support or implement a condition policy, as it's not something that comes up often internally at Palantir. But I'm happy to iterate on a design and pull request if you or anyone else would like to try implementing it.