palantir / policy-bot Goto Github PK
View Code? Open in Web Editor NEWA GitHub App that enforces approval policies on pull requests
License: Apache License 2.0
A GitHub App that enforces approval policies on pull requests
License: Apache License 2.0
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.
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.
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?
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.
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.
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
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 Predicate
s do not support nesting. Two options seem to be available:
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/.*"
nor
and nand
(see here). The rules would be evaluated and inversed.policy:
approval:
- or:
- the owner team has approved
- nor:
- code changed
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.
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.
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.
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.
// 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
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:}]
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.
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
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.
I've seen a couple of instances already where:
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.
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.
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.
A common workflow:
I make a PR, all the CI goes green, but the overall light is still stuck at yellow
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.
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
I go to the org homepage
Click teams
Search for my relevant team, click it
Click the members tab
Finally I can go message one of those people.
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.
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.)
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
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?
Currently the following sequence ends up in an approve:
This shouldn't have counted up as an approve, as the reviewer didn't provide any new information after requesting changes.
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.
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"]
Context:
Desired:
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
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?
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.
When enabling the bot for the first time, it should evaluate all open PRs and post status checks.
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.
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}}"]
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.
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:
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=="
...
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
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.
On a repository which I maintain, we have to keep a release branch system, so our release branches are named:
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.
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.
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:
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.
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 inIsApproved
. You can walk back thelastCommitOrder
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.
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.
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?
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
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
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.
policy-bot should handle PRs which change 300+ files (300 is a Github limitation for certain APIs).
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.
This bot should be able to read most of it configuration from environment variable as it's done for bulldozer (palantir/bulldozer#139)
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..."
}
A declarative, efficient, and flexible JavaScript library for building user interfaces.
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. ๐๐๐
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google โค๏ธ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.