From 9a465b2bc1b6823910ce2d4863d60b4729086e4c Mon Sep 17 00:00:00 2001 From: Jon Bodner Date: Mon, 16 May 2016 13:11:11 -0400 Subject: [PATCH 1/2] Put the names, emails, and logins of the approvers into the pull request comment --- remote/github/github.go | 35 ++++++++++++++++++++++++++--------- remote/mock/remote.go | 2 +- remote/remote.go | 6 +++--- web/comment_hook.go | 31 ++++++++++++++++++++----------- web/status_hook.go | 19 ++++++++++++------- 5 files changed, 62 insertions(+), 31 deletions(-) diff --git a/remote/github/github.go b/remote/github/github.go index b317021..56d8c88 100644 --- a/remote/github/github.go +++ b/remote/github/github.go @@ -414,7 +414,7 @@ func (g *Github) GetPRHook(r *http.Request) (*model.PRHook, error) { func (g *Github) GetPullRequestsForCommit(u *model.User, r *model.Repo, sha *string) ([]model.PullRequest, error) { client := setupClient(g.API, u.Token) log.Debug("sha == ", sha, *sha) - issues, _, err := client.Search.Issues(fmt.Sprintf("%s&type=pr", *sha), &github.SearchOptions { + issues, _, err := client.Search.Issues(fmt.Sprintf("%s&type=pr", *sha), &github.SearchOptions{ TextMatch: false, }) if err != nil { @@ -427,7 +427,7 @@ func (g *Github) GetPullRequestsForCommit(u *model.User, r *model.Repo, sha *str return nil, err } - mergeable:= true + mergeable := true if pr.Mergeable != nil { mergeable = *pr.Mergeable } @@ -439,15 +439,15 @@ func (g *Github) GetPullRequestsForCommit(u *model.User, r *model.Repo, sha *str log.Debug("current issue ==", v) log.Debug("current pr ==", *pr) - log.Debug("combined status ==",*status) + log.Debug("combined status ==", *status) combinedState := *status.State if combinedState == "success" { log.Debug("overall status is success -- checking to see if all status checks returned success") for _, v := range status.Statuses { - log.Debugf("status check %s returned %s",*v.Context, *v.State) + log.Debugf("status check %s returned %s", *v.Context, *v.State) if *v.State != "success" { - log.Debugf("setting combined status check to %s",*v.State) + log.Debugf("setting combined status check to %s", *v.State) combinedState = *v.State } } @@ -458,7 +458,7 @@ func (g *Github) GetPullRequestsForCommit(u *model.User, r *model.Repo, sha *str Title: *v.Title, Author: *v.User.Login, }, - Branch: model.Branch { + Branch: model.Branch{ Name: *pr.Head.Ref, BranchStatus: combinedState, Mergeable: mergeable, @@ -479,10 +479,27 @@ func (g *Github) GetBranchStatus(u *model.User, r *model.Repo, branch string) (* return (*model.BranchStatus)(statuses.State), nil } -func (g *Github) MergePR(u *model.User, r *model.Repo, pullRequest model.PullRequest) (*string, error) { +func (g *Github) MergePR(u *model.User, r *model.Repo, pullRequest model.PullRequest, approvers []*model.Person) (*string, error) { client := setupClient(g.API, u.Token) - result, _, err := client.PullRequests.Merge(r.Owner, r.Name, pullRequest.Number, "Merged by LGTM") + msg := "Merged by LGTM\n" + if len(approvers) > 0 { + apps := "Approved by:\n" + for _, v := range approvers { + //Brad Rydzewski (@bradrydzewski) + if len(v.Name) > 0 { + apps += fmt.Sprintf("%s", v.Name) + } + if len(v.Email) > 0 { + apps += fmt.Sprintf(" <%s>", v.Email) + } + if len(v.Login) > 0 { + apps += fmt.Sprintf(" (@%s)", v.Login) + } + apps += "\n" + } + } + result, _, err := client.PullRequests.Merge(r.Owner, r.Name, pullRequest.Number, msg) if err != nil { return nil, err } @@ -500,7 +517,7 @@ func (g *Github) ListTags(u *model.User, r *model.Repo) ([]model.Tag, error) { if err != nil { return nil, err } - out := make([]model.Tag,len(tags)) + out := make([]model.Tag, len(tags)) for k, v := range tags { out[k] = model.Tag(*v.Name) } diff --git a/remote/mock/remote.go b/remote/mock/remote.go index 35887b5..33f92f4 100644 --- a/remote/mock/remote.go +++ b/remote/mock/remote.go @@ -348,7 +348,7 @@ func (_m *Remote) GetUserToken(_a0 string) (string, error) { } // MergePR provides a mock function with given fields: u, r, pullRequest -func (_m *Remote) MergePR(u *model.User, r *model.Repo, pullRequest model.PullRequest) (*string, error) { +func (_m *Remote) MergePR(u *model.User, r *model.Repo, pullRequest model.PullRequest, approvers []*model.Person) (*string, error) { ret := _m.Called(u, r, pullRequest) var r0 *string diff --git a/remote/remote.go b/remote/remote.go index 56abbd6..9f95d8d 100644 --- a/remote/remote.go +++ b/remote/remote.go @@ -60,7 +60,7 @@ type Remote interface { GetBranchStatus(*model.User, *model.Repo, string) (*model.BranchStatus, error) // MergePR merges the named pull request from the remote system - MergePR(u *model.User, r *model.Repo, pullRequest model.PullRequest) (*string, error) + MergePR(u *model.User, r *model.Repo, pullRequest model.PullRequest, approvers []*model.Person) (*string, error) // GetMaxExistingTag finds the highest version across all tags ListTags(u *model.User, r *model.Repo) ([]model.Tag, error) @@ -153,8 +153,8 @@ func GetBranchStatus(c context.Context, u *model.User, r *model.Repo, branch str return FromContext(c).GetBranchStatus(u, r, branch) } -func MergePR(c context.Context, u *model.User, r *model.Repo, pullRequest model.PullRequest) (*string, error) { - return FromContext(c).MergePR(u, r, pullRequest) +func MergePR(c context.Context, u *model.User, r *model.Repo, pullRequest model.PullRequest, approvers []*model.Person) (*string, error) { + return FromContext(c).MergePR(u, r, pullRequest, approvers) } func ListTags(c context.Context, u *model.User, r *model.Repo) ([]model.Tag, error) { diff --git a/web/comment_hook.go b/web/comment_hook.go index 075c68a..f10960e 100644 --- a/web/comment_hook.go +++ b/web/comment_hook.go @@ -19,19 +19,11 @@ func processCommentHook(c *gin.Context, hook *model.Hook) { return } - comments, err := getComments(c, user, repo, hook.Issue.Number) + approvers, err := buildApprovers(c, user, repo, config, maintainer, hook.Issue) if err != nil { return } - alg, err := approval.Lookup(config.ApprovalAlg) - if err != nil { - log.Errorf("Error getting approval algorithm %s. %s", config.ApprovalAlg, err) - c.String(500, "Error getting approval algorithm %s. %s", config.ApprovalAlg, err) - return - } - approvers := getApprovers(config, maintainer, hook.Issue, comments, alg) - approved := len(approvers) >= config.Approvals err = remote.SetStatus(c, user, repo, hook.Issue.Number, approved) if err != nil { @@ -50,8 +42,25 @@ func processCommentHook(c *gin.Context, hook *model.Hook) { }) } -func getApprovers(config *model.Config, maintainer *model.Maintainer, - issue *model.Issue, comments []*model.Comment, matcher approval.Func) []*model.Person { +func buildApprovers(c *gin.Context, user *model.User, repo *model.Repo, config *model.Config, maintainer *model.Maintainer, issue *model.Issue) ([]*model.Person, error) { + comments, err := getComments(c, user, repo, issue.Number) + if err != nil { + log.Errorf("Error getting comments for %s/%s/%d", repo.Owner, repo.Name, issue.Number) + c.String(500, "Error getting comments for %s/%s/%d", repo.Owner, repo.Name, issue.Number) + return nil, err + } + + alg, err := approval.Lookup(config.ApprovalAlg) + if err != nil { + log.Errorf("Error getting approval algorithm %s. %s", config.ApprovalAlg, err) + c.String(500, "Error getting approval algorithm %s. %s", config.ApprovalAlg, err) + return nil, err + } + approvers := getApprovers(config, maintainer, issue, comments, alg) + return approvers, nil +} + +func getApprovers(config *model.Config, maintainer *model.Maintainer, issue *model.Issue, comments []*model.Comment, matcher approval.Func) []*model.Person { approvers := []*model.Person{} matcher(config, maintainer, issue, comments, func(maintainer *model.Maintainer, comment *model.Comment) { approvers = append(approvers, maintainer.People[comment.Author]) diff --git a/web/status_hook.go b/web/status_hook.go index f160f30..6283ba8 100644 --- a/web/status_hook.go +++ b/web/status_hook.go @@ -35,7 +35,7 @@ func processStatusHook(c *gin.Context, hook *model.StatusHook) { merged := map[string]StatusResponse{} - log.Debug("calling getPullRequestsForCommit for sha",hook.SHA) + log.Debug("calling getPullRequestsForCommit for sha", hook.SHA) pullRequests, err := remote.GetPullRequestsForCommit(c, user, hook.Repo, &hook.SHA) log.Debugf("sha for commit is %s, pull requests are: %s", hook.SHA, pullRequests) @@ -48,7 +48,12 @@ func processStatusHook(c *gin.Context, hook *model.StatusHook) { for _, v := range pullRequests { //if all of the statuses are success, then merge and create a tag for the version if v.Branch.BranchStatus == "success" && v.Branch.Mergeable { - sha, err := remote.MergePR(c, user, hook.Repo, v) + approvers, err := buildApprovers(c, user, repo, config, maintainer, &v.Issue) + if err != nil { + log.Warnf("Unable to merge pull request %s: %s", v.Title, err) + continue + } + sha, err := remote.MergePR(c, user, hook.Repo, v, approvers) if err != nil { log.Warnf("Unable to merge pull request %s: %s", v.Title, err) continue @@ -77,7 +82,7 @@ func processStatusHook(c *gin.Context, hook *model.StatusHook) { } if err != nil { - log.Warnf("Unable to generate a version tag: %s",err.Error()) + log.Warnf("Unable to generate a version tag: %s", err.Error()) continue } log.Debugf("Tagging merge from PR with tag: %s", *verStr) @@ -97,7 +102,7 @@ func processStatusHook(c *gin.Context, hook *model.StatusHook) { }) } -const modifiedRFC3339= "2006-01-02T15.04.05Z" +const modifiedRFC3339 = "2006-01-02T15.04.05Z" func handleTimestamp(config *model.Config) (*string, error) { /* @@ -116,7 +121,7 @@ func handleTimestamp(config *model.Config) (*string, error) { case "rfc3339", "": format = modifiedRFC3339 default: - log.Warnf("invalid version format %s. Using modified rfc3339",config.VersionFormat) + log.Warnf("invalid version format %s. Using modified rfc3339", config.VersionFormat) format = modifiedRFC3339 } out := curTime.Format(format) @@ -149,7 +154,7 @@ func handleSemver(c *gin.Context, user *model.User, hook *model.StatusHook, pr m maxVer = foundVersion } else { maxParts := maxVer.Segments() - maxVer, _ = version.NewVersion(fmt.Sprintf("%d.%d.%d", maxParts[0], maxParts[1], maxParts[2]+1)) + maxVer, _ = version.NewVersion(fmt.Sprintf("%d.%d.%d", maxParts[0], maxParts[1], maxParts[2] + 1)) } verStr := maxVer.String() @@ -182,7 +187,7 @@ func getMaxExistingTag(tags []model.Tag) *version.Version { // the function returns version 0.0.0. If there's a bug in the version pattern, // nil will be returned. func getMaxVersionComment(config *model.Config, maintainer *model.Maintainer, - issue model.Issue, comments []*model.Comment, matcher approval.Func) *version.Version { +issue model.Issue, comments []*model.Comment, matcher approval.Func) *version.Version { maxVersion, _ := version.NewVersion("0.0.0") ma, err := regexp.Compile(config.Pattern) if err != nil { From 69ae547d721e8784fb20bb5ceb996c46e0445781 Mon Sep 17 00:00:00 2001 From: Jon Bodner Date: Mon, 16 May 2016 13:15:24 -0400 Subject: [PATCH 2/2] Forgot to append the approvers onto the commit message --- remote/github/github.go | 1 + 1 file changed, 1 insertion(+) diff --git a/remote/github/github.go b/remote/github/github.go index 56d8c88..4b990bb 100644 --- a/remote/github/github.go +++ b/remote/github/github.go @@ -498,6 +498,7 @@ func (g *Github) MergePR(u *model.User, r *model.Repo, pullRequest model.PullReq } apps += "\n" } + msg += apps } result, _, err := client.PullRequests.Merge(r.Owner, r.Name, pullRequest.Number, msg) if err != nil {