Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Geoffrotism opened this issue Aug 29, 2022 · 3 comments
Open

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

Geoffrotism opened this issue Aug 29, 2022 · 3 comments

Comments

@Geoffrotism
Copy link

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?

@bluekeyes
Copy link
Member

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

@Geoffrotism
Copy link
Author

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.

@bluekeyes
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants