palantir/policy-bot

A GitHub App that enforces approval policies on pull requests

policy-bot's Issues

Customize the policy-bot PR title to enable wildcard matching on branches

On a repository which I maintain, we have to keep a release branch system, so our release branches are named:

• release/3.19.10.1
• release/3.19.10.2
• release/3.19.10.3
• ...

In order to get status checks working in Github, I create a pattern branch protection pattern:

release/.*

Which allows me to add checks for everything, except for Policybot, which uniquely names every check as:

policy-bot: release/3.19.10.1
policy-bot: release/3.19.10.2
...

The Github UI unfortunately does not allow me to match on wildcard status checks, so I need the status check name to be constant, which for Policy bot it's not.

Commit listing issues in 1.7.x and 1.8.x

The new commit listing logic introduced in 1.7.0 also introduced several significant bugs that break various parts of the application. As a result, we don't recommend deploying the 1.7.0, 1.7.1, 1.8.0, and 1.8.1 releases.

This issue serves as a summary of the problems and a tracking issue for the final fix.

In 1.7.0, commit listing was switched to read the history of a commit from the head repository instead of from the pull request to populate the pushedDate field and fix invalidate_on_push for pull requests from forks. This relied on the commits field existing in the pull request object, which was not true for objects included in pull_request_review event payloads. The server would crash when processing these events.

This was fixed in 1.8.0 and revealed a more significant problem. By listing the history of a single commit, commits already on the target branch but included in the PR via a merge commit were also considered by the bot. This broke collaborator detection and could break file predicates, since the commit list is truncated to the number of commits GitHub reports as being part of the pull request.

Both of these issues were caused by bad assumptions made about the GitHub API that were not adequately tested before release. In addition to fixing the bug, we'll also be looking at better integration testing strategies.

Use Github Data API to support policy files greater than 1 MB

We have a generated policy file that is currently about 1.08 MB, and policy-bot fails to deal with it.

The error reported is:

Failed to fetch configuration at ref=develop: failed to fetch content of org/repo@master/central-policy.yml: GET https://githuburl/api/v3/repos/org/repo/contents/central-policy.yml?ref=master: 403 This API returns blobs up to 1 MB in size. The requested blob is too large to fetch via the API, but you can use the Git Data API to request blobs up to 100 MB in size. [{Resource:Blob Field:data Code:too_large Message:}]

[Enhancement] Support Not

We structure our projects so that all code is in a src/ folder, and currently have a policy that looks like the following:

- name: no code changed
    if:
      only_changed_files:
        paths:
          - "README"
          - "gitignore"
          - "gitattributes"
          - "github/.*"

It would be convenient if we didn't have to maintain this list and instead could approve if files in a folder didn't change.

Looking through the code, it appears that Predicates do not support nesting. Two options seem to be available:

1. Add a new YAML section in Rule for nots. The rules would be loaded and predicates evaluated as normal, except the predicate would be inversed (see here). Predicates are currently "and" for group logic which seems fine so long as it's documented

- name: no code changed
    not:
      changed_files:
        paths:
          - "src/.*"

2. Add new Policy group options of nor and nand (see here). The rules would be evaluated and inversed.

policy:
  approval:
    - or:
      - the owner team has approved
      - nor:
      	- code changed

Support for multiple partial approvals joining together

In repo we often have different sets of files that can be approved by different groups of people. Often a PR will include changes to multiple files that are owned by different groups. If we have several rules OR'd together with if_only_changed_files, then only PRs that touch files owned by a single group can be approved that way. Instead of all-or-nothing approval rules, we would like to have an approval be accepted for a subset of files in a PR, so it can be joined together with other approvals.

For example if group A owns files 1 and 2, group B owns files 3 and 4, and all other files, including new files (thus dynamic) require approval by an admin group C, it would be great if we could design a policy such that if someone submitted a PR touching files 2 and 4 policy bot would pass if someone from group A (covering file 2) and someone from group B (covering file 4) approve (but not if only one of them do). For something simple like this example one could maybe use an AND of changed_files rules, but it would be hard to cover the group C case without explicitly listing all the files in a negative lookahead (if that even works), which gets crazy if there are many files and many groups

FR: Handle approvals embedded in GHE review comments

Sometimes a user will make a slightly weird, but otherwise correct review, and we'll see something like

User1 reviewed with a comment:

 👍 

Policybot looks at the comment text and the result of the github approval, but not the text of the github approval. I believe this is covered in the slightly different reviews api, so these types of "approvals" aren't counted.

Read some config values from the environment

This bot should be able to read most of it configuration from environment variable as it's done for bulldozer (palantir/bulldozer#139)

Support top-level "age" policies

One of our internal projects has a high rate of PR creation, which means that PRs that are open for more than a day or two are likely to have semantic merge conflicts with the target branch. Due to the length of builds and the number of PRs, it's currently infeasible to use GitHub's strict required status checks.

One idea is this to add a new top-level policy element, tentatively called age:

policy:
  approval:
    # ...
  disapproval:
    # ...
  age:
    max_days: 4
    max_commits: 20

In the example above, if a PR has been open for more than 4 days (and the target branch has changed) or a PR is more than 20 commits behind the target branch, policy-bot would post a failed status check indicating that the PR needs to be updated.

I'd like to avoid both adding state to track PRs and running large queries to find open PRs in the case of a time window. Is it possible to support this in an event-driven way? Maybe evaluating open PRs in response to the push event on the target branch? It does mean that the status won't necessarily update at or close to the expiration time, only on the next push to the target branch or other evaluation action.

Are there any other conditions that would make sense as part of this policy?

Consider adding options to have the bot comment on the PR

We have invalidate_on_push=true and one of the most common reported issues is someone updating a PR without getting a new review. It would be great if the bot could add a comment on the PR to notify the requestor of this situation.

Reevaluate PRs when policy.yml file is changed

When the .policy.yml file is updated, existing PRs need some kind of action (comment, reopen, etc.) to trigger reevaluation with the new policy. We should either:

1. Listen to push events, watch for policy.yml changes on the default or target branch, and reevaluate open PRs
2. Provide an API endpoint someone can POST to that will reevaluate all open PRs on a repository

author_is_only_contributor does not work with file edits from Github UI

If you edit a file in the Github UI directly and create a PR from there, the author_is_only_contributor condition does not hold.

For example I made the following policy on a test project:

policy:
  approval:
    - dwalker is author

approval_rules:
  - name: dwalker is author
    if:
      author_is_only_contributor: true
      has_author_in:
        users: ["dwalker"]
    options:
      invalidate_on_push: true
    requires:
      count: 0

and then created a test PR. The "dwalker is author" rule was skipped and had the message "Commit 5d0b04b219 was authored or committed by a different user". Looking at the commit you can see indeed Github is listed as the committer, not me:

$ git show --pretty=fuller 5d0b04b219
commit 5d0b04b2197e4442616269a24eb9efa092a682e9 (origin/dwalker-patch-1)
Author:     Dan Walker <redacted>
AuthorDate: Thu Jun 6 15:53:27 2019 -0400
Commit:     GitHub Enterprise <noreply@redacted>
CommitDate: Thu Jun 6 15:53:27 2019 -0400

    Create test

Is there a way to make this still work?

policy-bot triggering Github abuse detection mechanism

We use dependabot in combination with an auto-approve policy-bot rule, and although it may happen at other high-traffic times, we're definitely seeing many dependabot PRs go unapproved. We've traced this back to Github, who is returning our policy-bot a 403 abuse response.

Probably this is not actionable (unless we can slow down dependabot or someone has knowledge of ways to tell Github to relax its abuse protection), and is simply another use case for policy-bot holding state and acting asynchronously (as mentioned in #135) ... eg it could back off and retry outside of the scope of a single request.

sample log line:

failed to get pull request goodeggs/keyboard-barcode-scan-listener#185: GET https://api.github.com/repos/goodeggs/keyboard-barcode-scan-listener/pulls/185: 403 You have triggered an abuse detection mechanism. Please wait a few minutes before you try again.

Put "pending" polices at top of app

When there are many policies defined for a project, it seems they are displayed in order of definition and it can take a long time to figure out which policies need to pass still, could we change the display sorting such that pending polices are at top, then passed, then skipped or similar?

[Enhancement] Approve if other status check has passed

Github status check enforcement is an "all must pass" system. This poses a problem for our workflow since we only trigger builds if that portion of the code has changed - we would prefer to use a "one or the other" style system as a minimum, and preferably a "if this code section changed, require this build" setup down the line

I would like to setup an approval like so:

policy:
  approval:
    - and:
      - code has changed
      - or:
        - the API build has passed
        - the Client build has passed
      - the deploy has passed

approval_rules:
    - name: the API build has passed
      if:
        status_check_passed: My.Company.Project.API - CI

policy-bot already has read access for Status checks, but it does not appear to be currently possible to setup the above approval step.

401 when getting installation token

I have the AppId, ClientId, ClientSecret, and PrivateKey all setup in my .yml

I have verified that https://github.Company.com/api/v3/repos/Org/Repo/contents/.github/.policy.yml?ref=master correctly returns my policy file when hit with my own token. I have also verified that my installation is indeed install 5

It seems that I am still missing a step or configuration setting to get this working fully. I apologize for hitting constant walls on this and appreciate your help

2019-07-16T14:18:31.858804922Z |INFO| Server listening on 0.0.0.0:8080


2019-07-16T14:19:02.629873646Z |INFO| Received webhook event github_delivery_id=3c7.. github_event_type=pull_request rid=bkm..


2019-07-16T14:19:02.634483197Z |DEBUG| attempting to fetch policy definition for Org/Repo@master/.github/.policy.yml github_delivery_id=3c7.. github_event_type=pull_request github_installation_id=5 github_pr_num=0 github_repository_name=Repo github_repository_owner=Org github_sha=c2.. rid=bkm..


2019-07-16T14:19:02.756091192Z |DEBUG| github_request elapsed=120.844147 github_delivery_id=3c7.. github_event_type=pull_request github_installation_id=5 github_pr_num=230 github_repository_name=Repo github_repository_owner=Org github_sha=c20.. method=GET path=https://github.Company.com/api/v3/repos/Org/Repo/contents/.github/.policy.yml?ref=master rid=bkm.. size=-1 status=-1


2019-07-16T14:19:02.757219945Z |ERROR| Unexpected error handling webhook request error="failed to fetch policy: Org/Repo ref=master: failed to fetch content of Org/Repo@master/.github/.policy.yml: Get https://github.Company.com/api/v3/repos/Org/Repo/contents/.github/.policy.yml?ref=master: could not refresh installation id 5's token: received non 2xx response status \"401 Unauthorized\" when fetching https://github.Company.com/api/v3/installations/5/access_tokens\ngithub.com/palantir/policy-bot/server/handler.(*ConfigFetcher).fetchConfigContents\n\t/go/src/github.com/palantir/policy-bot/server/handler/fetcher.go:178\ngithub.com/palantir/policy-bot/server/handler.(*ConfigFetcher).fetchConfig\n\t/go/src/github.com/palantir/policy-bot/server/handler/fetcher.go:107\ngithub.com/palantir/policy-bot/server/handler.(*ConfigFetcher).ConfigForPR\n\t/go/src/github.com/palantir/policy-bot/server/handler/fetcher.go:85\ngithub.com/palantir/policy-bot/server/handler.(*Base).Evaluate\n\t/go/src/github.com/palantir/policy-bot/server/handler/base.go:142\ngithub.com/palantir/policy-bot/server/handler.(*PullRequest).Handle\n\t/go/src/github.com/palantir/policy-bot/server/handler/pull_request.go:47\ngithub.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp.(*eventDispatcher).ServeHTTP\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp/dispatcher.go:118\ngithub.com/palantir/policy-bot/vendor/goji%2eio.dispatch.ServeHTTP\n\t/go/src/github.com/palantir/policy-bot/vendor/goji.io/dispatch.go:17\ngithub.com/palantir/policy-bot/vendor/github.com/bluekeyes/hatpear.Recover.func1.1\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/bluekeyes/hatpear/hatpear.go:107\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:1995\ngithub.com/palantir/policy-bot/vendor/github.com/bluekeyes/hatpear.Catch.func1.1\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/bluekeyes/hatpear/hatpear.go:60\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:1995\ngithub.com/palantir/policy-bot/vendor/github.com/rs/zerolog/hlog.AccessHandler.func1.1\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/rs/zerolog/hlog/hlog.go:180\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:1995\ngithub.com/palantir/policy-bot/vendor/github.com/rs/zerolog/hlog.RequestIDHandler.func1.1\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/rs/zerolog/hlog/hlog.go:169\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:1995\ngithub.com/palantir/policy-bot/vendor/github.com/palantir/go-baseapp/baseapp.NewMetricsHandler.func1.1\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/palantir/go-baseapp/baseapp/middleware.go:55\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:1995\ngithub.com/palantir/policy-bot/vendor/github.com/rs/zerolog/hlog.NewHandler.func1.1\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/rs/zerolog/hlog/hlog.go:30\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:1995\ngithub.com/palantir/policy-bot/vendor/goji%2eio.(*Mux).ServeHTTP\n\t/go/src/github.com/palantir/policy-bot/vendor/goji.io/mux.go:74\nnet/http.serverHandler.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2774\nnet/http.(*conn).serve\n\t/usr/local/go/src/net/http/server.go:1878\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1337" github_delivery_id=3c7c1bf0-a7d5-11e9-841e-40a3482ae65d github_event_type=pull_request rid=bkmtplj11808ga9g3on0


2019-07-16T14:19:02.757655261Z |INFO| http_request elapsed=130.294199 method=POST path=/api/github/hook rid=bkmtplj11808ga9g3on0 size=22 status=500

FR: include ability to add all authorized reviewers to pull request

We work in a monorepo with very disparate teams. It would be nice if every authorized reviewer was added to a PR instead of picking at random until the requires.count limit is met. In particular, we'd like to be able to automatically request reviews from entire teams that own those portions of the codebase.

Support rule defaults for common options

It would be helpful if policy-bot support a way to set default options for all rules in a policy without copying code or using YAML references. I'm imagining something like this:

policy:
  approval:
    - rule using defaults

approval_defaults:
  options:
    invalidate_on_push: true
    ignore_update_merges: true

approval_rules:
  - name: rule using defaults
    requires:
      count: 1
      teams: ["palantir/devtools"]

Handle review dismissal event

If a GH review is performed with the result of request changes policybot will correctly fail the status check. If a person dismisses that review policybot will not reset the status check to pending.

FR: auto-request review after a short delay

What happened?

I've seen a couple of instances already where:

1. someone opens a PR,
2. then as soon as it's opened tries to request a specific reviewer,
3. policy-bot also races to assign reviewers (and usually wins)

This results in more people being notified than necessary, and one of my goals with the auto request reviews thing is to try and reduce notifications to exactly those necessary to get stuff done and no more.

What did you want to happen?

My proposal is to introduce a short delay to accommodate human forgetfulness, so that if people create a PR and then immediately assign a reviewer, policy-bot won't touch the PR.

address policy-bot 3000 file PR limit

policy-bot should handle PRs which change 300+ files (300 is a Github limitation for certain APIs).

Re-evaluate Approval Status on membership change events

P3: Policybot should accept webhooks for org changes and update Approvals accordingly.

For example, on #23, I approved before being a member of the devtools team and was correctly identified as not giving permission. Once I was added, Policybot didn't re-evaluate until a comment was posted.

Marking as a p3 because it is relatively low effort to post a comment, but it would be a nice quality of life update.

Dynamic team approval requirements based on file path

It would be great if we could somehow specify that the team required for approval for a rule would be dynamically determined based on the file path.

As an example, in MyCloud we have deployment-specific files in several repos whose path is a particular structure, and includes the MyCloud identifier. We want to be able to have a rule, therefore, that says if there are changes only to a specific deployment's files, allow members of that deployment's team to approve. We can have explicit rules for each identifier to do this, but there are over 100 identifiers and there is churn, so it would be awesome if we could just specify one rule that is dynamic.

An example implementation would be having capture groups in the file path regex, and then being able to use the captured text in the list of teams that can approve for that rule

rough outline of a spitball idea we talked about:

  - name: match a team to a file
    if:
      changed_files:
        paths:
          - "^folder/{{captured}}/somefile\\.yml$"
    requires:
      count: 1
      teams: ["team-{{captured}}"]

Github hook error with some PRs update

Hi,

Policy bot is working fine since more than 6 months and we face an issue when we create pull requests (~200/250 files) from a release branch to master branch: "panic: runtime error: invalid memory address or nil pointer dereference"

The stack trace is really strange and we do not understand what is going on. We suspect an issue with the githubapp hook that send logs/metrics when we try to get datas from github membership config.

For the same hook message, sometimes it works fine, sometime it fails. (we can retry previous event hook form github UI and check roundtrip result)

Here is one stacktrace:

2019-11-12T15:12:21.654223183Z |DEBUG| rule evaluation resulted in pending:"0/1 approvals required" github_delivery_id=cc3f8100-055e-11ea-8437-c05dd1c52794 github_event_type=pull_request github_installation_id=1007339 github_pr_num=1183 github_repository_name=yann-platform github_repository_owner=yannou github_sha=985b2acc17e97f137119ff4a2382e5a04ffd5163 rid=bn5cnj2ara13u0ug6k50 rule="PMs must approve PR to master branch"
2019-11-12T15:12:21.654289960Z |INFO| Setting "policy-bot: master" status on 985b2acc17e97f137119ff4a2382e5a04ffd5163 to pending: 0/1 rules approved github_delivery_id=cc3f8100-055e-11ea-8437-c05dd1c52794 github_event_type=pull_request github_installation_id=1007339 github_pr_num=1183 github_repository_name=yann-platform github_repository_owner=yannou github_sha=985b2acc17e97f137119ff4a2382e5a04ffd5163 rid=bn5cnj2ara13u0ug6k50
2019-11-12T15:12:22.184688541Z |ERROR| Unhandled error while serving route error="panic: runtime error: invalid memory address or nil pointer dereference\nruntime.gopanic\n\t/usr/local/go/src/runtime/panic.go:522\nruntime.panicmem\n\t/usr/local/go/src/runtime/panic.go:82\nruntime.sigpanic\n\t/usr/local/go/src/runtime/signal_unix.go:390\ngithub.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp.cacheControl.func1.1\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp/client_creator.go:344\ngithub.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp.roundTripperFunc.RoundTrip\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp/middleware.go:151\ngithub.com/palantir/policy-bot/vendor/github.com/gregjones/httpcache.(*Transport).RoundTrip\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/gregjones/httpcache/httpcache.go:214\ngithub.com/palantir/policy-bot/vendor/github.com/bradleyfalzon/ghinstallation.(*Transport).RoundTrip\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/bradleyfalzon/ghinstallation/transport.go:94\ngithub.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp.ClientMetrics.func1.1\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp/middleware.go:64\ngithub.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp.roundTripperFunc.RoundTrip\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp/middleware.go:151\ngithub.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp.ClientLogging.func1.1\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp/middleware.go:121\ngithub.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp.roundTripperFunc.RoundTrip\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp/middleware.go:151\ngithub.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp.setInstallationID.func1.1\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp/client_creator.go:369\ngithub.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp.roundTripperFunc.RoundTrip\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp/middleware.go:151\nnet/http.send\n\t/usr/local/go/src/net/http/client.go:250\nnet/http.(*Client).send\n\t/usr/local/go/src/net/http/client.go:174\nnet/http.(*Client).do\n\t/usr/local/go/src/net/http/client.go:641\nnet/http.(*Client).Do\n\t/usr/local/go/src/net/http/client.go:509\ngithub.com/palantir/policy-bot/vendor/github.com/google/go-github/github.(*Client).Do\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/google/go-github/github/github.go:513\ngithub.com/palantir/policy-bot/vendor/github.com/google/go-github/github.(*RepositoriesService).CreateStatus\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/google/go-github/github/repos_statuses.go:81\ngithub.com/palantir/policy-bot/server/handler.(*Base).postGitHubRepoStatus\n\t/go/src/github.com/palantir/policy-bot/server/handler/base.go:115\ngithub.com/palantir/policy-bot/server/handler.(*Base).PostStatus\n\t/go/src/github.com/palantir/policy-bot/server/handler/base.go:98\ngithub.com/palantir/policy-bot/server/handler.(*Base).EvaluateFetchedConfig\n\t/go/src/github.com/palantir/policy-bot/server/handler/base.go:199\ngithub.com/palantir/policy-bot/server/handler.(*Base).Evaluate\n\t/go/src/github.com/palantir/policy-bot/server/handler/base.go:150\ngithub.com/palantir/policy-bot/server/handler.(*PullRequest).Handle\n\t/go/src/github.com/palantir/policy-bot/server/handler/pull_request.go:52\ngithub.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp.(*eventDispatcher).ServeHTTP\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/palantir/go-githubapp/githubapp/dispatcher.go:178\ngithub.com/palantir/policy-bot/vendor/goji%2eio.dispatch.ServeHTTP\n\t/go/src/github.com/palantir/policy-bot/vendor/goji.io/dispatch.go:17\ngithub.com/palantir/policy-bot/vendor/github.com/bluekeyes/hatpear.Recover.func1.1\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/bluekeyes/hatpear/hatpear.go:107\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:1995\ngithub.com/palantir/policy-bot/vendor/github.com/bluekeyes/hatpear.Catch.func1.1\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/bluekeyes/hatpear/hatpear.go:60\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:1995\ngithub.com/palantir/policy-bot/vendor/github.com/rs/zerolog/hlog.AccessHandler.func1.1\n\t/go/src/github.com/palantir/policy-bot/vendor/github.com/rs/zerolog/hlog/hlog.go:180" method=POST path=/api/github/hook rid=bn5cnj2ara13u0ug6k50
2019-11-12T15:12:22.185130326Z |INFO| http_request elapsed=9664.741379 method=POST path=/api/github/hook rid=bn5cnj2ara13u0ug6k50 size=33 status=500
2019-11-12T15:12:22.697482102Z |INFO| Received webhook event github_delivery_id=d352d410-055e-11ea-9e02-785d94577c26 github_event_type=status rid=bn5cnliara13u0ug6k60
2019-11-12T15:12:22.698192766Z |INFO| http_request elapsed=0.8035 method=POST path=/api/github/hook rid=bn5cnliara13u0ug6k60 size=0 status=200

We use latest version 1.12.3 of policy-bot deployed with docker/k8s on GCP with a traefik in between.

On fail we can found an error in github UI, in the policy bot app config advanced recent deliveries: "We couldn’t deliver this payload: Service Timeout"

Any clue would help here.

Thank you for

Dismissing negative review shouldn't use prior approve

Currently the following sequence ends up in an approve:

1. Reviewer approves
2. Reviewer requests changes (e.g. because an issue was spotted later on)
3. PR author dismisses request changes (as they've addressed the feedback)

This shouldn't have counted up as an approve, as the reviewer didn't provide any new information after requesting changes.

Allow writing policies which mention repo 'admins' or 'collaborators'

I want to make sure certain excavator changes can merge without human intervention in every single repo in foundry. The problem is that I need to generate a policy.yml that is appropriate for each repo, taking into account all their different collaborators and admins.

Proposal

Allow the values 'admins' and 'collaborators' in policy.yml files, e.g.:

policy:
  approval:
    - or:
      - maintainer approval
      - excavator only touched gradle files

approval_rules:

  - name: maintainer approval
    requires:
      count: 1
      github:
        - admins # <--- policy-bot would expand this to all users & teams with admin access
        - collaborators

  - name: excavator only touched gradle files
    requires:
      count: 0
    if:
      has_author_in:
        users: [ "svc-excavator" ]
      only_changed_files:
        paths:
          - "^.*gradle$"

This ensures there's one source of truth for who is in control of a repository and for the moment it's the github settings tab.

FR: UI should expand which teams (and users) can actually approve something

What happened

A common workflow:

1. I make a PR, all the CI goes green, but the overall light is still stuck at yellow

2. I click through to the policy bot UI to see what I need to do to get my thing merged
   
   Unfortunately the UI doesn't tell me who I need to ping.

3. I go back to the main file browser, hit t to browse files, then read the policy.yml file to see which teams are considered maintainers

4. I go to the org homepage

5. Click teams

6. Search for my relevant team, click it

7. Click the members tab

Finally I can go message one of those people.

What should have happened

I'd like this to require less steps - ideally, I'd like the policy-bot UI (and reported commit status summary) to give me a bit more information about who I need to go talk to.

Status audit descriptions are too long

Hi there,

Setting up policy-bot for the first time, I seem to be having a problem posting back status checks; running the server on debug mode shows that the github API is returning a 422 error for the description field of something:

2019-04-24T19:53:48.877708064Z |INFO| Server listening on 0.0.0.0:8080                           
2019-04-24T19:54:01.617921615Z |INFO| Received webhook event github_delivery_id=a076d8fe-66bf-11e9-90b4-5f63d75174d3 github_event_type=status rid=bj0btm9mqqplijrec0hg                                                                    
2019-04-24T19:54:01.619487450Z |WARN| Entity 'policy-bot[bot]' overwrote status check 'policy-bot: master' on ref=7f5271ba1aa085f8d62b55756e6b889b5af967f7 to status='pending' description='0/1 rules approved' targetURL='https://pbot.name.org/details/githuborg/reponame/54' audit=status github_delivery_id=a076d8fe-66bf-11e9-90b4-5f63d75174d3 github_event_type=status github_installation_id=865320 github_repository_name=reponame github_repository_owner=githuborg rid=bj0btm9mqqplijrec0hg
2019-04-24T19:54:01.836555577Z |DEBUG| github_request elapsed=216.917109 github_delivery_id=a076d8fe-66bf-11e9-90b4-5f63d75174d3 github_event_type=status github_installation_id=865320 github_repository_name=reponame github_repository_owner=githuborg method=POST path=https://api.github.com/repos/githuborg/reponame/statuses/7f5271ba1aa085f8d62b55756e6b889b5af967f7 rid=bj0btm9mqqplijrec0hg size=252 status=422                                                                                             
2019-04-24T19:54:01.837172027Z |ERROR| Unexpected error handling webhook request error="POST https://api.github.com/repos/githuborg/reponame/statuses/7f5271ba1aa085f8d62b55756e6b889b5af967f7: 422 Validation Failed [{Resource:Status Field:description Code:custom Message:description is too long (maximum is 140 characters)}]" github_delivery_id=a076d8fe-66bf-11e9-90b4-5f63d75174d3 github_event_type=status rid=bj0btm9mqqplijrec0hg                                                                            
2019-04-24T19:54:01.837235280Z |INFO| http_request elapsed=219.432713 method=POST path=/api/github/hook rid=bj0btm9mqqplijrec0hg size=22 status=500

.policy.yml

policy:                                                                                                                                                                                                                                                                             
  approval:
  - or:
    - deploy updates
    - submodule updates
    - at least one human approval

approval_rules:
- name: at least one human approval
  options:
    invalidate_on_push: true
  requires:
    count: 1

- name: deploy updates
  options:
    invalidate_on_push: true
  if: 
    only_changed_files:
      paths:
      - '^deploy/.*'

- name: submodule updates
  options:
    invalidate_on_push: true
  if: 
    only_changed_files:
      paths:
      - '^src/.*'

My server config is pretty standard-looking, I'm not sure what else to include to help identify the issue. Any ideas would be greatly appreciated.

Consider auditing status updates by installation ID

Currently, we audit for malicious status updates by looking at the name of the user who created the status and matching it against the name configured in the server. This has led to several issues (#72, #107) when configuring the server, as it is easy to set the wrong value for options.app_name or to omit it completely.

If possible, it would be better to audit status events by a property that is known without configuration, like the installation ID.

FR: Add a min_lines_of_code predicate

In my repository, I'd like to require special approval for pull requests over a certain size. E.g. if more than 1000 lines of code have been modified, a certain set of people must approve the pull request.

Would it be possible to add a predicate to support this?

Github appends :master to context

When using status_check_context: "policy-bot",

Github ends up sending a webhook with the payload

"context": "policy-bot: master",
"description": "All rules are approved",

policy-bot then doesn't recognize the context as matching and triggers off it's own event, creating an infinite loop of processing and causing a failure

  "context": "policy-bot: master",
  "description": "'policy-bot[bot]' overwrote status to 'failure'",

Swapping the context to policy-bot: master unfortunately isn't a workaround:

  "context": "policy-bot: master: master",
  "description": "'policy-bot[bot]' overwrote status to 'failure'",

I am using the latest Dockerhub image of policy-bot and Github Enterprise 2.17

Checking the payload, it looks like it's more appropriate to check the installation section of the payload:

  "installation": {
    "id": 5,
    "node_id": "MDI..."
  }

Don't allow multiple approvals by the same person

We recently enabled a co-contributor approval option, where you allowed approvals from contributors, but required two approvals. The behaviour of this however was to allow the same person to approve twice (eg if they had at least two comments with a 👍 ). I don't think this is desirable behaviour - one person's approval should only ever count once.

Support diff types in file predicates

It would be helpful to specify different approval rules for additive-only changes to certain files. This should be easy to add in code (the File abstraction already contains the information), so the main question is how to configure this without making the default path configuration too hard.

Maybe something like this?

if:
  files_changed:
    paths:
      - some/path
      - some/path:
          mode: additive

That would be a bit annoying to parse. Maybe the mode could be specified at the top-level and applies to all paths in this predicate?

if:
  files_changed:
    mode: additive
    paths:
      - some/path
      - some/path

If you want different modes on different paths, you'd have to use multiple rules, but that might actually be fine or desired for the common case (i.e. auto-approval of certain files.)

switch to using github api v4

we should look into switching to using the graphql version of the API for GHE. this would give us pushed timestamps, making invalidate_on_push a lot simpler.

a sample query for this would look like:

{
  repository(owner: "gradle", name: "gradle") {
    pullRequest(number: 5813) {
      comments(first: 100) {
        pageInfo {
          endCursor
          hasNextPage
        }
        nodes {
          body
          createdAt
        }
      }
      reviews(first: 100) {
        nodes {
          createdAt
          author {
            login
          }
          state
        }
      }
      commits(first: 100) {
        nodes {
          commit {
            pushedDate
            author {
              user {
                login
              }
            }
            committer {
              user {
                login
              }
            }
          }
        }
      }
    }
  }
}

As a note, we still need to use the v3 API for changed files, since these are not exposed via GraphQL at this time.

add a has_label_in or similar approval rule predicate

I'd like to be able to apply a set of approval rules based on a label, commit message content, or similar marker on a PR that indicates some special set of circumstances that require different approval rules. The use case is something like the following:

• a user wants to merge a PR that would ordinarily be blocked by some failing automated tests or other policy
• they might have the ability to silence or bypass the automated checks in some way
• if they do this, the PR should require special approval from some set of important people who are vouching for the change being OK to submit, even though checks are being explicitly skipped
• to indicate that, the user adds a "Requires-Lead-Approval" label (or similar) to the PR

I'd imagine the configuration looking something like:

approval_rules:
  - name: important people have approved a scary change
    if:
      has_label_in:
        label: ["Requires-Lead-Approval"]
    requires:
      count: 1
      teams:
        - "ImportantGatekeepers"

Alternative to the label approach might be something where a commit message or the PR description has a special marker, e.g.

approval_rules:
  - name: important people have approved a scary change
    if:
      description_has: "==API_BREAK=="
...

Support remote policy files in private repositories

Hello,

I would like to know why the remote policy configuration has to be full public.
We would like to use it with a github entreprise on premise version where everything is private.

Since policy bot is installed on the organization level, I do not see any blocker to also allow the remote configuration policy to be access privately (using token).

Thx in advance for clarification, I can help to provide this feature.

Add users capable of approving a change as reviewers

// tagging @whickman as the original reporter

We have several workflows which depend on users knowing which PRs they are on the hook to approve, right now this is very hard to do with policy bot because it does not request reviewers. We'd like it if there was some ability to do this

invalidate_on_push fails open if GitHub APIs are delayed

We're currently investigating an internal issue where GitHub Enterprise seems to be sending webhooks before the commits referenced by those hooks are available in other APIs. Because policy-bot uses only the PR timeline to find new commits, it can end up posting an incorrect approved status on the latest commit.

At minimum, we should make sure that the commit referenced by the webhook we are processing appears in the commit list. If it does not, we could delay and try again or immediately fail. Either option will be more correct than the current behavior.

Proposal: replace allow_contributor option with allow_non_author option

Because allow_contributor includes the PR author and overrides the allow_author option, you are unable to achieve the following workflow:

Scenario: you want to enable an approver who sees a small typo in an otherwise good PR to fix it. However, this policy doesn't quite do what you want because allow_contributor overrides allow_author.

    options:
      allow_contributor: true
      allow_author: false

Proposal is to deprecate allow_contributor and instead have allow_non_author which functions the same but is mutually exclusive.

Policy Bot 404 on webhook post

I am connecting to a private Enterprise org deployed to the same internal network as the policy-bot docker image.

I have my settings setup as so:

github:
  # The URL of the GitHub homepage
  web_url: "https://github.<company>.com"
  # The base URL for v3 (REST) API requests
  v3_api_url: "https://github.<company>.com/api/v3"
  # The base URL for v4 (GraphQL) API requests
  v4_api_url: "https://github.<company>.com/api/graphql"

I have verified I can hit these endpoints myself via a REST client.

When a webhook comes in, the container is dropping a 404 error {"level":"info","rid":"bkmf1qr11808ga9im6jg","method":"POST","path":"/","status":404,"size":19,"elapsed":0.06792,"time":"2019-07-15T21:32:27.549643814Z","message":"http_request"}

I'm not sure what method is generating the 404 unfortunately, making this error hard to debug on my end

Verify the validity of policy.yml files before PRs merge

Context:

• currently it is possible to open a PR to a repo containing an invalid policy.yml file, because there is no pre-merge validation on this

Desired:

• a red CI light (a status) from policy-bot preventing me from merging invalid policy.yml files.

More secure ignore_update_merges option

supposing require branch up to date is required and ignore_update_merges option is set to true, it is currently possible to use the merge conflict editor in the UI to sneak a change past policybot. A user with an approved pr with merge conflicts can use the merge conflict editor to make additional changes. Policybot will consider this pr to just be a merge and so will not invalidate the change

Random thumbs up in comment shouldn't approve PR

Currently, if someone comments 👍, it counts as approving the PR. This is fine if the 👍 is the whole comment, but if someone says "👍 on cleaning up code", it shouldn't count as approval.

Check doesn't get removed when base branch changes

Steps to reproduce:

• change the base branch from develop to branch-foo
• a check is created policy-bot: branch-foo
• change base branch back to develop
• the check for policy-bot: branch foo is still there.

Contributors ignored during a single evaluation

Edit: see comment for a summary of the underlying bug.

---

I left a comment on a single line of this PR (palantir/gradle-conjure#136), but it seems it caused the entire thing to merge.

Interestingly, the UI seems to still say the PR has not yet been approved, but the status check went green so bulldozer mnerged it.

Support "only_has_contributor_in"

It would be useful to be able to design a policy that only allows PRs that are signed off on by a team by nature of either approving or by being the author/sole contributor. Currently we can create a policy with a top-level OR of two rules - 1) if has author in desired team, allow approval from broad group 2) allow approval from desired team. However, with this policy if the desired team opens a PR and then someone from outside that team pushes an arbitrary update to the branch, anyone in the broad group can approve, so that update is not signed off on by the desired team.

If we had a "only_has_contributor_in" we could combine that with has_author_in to ensure that edge case cannot happen. Open to other ways of accomplishing this as well!

This is useful for a workflow where the desired team that owns a particular process is small, so it is unfortunate to block those PRs, created by that team, on an approval from another person in that team.

Permission failure for private repos

In 1.4.0 we started using the timeline API, which seems to have some slightly unexpected permission changes for private repos. The result is that policy-bot is unable to evaluate policy ymls for private repos.

Error in the UI:

failed to compute approval status: failed to get approval candidates: failed to fetch comments: failed to list pull request timeline: Resource not accessible by integration

I think the fix is to probably update the required permissions, but we should test it out first.

Allow branch updates/merges from target without invalidation

This makes it possible to keep branches up-to-date without (1) requiring reapproval and (2) counting the person who clicked merge as a contributor.

Thoughts on the implementation from the internal issue:

I'm not sure how to detect this... maybe by allowing merge commits that don't themselves add any new changes or files to the branch?

In terms of implementation, there are two parts to consider: (1) how to identify a safe/clean commit and (2) how to ignore those.

The second is easiest: take a look at the InvalidateOnPush handling in IsApproved. You can walk back the lastCommitOrder until the first unsafe commit. You'll also need to adjust the logic for finding contributors to ignore the authors of safe commits; that's here.

For identifying commits, a good starting point is finding merge commits. This should be easy as they'll have more than one parent. Unfortunately, merge commits can also modify code, as when conflicts are resolved. I think you could do this by looking up the tree associated with the commit and seeing if it is empty, but I'm not quite sure how this is represented in Git.

We'll also have to decide if we only want to allow commits created by GitHub (or cleverly faked by users) or if local merges can count as well.

The last part is integrating that into the GitHub implementation of pull.Context, which wraps all the API logic and handles caching for the duration of a request. Try to minimize API calls, but correctness is more important than efficiency.

Allow approval based on source branch name

For example,

if:
  with_source_branch:
    pattern: "^(regexPattern)$"

This would be useful for PRs automatically opened/frequently opened on the same branch, e.g. to update dependencies

could not parse private key: Invalid Key: Key must be PEM encoded PKCS1 or PKCS8 private key

I am using the latest Dockerhub image for policy-bot and cannot successfully start the container to the error Error: failed to initialize Github app client: could not parse private key: Invalid Key: Key must be PEM encoded PKCS1 or PKCS8 private key

I have attempted to use various formats for the .pem generated by Github, such as:

private_key: "-----BEGIN RSA PRIVATE KEY-----
MIIEpAIBAAKCAQEArjfzbTEsi4kxQTi73Weh9zKmxCZ7nu3osRUM7ovd1XEnRK6z
. . .
HH3A+JuNscHTnGjTm2dmaTjTyEnCQxNQ8Z0NSxKVytOAymxXiiSyxg==
-----END RSA PRIVATE KEY-----"

Putting it all on one line or removing the -----BEGIN RSA PRIVATE KEY----- and -----END RSA PRIVATE KEY----- does not work as well.

I took a look through the code to try and see what I was doing wrong, but unfortunately I have never worked with Go before and only got as far as determining that you are somewhere converting the base64 content into a byte array

Evaluate open PRs when installed on a repository

When enabling the bot for the first time, it should evaluate all open PRs and post status checks.