From 0116c3f4c4f7729052ea9d094d0dbdac1a5b8932 Mon Sep 17 00:00:00 2001 From: "Alex Ellis (OpenFaaS Ltd)" Date: Thu, 16 Jan 2020 20:51:30 +0000 Subject: [PATCH 1/6] Add merge/rebase feature Signed-off-by: Alex Ellis (OpenFaaS Ltd) --- handler/comment_handler.go | 8 ++++ handler/merge.go | 92 ++++++++++++++++++++++++++++++++++++++ types/types.go | 6 +++ 3 files changed, 106 insertions(+) create mode 100644 handler/merge.go diff --git a/handler/comment_handler.go b/handler/comment_handler.go index 9f78b61..a810d63 100644 --- a/handler/comment_handler.go +++ b/handler/comment_handler.go @@ -36,6 +36,7 @@ const ( assignReviewerConstant string = "AssignReviewer" unassignReviewerConstant string = "UnassignReviewer" messageConstant string = "message" + mergePRConstant string = "Merge" noDCO string = "no-dco" labelLimitDefault int = 5 @@ -120,6 +121,12 @@ func HandleComment(req types.IssueCommentOuter, config config.Config, derekConfi feedback, err = createMessage(req, command.Type, command.Value, config, derekConfig) break + case mergePRConstant: + merger := merge{} + feedback, err = merger.Merge(req, command.Type, command.Value) + + break + default: feedback = "Unable to work with comment: " + req.Comment.Body err = nil @@ -434,6 +441,7 @@ func parse(body string, commandTriggers []string) *types.CommentAction { commandTrigger + "clear reviewer: ": unassignReviewerConstant, commandTrigger + "message: ": messageConstant, commandTrigger + "msg: ": messageConstant, + commandTrigger + "merge": mergePRConstant, } for trigger, commandType := range commands { diff --git a/handler/merge.go b/handler/merge.go new file mode 100644 index 0000000..e69073a --- /dev/null +++ b/handler/merge.go @@ -0,0 +1,92 @@ +package handler + +import ( + "context" + "fmt" + + "github.com/alexellis/derek/config" + "github.com/alexellis/derek/types" + "github.com/google/go-github/github" +) + +type merge struct { + Config *config.Config +} + +func (m *merge) Merge(req types.IssueCommentOuter, cmdType string, cmdValue string) (string, error) { + result := "" + + client, ctx := makeClient(req.Installation.ID, *m.Config) + + if req.Issue.PullRequest == nil { + return "can't merge a non-PR issue", nil + } + + pr, _, err := client.PullRequests.Get(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number) + if err != nil { + return "unable to get pull request", err + } + + if pr.GetMerged() == false { + + if pr.GetMergeable() == true { + + if validMergePolicy(req) == false { + sendComment(client, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number, + "I am unable to merge this PR due to merge-policy exception(s)") + + return "invalid merge policy", nil + } + + pullRequestOptions := github.PullRequestOptions{ + MergeMethod: "rebase", + CommitTitle: fmt.Sprintf("Merge PR #%d", req.Issue.Number), + } + mergeRes, _, err := client.PullRequests.Merge(ctx, + req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number, + fmt.Sprintf(`Merging PR #%d by Derek +This is an automated merge by the bot Derek, find more +https://github.com/alexellis/derek/ +Signed-off-by: derek@openfaas.com`, req.Issue.Number), &pullRequestOptions) + + if err != nil { + + body := fmt.Sprintf(`I have been unable to merge the requested PR: %s`, err.Error()) + + sendComment(client, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number, + body) + + return fmt.Sprintf("Merge issue: %s, %t", mergeRes.GetMessage(), mergeRes.GetMerged()), err + } + + sendComment(client, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number, + `I have merged the pull request using the rebase strategy.`) + } else { + sendComment(client, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number, + "This pull request cannot be merged. Rebase your work and try again.") + } + } + + return result, err +} + +func sendComment(client *github.Client, login string, repo string, issue int, comment string) { + + issueComment := &github.IssueComment{ + Body: &comment, + } + client.Issues.CreateComment(context.Background(), + login, repo, issue, issueComment) +} + +func validMergePolicy(req types.IssueCommentOuter) bool { + validDCO := true + for _, label := range req.Issue.Labels { + if label.Name == "no-dco" { + validDCO = false + break + } + } + + return validDCO +} diff --git a/types/types.go b/types/types.go index 65a740a..88336a0 100644 --- a/types/types.go +++ b/types/types.go @@ -56,6 +56,12 @@ type Issue struct { State string `json:"state"` Milestone Milestone `json:"milestone"` URL string `json:"url"` + + PullRequest *PullRequestIssueLink `json:"pull_request"` +} + +type PullRequestIssueLink struct { + URL string `json:"url"` } type Milestone struct { From a15ced82b7af31e0574efb663fd5560fccc6c6c2 Mon Sep 17 00:00:00 2001 From: "Alex Ellis (OpenFaaS Ltd)" Date: Thu, 16 Jan 2020 21:21:05 +0000 Subject: [PATCH 2/6] Add must approve users Signed-off-by: Alex Ellis (OpenFaaS Ltd) --- handler/comment_handler.go | 6 +++- handler/merge.go | 64 +++++++++++++++++++++++++++++++++++--- types/types.go | 14 ++++++--- 3 files changed, 75 insertions(+), 9 deletions(-) diff --git a/handler/comment_handler.go b/handler/comment_handler.go index a810d63..102295c 100644 --- a/handler/comment_handler.go +++ b/handler/comment_handler.go @@ -122,7 +122,11 @@ func HandleComment(req types.IssueCommentOuter, config config.Config, derekConfi break case mergePRConstant: - merger := merge{} + merger := merge{ + Config: config, + RepoConfig: derekConfig, + } + feedback, err = merger.Merge(req, command.Type, command.Value) break diff --git a/handler/merge.go b/handler/merge.go index e69073a..fc4b25f 100644 --- a/handler/merge.go +++ b/handler/merge.go @@ -3,6 +3,7 @@ package handler import ( "context" "fmt" + "log" "github.com/alexellis/derek/config" "github.com/alexellis/derek/types" @@ -10,18 +11,27 @@ import ( ) type merge struct { - Config *config.Config + Config config.Config + RepoConfig *types.DerekRepoConfig } func (m *merge) Merge(req types.IssueCommentOuter, cmdType string, cmdValue string) (string, error) { result := "" - client, ctx := makeClient(req.Installation.ID, *m.Config) + client, ctx := makeClient(req.Installation.ID, m.Config) if req.Issue.PullRequest == nil { return "can't merge a non-PR issue", nil } + if len(m.RepoConfig.Mergers) == 0 { + return "can't merge without at least one merger", nil + } + + if mayMerge(req.Comment.User.Login, m.RepoConfig.Mergers) == false { + return fmt.Sprintf("user %s, may not merge", req.Comment.User.Login), nil + } + pr, _, err := client.PullRequests.Get(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number) if err != nil { return "unable to get pull request", err @@ -38,16 +48,46 @@ func (m *merge) Merge(req types.IssueCommentOuter, cmdType string, cmdValue stri return "invalid merge policy", nil } + listOpts := &github.ListOptions{} + + reviews, _, listReviewsErr := client.PullRequests.ListReviews(ctx, req.Repository.Owner.Login, + req.Repository.Name, req.Issue.Number, listOpts) + + if listReviewsErr != nil { + return fmt.Sprintf("unable to list reviews for %d", pr.GetID()), listReviewsErr + } + + mustApproveConfirmed := []github.PullRequestReview{} + for _, r := range reviews { + for _, approver := range m.RepoConfig.MustApprove { + if r.GetState() == "APPROVED" && + r.GetUser().GetLogin() == approver && + r.GetCommitID() == pr.GetHead().GetSHA() { + mustApproveConfirmed = append(mustApproveConfirmed, *r) + } + } + } + + if len(m.RepoConfig.MustApprove) > 0 { + if len(m.RepoConfig.MustApprove) != len(mustApproveConfirmed) { + return fmt.Sprintf("needed %d approvals, but had: %d", + len(m.RepoConfig.MustApprove), len(mustApproveConfirmed)), nil + } + } + pullRequestOptions := github.PullRequestOptions{ MergeMethod: "rebase", CommitTitle: fmt.Sprintf("Merge PR #%d", req.Issue.Number), } + mergeRes, _, err := client.PullRequests.Merge(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number, fmt.Sprintf(`Merging PR #%d by Derek + This is an automated merge by the bot Derek, find more https://github.com/alexellis/derek/ -Signed-off-by: derek@openfaas.com`, req.Issue.Number), &pullRequestOptions) + +Signed-off-by: Derek `, req.Issue.Number), &pullRequestOptions) if err != nil { @@ -75,8 +115,12 @@ func sendComment(client *github.Client, login string, repo string, issue int, co issueComment := &github.IssueComment{ Body: &comment, } - client.Issues.CreateComment(context.Background(), + + _, _, err := client.Issues.CreateComment(context.Background(), login, repo, issue, issueComment) + if err != nil { + log.Printf("Error creating comment %s %s %d\n", login, repo, issue) + } } func validMergePolicy(req types.IssueCommentOuter) bool { @@ -90,3 +134,15 @@ func validMergePolicy(req types.IssueCommentOuter) bool { return validDCO } + +func mayMerge(user string, list []string) bool { + may := false + + for _, item := range list { + if item == user { + may = true + break + } + } + return may +} diff --git a/types/types.go b/types/types.go index 88336a0..9cda2ca 100644 --- a/types/types.go +++ b/types/types.go @@ -85,21 +85,27 @@ type CommentAction struct { type DerekRepoConfig struct { // A redirect URL to load the config from another location. - Redirect string + Redirect string `yaml:"redirect"` // Features can be turned on/off if needed. - Features []string + Features []string `yaml:"features"` // Users who are enrolled to make use of Derek - Maintainers []string + Maintainers []string `yaml:"maintainers"` // Curators is an alias for Maintainers and is only used if the Maintainers list is empty. - Curators []string + Curators []string `yaml:"curators"` //ContributingURL url to contribution guide ContributingURL string `yaml:"contributing_url"` Messages []Message `yaml:"custom_messages"` + + // Mergers are those who can perform a rebase + Mergers []string `yaml:"mergers"` + + // MustApprove are those from whom an approval must be in place for a merge + MustApprove []string `yaml:"must_approve"` } type Message struct { From 138b49f9403be7c3349724cb44a1234ac608e2f9 Mon Sep 17 00:00:00 2001 From: "Alex Ellis (OpenFaaS Ltd)" Date: Thu, 16 Jan 2020 21:50:14 +0000 Subject: [PATCH 3/6] Don't review if only approver is also owner Signed-off-by: Alex Ellis (OpenFaaS Ltd) --- handler/comment_handler.go | 9 +++++++++ handler/merge.go | 33 ++++++++++++++++++--------------- kumbaya | 1 + 3 files changed, 28 insertions(+), 15 deletions(-) create mode 160000 kumbaya diff --git a/handler/comment_handler.go b/handler/comment_handler.go index 102295c..69c9a46 100644 --- a/handler/comment_handler.go +++ b/handler/comment_handler.go @@ -129,6 +129,14 @@ func HandleComment(req types.IssueCommentOuter, config config.Config, derekConfi feedback, err = merger.Merge(req, command.Type, command.Value) + if len(feedback) > 0 { + log.Printf("Feedback: %s", feedback) + } + + if err != nil { + fmt.Println(err) + } + break default: @@ -446,6 +454,7 @@ func parse(body string, commandTriggers []string) *types.CommentAction { commandTrigger + "message: ": messageConstant, commandTrigger + "msg: ": messageConstant, commandTrigger + "merge": mergePRConstant, + commandTrigger + "rebase": mergePRConstant, } for trigger, commandType := range commands { diff --git a/handler/merge.go b/handler/merge.go index fc4b25f..ba69b82 100644 --- a/handler/merge.go +++ b/handler/merge.go @@ -48,27 +48,30 @@ func (m *merge) Merge(req types.IssueCommentOuter, cmdType string, cmdValue stri return "invalid merge policy", nil } - listOpts := &github.ListOptions{} + if len(m.RepoConfig.MustApprove) == 1 && + pr.GetUser().GetLogin() == m.RepoConfig.MustApprove[0] { + fmt.Printf("OK to merge own PR.\n") + } else if len(m.RepoConfig.MustApprove) > 0 { - reviews, _, listReviewsErr := client.PullRequests.ListReviews(ctx, req.Repository.Owner.Login, - req.Repository.Name, req.Issue.Number, listOpts) + listOpts := &github.ListOptions{} + reviews, _, listReviewsErr := client.PullRequests.ListReviews(ctx, req.Repository.Owner.Login, + req.Repository.Name, req.Issue.Number, listOpts) - if listReviewsErr != nil { - return fmt.Sprintf("unable to list reviews for %d", pr.GetID()), listReviewsErr - } + if listReviewsErr != nil { + return fmt.Sprintf("unable to list reviews for %d", pr.GetID()), listReviewsErr + } - mustApproveConfirmed := []github.PullRequestReview{} - for _, r := range reviews { - for _, approver := range m.RepoConfig.MustApprove { - if r.GetState() == "APPROVED" && - r.GetUser().GetLogin() == approver && - r.GetCommitID() == pr.GetHead().GetSHA() { - mustApproveConfirmed = append(mustApproveConfirmed, *r) + mustApproveConfirmed := []github.PullRequestReview{} + for _, r := range reviews { + for _, approver := range m.RepoConfig.MustApprove { + if r.GetState() == "APPROVED" && + r.GetUser().GetLogin() == approver && + r.GetCommitID() == pr.GetHead().GetSHA() { + mustApproveConfirmed = append(mustApproveConfirmed, *r) + } } } - } - if len(m.RepoConfig.MustApprove) > 0 { if len(m.RepoConfig.MustApprove) != len(mustApproveConfirmed) { return fmt.Sprintf("needed %d approvals, but had: %d", len(m.RepoConfig.MustApprove), len(mustApproveConfirmed)), nil diff --git a/kumbaya b/kumbaya new file mode 160000 index 0000000..a4d0fb8 --- /dev/null +++ b/kumbaya @@ -0,0 +1 @@ +Subproject commit a4d0fb88dac0f30295a36a3885caed45c5b27fff From 2404c53cc174c866b9b4d8c7c4620f9edcc3d5a1 Mon Sep 17 00:00:00 2001 From: "Alex Ellis (OpenFaaS Ltd)" Date: Thu, 16 Jan 2020 22:02:56 +0000 Subject: [PATCH 4/6] Filter out an approver if he is the PR owner Signed-off-by: Alex Ellis (OpenFaaS Ltd) --- handler/merge.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/handler/merge.go b/handler/merge.go index ba69b82..0ef3cf1 100644 --- a/handler/merge.go +++ b/handler/merge.go @@ -48,10 +48,17 @@ func (m *merge) Merge(req types.IssueCommentOuter, cmdType string, cmdValue stri return "invalid merge policy", nil } - if len(m.RepoConfig.MustApprove) == 1 && - pr.GetUser().GetLogin() == m.RepoConfig.MustApprove[0] { - fmt.Printf("OK to merge own PR.\n") - } else if len(m.RepoConfig.MustApprove) > 0 { + mustApprove := []string{} + + // An approver, can't approve their own PRs so must come out + // of the list + for _, approver := range m.RepoConfig.MustApprove { + if approver != pr.GetUser().GetLogin() { + mustApprove = append(mustApprove, approver) + } + } + + if len(mustApprove) > 0 { listOpts := &github.ListOptions{} reviews, _, listReviewsErr := client.PullRequests.ListReviews(ctx, req.Repository.Owner.Login, @@ -63,7 +70,7 @@ func (m *merge) Merge(req types.IssueCommentOuter, cmdType string, cmdValue stri mustApproveConfirmed := []github.PullRequestReview{} for _, r := range reviews { - for _, approver := range m.RepoConfig.MustApprove { + for _, approver := range mustApprove { if r.GetState() == "APPROVED" && r.GetUser().GetLogin() == approver && r.GetCommitID() == pr.GetHead().GetSHA() { From e291cda0dfb8cdc8219907fd3c1167e20167aa4f Mon Sep 17 00:00:00 2001 From: "Alex Ellis (OpenFaaS Ltd)" Date: Thu, 16 Jan 2020 22:14:08 +0000 Subject: [PATCH 5/6] Printout SHA for review Signed-off-by: Alex Ellis (OpenFaaS Ltd) --- handler/comment_handler.go | 2 +- handler/merge.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/handler/comment_handler.go b/handler/comment_handler.go index 69c9a46..f90535d 100644 --- a/handler/comment_handler.go +++ b/handler/comment_handler.go @@ -130,7 +130,7 @@ func HandleComment(req types.IssueCommentOuter, config config.Config, derekConfi feedback, err = merger.Merge(req, command.Type, command.Value) if len(feedback) > 0 { - log.Printf("Feedback: %s", feedback) + log.Printf("Feedback: %s\n", feedback) } if err != nil { diff --git a/handler/merge.go b/handler/merge.go index 0ef3cf1..b6605e4 100644 --- a/handler/merge.go +++ b/handler/merge.go @@ -70,6 +70,7 @@ func (m *merge) Merge(req types.IssueCommentOuter, cmdType string, cmdValue stri mustApproveConfirmed := []github.PullRequestReview{} for _, r := range reviews { + fmt.Printf("Review state: %s, commitID: %s, HEADSHA: %s\n", r.GetState(), r.GetCommitID(), pr.GetHead().GetSHA()) for _, approver := range mustApprove { if r.GetState() == "APPROVED" && r.GetUser().GetLogin() == approver && From 37fd5930bf79deff17004fc0ba4c2d88851a4897 Mon Sep 17 00:00:00 2001 From: "Alex Ellis (OpenFaaS Ltd)" Date: Thu, 16 Jan 2020 22:25:35 +0000 Subject: [PATCH 6/6] Print logs Signed-off-by: Alex Ellis (OpenFaaS Ltd) --- handler/comment_handler.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/handler/comment_handler.go b/handler/comment_handler.go index f90535d..14c951d 100644 --- a/handler/comment_handler.go +++ b/handler/comment_handler.go @@ -134,7 +134,7 @@ func HandleComment(req types.IssueCommentOuter, config config.Config, derekConfi } if err != nil { - fmt.Println(err) + log.Println(err) } break @@ -145,10 +145,10 @@ func HandleComment(req types.IssueCommentOuter, config config.Config, derekConfi break } - fmt.Print(feedback) + log.Print(feedback) if err != nil { - fmt.Println(err) + log.Println(err) } }