search

palantir / policy-bot

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

Add Option to not post failure status to github when there is no successful policy #711

Closed RoryDoherty closed 4 days ago

RoryDoherty commented 4 months ago

This should address the following issue:

https://github.com/palantir/policy-bot/issues/709

palantirtech commented 4 months ago

Thanks for your interest in palantir/policy-bot, @RoryDoherty! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

RoryDoherty commented 4 months ago

I also think the settings in config could be used to implement this: https://github.com/palantir/policy-bot/issues/387 Could have another option called approve_pr_on_success_status

bluekeyes commented 4 months ago

Thanks for your contribution! Could you please recreate this as two PRs, one that adds the equality operator and one for the status changes? The equality operator seems straightforward and should be easy to merge, while the status behavior is something I'd like to think about more. While the code change is minimal, it adds some new concepts to Policy Bot that deserve consideration.

RoryDoherty commented 4 months ago

@bluekeyes thanks for the feedback, I've updated this PR so that it is just the option to not set failure status checks and opened this PR https://github.com/palantir/policy-bot/pull/712 with the changes for adding the equals operator for file modifications

RoryDoherty commented 4 months ago

@bluekeyes would you be able to take a look at this PR anytime soon? I'm eager to also implement https://github.com/palantir/policy-bot/issues/387 as these missing features are blocking us from currently rolling this out I don't want to start on 387 however until I know your happy with the config settings being in the .policy.yml file

bluekeyes commented 4 months ago

Sorry about the delay getting back to this. As I mentioned on the linked issue, I think this feature is something we can add with two changes:

  1. Only skip posting a status when there are no matching rules. If there are matching rules, pending and failure status should still be posted.
  2. Make the option to configure this a feature of the server configuration rather than the policy. We use Policy Bot on thousands of repositories and we want to make sure the behavior is consistent, where individual projects cannot opt-in to this behavior. A server-level flag would allow people who don't care about this consistency to get the skipping behavior.
RoryDoherty commented 4 months ago

Unfortunately the changes you've suggested would make this feature unusable for us

  1. If a matching policy fails I just want it to be ignored completely, I wouldn't want to post the failure status as it unnecessarily adds failed checks to a commit which would make people think something was wrong with the commit
  2. Would it be possible to have this as a server level flag to allow the setting in the policy file, This way you can have repos mix and match but only if the server has the option enabled? This would save from having to deploy two different instances to get the same feature set