From dd73a99f1841ea2a9d73aba6e8af01a85baec785 Mon Sep 17 00:00:00 2001 From: Zero King Date: Tue, 15 Aug 2017 09:25:52 +0000 Subject: [PATCH] Handle maintainer timeout --- pr/cron/maintainer_timeout.go | 78 +++++++++++++++++++++++++++++ pr/db/dbutil.go | 82 +++++++++++++++++++++++++++++++ pr/githubapi/client.go | 1 + pr/githubapi/pull_request.go | 5 ++ pr/prbot/main.go | 8 +++ pr/webhook/issue_comment.go | 50 +++++++++++++++++++ pr/webhook/pull_request.go | 14 +++++- pr/webhook/pull_request_review.go | 49 +++++++++++++++++- pr/webhook/pull_request_test.go | 21 ++++++++ pr/webhook/server.go | 2 + 10 files changed, 308 insertions(+), 2 deletions(-) create mode 100644 pr/cron/maintainer_timeout.go create mode 100644 pr/webhook/issue_comment.go diff --git a/pr/cron/maintainer_timeout.go b/pr/cron/maintainer_timeout.go new file mode 100644 index 0000000..e432b26 --- /dev/null +++ b/pr/cron/maintainer_timeout.go @@ -0,0 +1,78 @@ +package cron + +import ( + "log" + "strconv" + "time" + + "github.com/macports/mpbot-github/pr/db" + "github.com/macports/mpbot-github/pr/githubapi" +) + +type Manager struct { + DB db.DBHelper + Client githubapi.Client +} + +func (manager *Manager) Start() { + manager.MaintainerTimeout() + for { + select { + case <-time.After(12 * time.Hour): + manager.MaintainerTimeout() + } + } +} + +func (manager *Manager) MaintainerTimeout() { + //TODO: properly handle nil pointers + defer func() { + if r := recover(); r != nil { + log.Println(r) + } + }() + + prs, err := manager.DB.GetTimeoutPRs() + if err != nil { + log.Println(err) + return + } +prLoop: + for _, pr := range prs { + log.Println("maintainer timeout of PR #" + strconv.Itoa(pr.Number) + " detected") + prStatus, err := manager.Client.GetPullRequest("macports", "macports-ports", pr.Number) + if err != nil { + log.Println("Failed to get status of PR #" + strconv.Itoa(pr.Number)) + continue + } + if *prStatus.State == "closed" { + log.Println("PR #" + strconv.Itoa(pr.Number) + " closed, clear pending_review") + manager.DB.SetPRPendingReview(pr.Number, false) + continue + } + //TODO: de-hardcode + labels, err := manager.Client.ListLabels("macports", "macports-ports", pr.Number) + if err != nil { + continue + } + isApprovalRequired := false + for _, label := range labels { + if label == "maintainer: requires approval" { + isApprovalRequired = true + } + if label == "maintainer: timeout" { + manager.DB.SetPRPendingReview(pr.Number, false) + continue prLoop + } + } + if !isApprovalRequired { + manager.DB.SetPRPendingReview(pr.Number, false) + } else { + labels = append(labels, "maintainer: timeout") + err = manager.Client.ReplaceLabels("macports", "macports-ports", pr.Number, labels) + if err == nil { + manager.DB.SetPRPendingReview(pr.Number, false) + } + } + } +} diff --git a/pr/db/dbutil.go b/pr/db/dbutil.go index 653d3bd..5c3ea34 100644 --- a/pr/db/dbutil.go +++ b/pr/db/dbutil.go @@ -6,6 +6,7 @@ import ( "log" "os" "strings" + "time" // PostgreSQL driver _ "github.com/lib/pq" @@ -23,9 +24,21 @@ type PortMaintainer struct { OpenMaintainer bool } +type PullRequest struct { + Number int + Processed bool + PendingReview bool + Maintainers []string +} + type DBHelper interface { GetGitHubHandle(email string) (string, error) GetPortMaintainer(port string) (*PortMaintainer, error) + NewPR(number int, maintainers []string) error + GetPR(number int) (*PullRequest, error) + GetTimeoutPRs() ([]*PullRequest, error) + SetPRProcessed(number int, processed bool) error + SetPRPendingReview(number int, pendingReview bool) error } func NewDBHelper() (DBHelper, error) { @@ -56,6 +69,17 @@ func NewDBHelper() (DBHelper, error) { if err != nil { return nil, err } + _, err = prDB.Exec(`CREATE TABLE IF NOT EXISTS pull_requests +( + number INT PRIMARY KEY, + created TIMESTAMP NOT NULL, + processed BOOLEAN NOT NULL, + pending_review BOOLEAN NOT NULL, + maintainers TEXT NOT NULL +);`) + if err != nil { + return nil, err + } return &sqlDBHelper{ tracDB: tracDB, @@ -120,6 +144,7 @@ func (sqlDB *sqlDBHelper) GetPortMaintainer(port string) (*PortMaintainer, error err = rows.Err() if err != nil { + //TODO: log in caller log.Println(err) } @@ -130,6 +155,63 @@ func (sqlDB *sqlDBHelper) GetPortMaintainer(port string) (*PortMaintainer, error return maintainer, nil } +func (sqlDB *sqlDBHelper) NewPR(number int, maintainers []string) error { + _, err := sqlDB.prDB.Exec("INSERT INTO pull_requests VALUES ($1, $2, $3, $4, $5)", + number, time.Now(), false, false, strings.Join(maintainers, " ")) + return err +} + +func (sqlDB *sqlDBHelper) GetPR(number int) (*PullRequest, error) { + pr := new(PullRequest) + var maintainerString string + err := sqlDB.prDB.QueryRow( + "SELECT number, processed, pending_review, maintainers FROM pull_requests WHERE number = $1", number). + Scan(&pr.Number, &pr.Processed, &pr.PendingReview, &maintainerString) + if err != nil { + return nil, err + } + pr.Maintainers = strings.Split(maintainerString, " ") + return pr, nil +} + +func (sqlDB *sqlDBHelper) GetTimeoutPRs() ([]*PullRequest, error) { + var prs []*PullRequest + rows, err := sqlDB.wwwDB.Query("SELECT number, processed, pending_review, maintainers "+ + "FROM pull_requests "+ + "WHERE created <= $1 AND pending_review = true", time.Now().AddDate(0, 0, -3)) + if err != nil { + return nil, err + } + defer rows.Close() + + for rows.Next() { + pr := new(PullRequest) + var maintainerString string + if err := rows.Scan(&pr.Number, &pr.Processed, &pr.PendingReview, &maintainerString); err != nil { + return nil, err + } + pr.Maintainers = strings.Split(maintainerString, " ") + prs = append(prs, pr) + } + + err = rows.Err() + if err != nil { + log.Println(err) + } + + return prs, nil +} + +func (sqlDB *sqlDBHelper) SetPRProcessed(number int, processed bool) error { + _, err := sqlDB.prDB.Exec("UPDATE pull_requests SET processed = $1 WHERE number = $2", processed, number) + return err +} + +func (sqlDB *sqlDBHelper) SetPRPendingReview(number int, pendingReview bool) error { + _, err := sqlDB.prDB.Exec("UPDATE pull_requests SET pending_review = $1 WHERE number = $2", pendingReview, number) + return err +} + func (sqlDB *sqlDBHelper) parseMaintainer(maintainerFullString string) *Maintainer { maintainer := parseMaintainerString(maintainerFullString) if maintainer.GithubHandle == "" && maintainer.Email != "" { diff --git a/pr/githubapi/client.go b/pr/githubapi/client.go index ff31600..72dc255 100644 --- a/pr/githubapi/client.go +++ b/pr/githubapi/client.go @@ -8,6 +8,7 @@ import ( ) type Client interface { + GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) ListChangedPortsAndFiles(owner, repo string, number int) (ports []string, commitFiles []*github.CommitFile, err error) CreateComment(owner, repo string, number int, body *string) error ReplaceLabels(owner, repo string, number int, labels []string) error diff --git a/pr/githubapi/pull_request.go b/pr/githubapi/pull_request.go index 96632ac..90e0c38 100644 --- a/pr/githubapi/pull_request.go +++ b/pr/githubapi/pull_request.go @@ -7,6 +7,11 @@ import ( "github.com/google/go-github/github" ) +func (client *githubClient) GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) { + pr, _, err := client.PullRequests.Get(context.Background(), owner, repo, number) + return pr, err +} + func (client *githubClient) ListChangedPortsAndFiles(owner, repo string, number int) (ports []string, commitFiles []*github.CommitFile, err error) { var allFiles []*github.CommitFile opt := &github.ListOptions{PerPage: 30} diff --git a/pr/prbot/main.go b/pr/prbot/main.go index 0d3a875..c5bbc53 100644 --- a/pr/prbot/main.go +++ b/pr/prbot/main.go @@ -5,7 +5,9 @@ import ( "log" "os" + "github.com/macports/mpbot-github/pr/cron" "github.com/macports/mpbot-github/pr/db" + "github.com/macports/mpbot-github/pr/githubapi" "github.com/macports/mpbot-github/pr/webhook" ) @@ -32,5 +34,11 @@ func main() { log.Fatal(err) } + cronManager := cron.Manager{ + DB: dbHelper, + Client: githubapi.NewClient(botSecret), + } + go cronManager.Start() + webhook.NewReceiver(*webhookAddr, hookSecret, botSecret, prodFlag, dbHelper).Start() } diff --git a/pr/webhook/issue_comment.go b/pr/webhook/issue_comment.go new file mode 100644 index 0000000..5165079 --- /dev/null +++ b/pr/webhook/issue_comment.go @@ -0,0 +1,50 @@ +package webhook + +import ( + "encoding/json" + "log" + "strconv" + + "github.com/google/go-github/github" +) + +func (receiver *Receiver) handleIssueComment(body []byte) { + defer func() { + if r := recover(); r != nil { + log.Println(r) + } + }() + + event := &github.IssueCommentEvent{} + err := json.Unmarshal(body, event) + if err != nil { + log.Println(err) + return + } + + number := *event.Issue.Number + sender := *event.Sender.Login + + // TODO: refactor, share with PullRequestReview + pr, err := receiver.dbHelper.GetPR(number) + if err != nil { + log.Println(err) + return + } + if !pr.Processed { + return + } + if !pr.PendingReview { + return + } + isOneMaintainer := false + for _, maintainer := range pr.Maintainers { + if maintainer == sender { + isOneMaintainer = true + } + } + if isOneMaintainer { + log.Println("Maintainer responded in PR #" + strconv.Itoa(pr.Number)) + receiver.dbHelper.SetPRPendingReview(number, false) + } +} diff --git a/pr/webhook/pull_request.go b/pr/webhook/pull_request.go index 188b328..3dad94b 100644 --- a/pr/webhook/pull_request.go +++ b/pr/webhook/pull_request.go @@ -22,7 +22,6 @@ func (receiver *Receiver) handlePullRequest(body []byte) { event := &github.PullRequestEvent{} err := json.Unmarshal(body, event) if err != nil { - // TODO: log log.Println(err) return } @@ -87,8 +86,18 @@ func (receiver *Receiver) handlePullRequest(body []byte) { } isMaintainer = isOneMaintainer && isMaintainer + maintainers := make([]string, len(handles)) + { + i := 0 + for k := range handles { + maintainers[i] = k + i++ + } + } + switch *event.Action { case "opened": + receiver.dbHelper.NewPR(number, maintainers) // Notify maintainers mentionSymbol := "@_" if receiver.production { @@ -125,6 +134,7 @@ func (receiver *Receiver) handlePullRequest(body []byte) { } else if !isSubmission && !isMaintainer { // TODO: store in DB maintainerLabels = append(maintainerLabels, "maintainer: requires approval") + receiver.dbHelper.SetPRPendingReview(number, true) } // Collect existing labels (PR sender could add labels when creating a PR) @@ -176,6 +186,8 @@ func (receiver *Receiver) handlePullRequest(body []byte) { if err != nil { log.Println(err) } + + receiver.dbHelper.SetPRProcessed(number, true) // fallthrough //case "synchronize": } diff --git a/pr/webhook/pull_request_review.go b/pr/webhook/pull_request_review.go index d6986d7..0597f5d 100644 --- a/pr/webhook/pull_request_review.go +++ b/pr/webhook/pull_request_review.go @@ -1,3 +1,50 @@ package webhook -func (receiver *Receiver) handlePullRequestReview(body []byte) {} +import ( + "encoding/json" + "log" + "strconv" + + "github.com/google/go-github/github" +) + +func (receiver *Receiver) handlePullRequestReview(body []byte) { + defer func() { + if r := recover(); r != nil { + log.Println(r) + } + }() + + event := &github.PullRequestReviewEvent{} + err := json.Unmarshal(body, event) + if err != nil { + log.Println(err) + return + } + + number := *event.PullRequest.Number + sender := *event.Sender.Login + + // TODO: refactor, share with IssueComment + pr, err := receiver.dbHelper.GetPR(number) + if err != nil { + log.Println(err) + return + } + if !pr.Processed { + return + } + if !pr.PendingReview { + return + } + isOneMaintainer := false + for _, maintainer := range pr.Maintainers { + if maintainer == sender { + isOneMaintainer = true + } + } + if isOneMaintainer { + log.Println("Maintainer responded in PR #" + strconv.Itoa(pr.Number)) + receiver.dbHelper.SetPRPendingReview(number, false) + } +} diff --git a/pr/webhook/pull_request_test.go b/pr/webhook/pull_request_test.go index 00e0fff..2bba745 100644 --- a/pr/webhook/pull_request_test.go +++ b/pr/webhook/pull_request_test.go @@ -92,6 +92,10 @@ type stubGitHubClient struct { newLabels []string } +func (stub *stubGitHubClient) GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) { + return nil, errNotFound +} + func (stub *stubGitHubClient) ListChangedPortsAndFiles(owner, repo string, number int) (ports []string, commitFiles []*github.CommitFile, err error) { if owner != "macports" || repo != "macports-ports" { return nil, nil, errNotFound @@ -182,6 +186,23 @@ func (stub *stubDBHelper) GetPortMaintainer(port string) (*db.PortMaintainer, er return nil, errors.New("port not found") } +func (stub *stubDBHelper) NewPR(number int, maintainers []string) error { + return nil +} +func (stub *stubDBHelper) GetPR(number int) (*db.PullRequest, error) { + return nil, nil +} +func (stub *stubDBHelper) GetTimeoutPRs() ([]*db.PullRequest, error) { + return nil, nil +} +func (stub *stubDBHelper) SetPRProcessed(number int, processed bool) error { + return nil +} + +func (stub *stubDBHelper) SetPRPendingReview(number int, pendingReview bool) error { + return nil +} + func ptrOfStr(s string) *string { return &s } diff --git a/pr/webhook/server.go b/pr/webhook/server.go index 1e637e8..f2c5911 100644 --- a/pr/webhook/server.go +++ b/pr/webhook/server.go @@ -70,6 +70,8 @@ func (receiver *Receiver) Start() { go receiver.handlePullRequest(body) case "pull_request_review": go receiver.handlePullRequestReview(body) + case "issue_comment": + go receiver.handleIssueComment(body) } w.WriteHeader(http.StatusNoContent)