Bulldozer 1.0 Github App rewrite #50
Conversation
There was a problem hiding this comment.
This is more of a spot check, since I didn't closely read all of the new code. Of things flagged, I think the commented out code and the policy-bot references are the only things that need to be fixed before merging this, but it would be nice to fix or discuss the other stuff as well.
.circleci/config.yml
Outdated
| ./godelw docker push --tags=latest,version | ||
| ./godelw publish bintray \ | ||
| --url https://api.bintray.com \ | ||
| --subject palantir --repository releases --product policy-bot \ |
There was a problem hiding this comment.
Need to fix the policy-bot references in this command (--product and --publish).
README.md
Outdated
| The `Bulldozer` is a bot that auto-merges PRs when all status checks are green and the PR is reviewed. | ||
| It uses [GitHub status checks](https://developer.github.com/v3/repos/statuses/) and [GitHub reviews](https://developer.github.com/v3/pulls/reviews/) to do this. | ||
| `bulldozer` is a [GitHub App](https://developer.github.com/apps/) that auto-merges | ||
| PRs when all status checks are green and the PR is reviewed |
README.md
Outdated
|
|
||
| * The server expects a config file name to be passed-in with the `-c` parameter (see `./bulldozer --help`). This config file should look like this. | ||
| - If the file does not exist, `bulldozer` will attempt to read from the configuration_v0_path, which is |
There was a problem hiding this comment.
If we want to call out the configuration_v0_path, I'd put that in a section on migrating from 0.4.x to 1.x. I think it is confusing for it to appear this early.
README.md
Outdated
|
|
||
| If the repository has branch protection active and enforces code reviews on PRs Bulldozer will wait until all status checks are green and at least one APPROVED review exists on the PR before merging it | ||
| We provide both a Docker container and a binary distribution of the server. A | ||
| sample configuration file is provided at `var/conf/bulldozer.yml`. We |
There was a problem hiding this comment.
Is var/conf/bulldozer.yml still the correct path or did that move somewhere else?
README.md
Outdated
| details. | ||
|
|
||
| ### Example Files | ||
| Example `bulldozer` files can be found in [`config/examples`](https://github.com/palantir/bulldozer/tree/develop/config/examples) |
server/handler/pull_request.go
Outdated
|
|
||
| package handler | ||
|
|
||
| /* |
There was a problem hiding this comment.
Should this be uncommented or should it be deleted instead?
| installationID := githubapp.GetInstallationIDFromEvent(&event) | ||
| baseRef := event.GetRef() | ||
|
|
||
| // todo: fixup PushEventRepository != Repository |
There was a problem hiding this comment.
Does this still need to happen? In what cases will these values not be equal?
There was a problem hiding this comment.
githubapp.PrepareRepoContext only accepts a github.Repository. The github.PushEvent only exposes a PushEventRepository type, which isn't accepted here. Since we only care about name and owner, we can just fake it. But it would probably be better to open a PR against githubapp to allow for a similar PrepareRepoContext that accepts org, repo string
|
|
||
| for _, pr := range prs { | ||
| pullCtx := pull.NewGithubContext(client, pr, owner, repoName, pr.GetNumber()) | ||
| logger := logger.With().Int(githubapp.LogKeyPRNum, pr.GetNumber()).Logger() |
There was a problem hiding this comment.
Should this use githubapp.PreparePRContext?
server/server.go
Outdated
| githubapp.WithClientMiddleware( | ||
| githubapp.ClientLogging(zerolog.DebugLevel), | ||
| githubapp.ClientMetrics(base.Registry()), | ||
| )) |
There was a problem hiding this comment.
style: put final closing ) on new line
server/server.go
Outdated
| }) | ||
| webhookHandler := githubapp.NewDefaultEventDispatcher(c.Github, | ||
| &handler.IssueComment{Base: baseHandler}, | ||
| //&handler.PullRequest{Base: baseHandler}, |
This change is