From eedb219cdd937b9f5d08276e761ce87c00268116 Mon Sep 17 00:00:00 2001 From: su7edja Date: Mon, 14 Oct 2019 11:08:26 -0700 Subject: [PATCH 1/4] Added function to addReview to PR with approve/request_changes/comment event types --- ghutil/ghutil.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/ghutil/ghutil.go b/ghutil/ghutil.go index 786f4da..3beb984 100644 --- a/ghutil/ghutil.go +++ b/ghutil/ghutil.go @@ -61,6 +61,7 @@ type IssuesService interface { // PullRequestsService is the subset of `github.PullRequestsService` used by // this module. type PullRequestsService interface { + CreateReview(ctx context.Context, owner string, repo string, number int, review *github.PullRequestReviewRequest) (*github.PullRequestReview, *github.Response, error) List(ctx context.Context, owner string, repo string, opt *github.PullRequestListOptions) ([]*github.PullRequest, *github.Response, error) ListCommits(ctx context.Context, owner string, repo string, number int, opt *github.ListOptions) ([]*github.RepositoryCommit, *github.Response, error) } @@ -384,8 +385,27 @@ func (ghc *GitHubClient) ProcessPullRequest(ctx context.Context, orgName string, } } + addReview := func(review string, event string) { + logging.Infof(" Adding review to repo '%s/%s/ PR %d: %s", orgName, repoName, *pull.Number, review) + if updateRepo { + prReview := github.PullRequestReviewRequest{ + Body: &review, + Event: &event, + } + _, _, err := ghc.PullRequests.CreateReview(ctx, orgName, repoName, *pull.Number, &prReview) + if err != nil { + logging.Errorf(" Error leaving review on PR %d: %v", *pull.Number, err) + } + } else { + logging.Info(" ... but -update-repo flag is disabled; skipping") + } + } + // Add or remove [cla: yes] and [cla: no] labels, as appropriate. if pullRequestIsCompliant { + logging.Info("Going to add a review") + addReview("Bad PR", "REQUEST_CHANGES") + //addReview("I APPROVED!", "APPROVE") // if PR has [cla: no] label, remove it. if hasLabelClaNo { removeLabel(LabelClaNo) From 9f15b27a31d5f8388bfb1fa773ef6ca9b4c6999f Mon Sep 17 00:00:00 2001 From: su7edja Date: Mon, 14 Oct 2019 11:12:58 -0700 Subject: [PATCH 2/4] Replaced addComment with addReview --- ghutil/ghutil.go | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/ghutil/ghutil.go b/ghutil/ghutil.go index 3beb984..55a2a42 100644 --- a/ghutil/ghutil.go +++ b/ghutil/ghutil.go @@ -35,6 +35,8 @@ const ( LabelClaYes = "cla: yes" LabelClaNo = "cla: no" LabelClaExternal = "cla: external" + + ReviewRequestChanges = "REQUEST_CHANGES" ) // OrganizationsService is the subset of `github.OrganizationsService` used by @@ -370,21 +372,6 @@ func (ghc *GitHubClient) ProcessPullRequest(ctx context.Context, orgName string, } } - addComment := func(comment string) { - logging.Infof(" Adding comment to repo '%s/%s/ PR %d: %s", orgName, repoName, *pull.Number, comment) - if updateRepo { - issueComment := github.IssueComment{ - Body: &comment, - } - _, _, err := ghc.Issues.CreateComment(ctx, orgName, repoName, *pull.Number, &issueComment) - if err != nil { - logging.Errorf(" Error leaving comment on PR %d: %v", *pull.Number, err) - } - } else { - logging.Info(" ... but -update-repo flag is disabled; skipping") - } - } - addReview := func(review string, event string) { logging.Infof(" Adding review to repo '%s/%s/ PR %d: %s", orgName, repoName, *pull.Number, review) if updateRepo { @@ -403,9 +390,6 @@ func (ghc *GitHubClient) ProcessPullRequest(ctx context.Context, orgName string, // Add or remove [cla: yes] and [cla: no] labels, as appropriate. if pullRequestIsCompliant { - logging.Info("Going to add a review") - addReview("Bad PR", "REQUEST_CHANGES") - //addReview("I APPROVED!", "APPROVE") // if PR has [cla: no] label, remove it. if hasLabelClaNo { removeLabel(LabelClaNo) @@ -436,7 +420,7 @@ func (ghc *GitHubClient) ProcessPullRequest(ctx context.Context, orgName string, } if labelsUpdatedForNonCompliance { - addComment(pullRequestNonComplianceReason) + addReview(pullRequestNonComplianceReason, ReviewRequestChanges) } } } From 74d399108cff687566d33731f56527737e2159d8 Mon Sep 17 00:00:00 2001 From: su7edja Date: Tue, 15 Oct 2019 15:41:27 -0700 Subject: [PATCH 3/4] Adding approve if PR is compliant --- ghutil/ghutil.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/ghutil/ghutil.go b/ghutil/ghutil.go index 6ad2d9e..3083df4 100644 --- a/ghutil/ghutil.go +++ b/ghutil/ghutil.go @@ -37,6 +37,7 @@ const ( LabelClaExternal = "cla: external" ReviewRequestChanges = "REQUEST_CHANGES" + ReviewApprove = "APPROVE" ) // OrganizationsService is the subset of `github.OrganizationsService` used by @@ -384,7 +385,7 @@ func (ghc *GitHubClient) ProcessPullRequest(ctx context.Context, orgName string, } addReview := func(review string, event string) { - logging.Infof(" Adding review to repo '%s/%s/ PR %d: %s", orgName, repoName, *pull.Number, review) + logging.Infof(" Adding %s review to repo '%s/%s/ PR %d: %s", event, orgName, repoName, *pull.Number, review) if updateRepo { prReview := github.PullRequestReviewRequest{ Body: &review, @@ -413,26 +414,23 @@ func (ghc *GitHubClient) ProcessPullRequest(ctx context.Context, orgName string, } else { logging.Infof(" No action needed: [%s] label already added", LabelClaYes) } + + addReview(pullRequestNonComplianceReason, ReviewApprove) } else /* !pullRequestIsCompliant */ { - labelsUpdatedForNonCompliance := false // if PR doesn't have [cla: no] label, add it. if !hasLabelClaNo { addLabel(LabelClaNo) - labelsUpdatedForNonCompliance = true } else { logging.Infof(" No action needed: [%s] label already added", LabelClaNo) } // if PR has [cla: yes] label, remove it. if hasLabelClaYes { removeLabel(LabelClaYes) - labelsUpdatedForNonCompliance = true } else { logging.Infof(" No action needed: [%s] label already missing", LabelClaYes) } - if labelsUpdatedForNonCompliance { - addReview(pullRequestNonComplianceReason, ReviewRequestChanges) - } + addReview(pullRequestNonComplianceReason, ReviewRequestChanges) } } return nil From f0c5882b21cb65fdb8c1b83f81a762ac096ed3ad Mon Sep 17 00:00:00 2001 From: su7edja Date: Sat, 26 Oct 2019 16:46:17 -0700 Subject: [PATCH 4/4] Added test for adding reviews --- ghutil/ghutil_test.go | 100 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/ghutil/ghutil_test.go b/ghutil/ghutil_test.go index de14049..40acf85 100644 --- a/ghutil/ghutil_test.go +++ b/ghutil/ghutil_test.go @@ -442,3 +442,103 @@ func TestCheckPullRequestCompliance_OneCompliantOneNot(t *testing.T) { assert.Equal(t, "Committer of one or more commits is not listed as a CLA signer, either individual or as a member of an organization.", nonComplianceReason) assert.Nil(t, err) } + +func TestCheckProcessPullRequest_Compliant_Approve(t *testing.T) { + setUp(t) + defer tearDown(t) + + expectRepoLabels(orgName, repoName, true, true, false) + repoClaLabelStatus := ghc.GetRepoClaLabelStatus(ctx, orgName, repoName) + + claSigners := config.ClaSigners{ + People: []config.Account{ + { + Name: "John Doe", + Email: "john@example.com", + Login: "john-doe", + }, + }, + } + var prNum = 42 + var num *int + num = &prNum + var prTitle = "Fix Things" + var title *string + title = &prTitle + pull := &github.PullRequest{Number: num, Title: title} + + var commits []*github.RepositoryCommit + mockGhc.PullRequests.EXPECT().ListCommits(ctx, orgName, repoName, pullNumber, nil).Return(commits, nil, nil) + mockGhc.Issues.EXPECT().ListLabelsByIssue(ctx, orgName, repoName, prNum, nil).Return(nil, nil, nil) + mockGhc.Issues.EXPECT().AddLabelsToIssue(ctx, orgName, repoName, prNum, []string{"cla: yes"}).Return(nil, nil, nil) + var reviewBody = "" + var body *string + body = &reviewBody + var reviewEvent = "APPROVE" + var event *string + event = &reviewEvent + var prReview = github.PullRequestReviewRequest{ + Body: body, + Event: event, + } + mockGhc.PullRequests.EXPECT().CreateReview(ctx, orgName, repoName, prNum, &prReview).Return(nil, nil, nil) + err := ghc.ProcessPullRequest(ctx, orgName, repoName, pull, claSigners, repoClaLabelStatus, true) + assert.Nil(t, err) +} + +func TestCheckProcessPullRequest_NotCompliant_RequestChanges(t *testing.T) { + setUp(t) + defer tearDown(t) + + expectRepoLabels(orgName, repoName, true, true, false) + repoClaLabelStatus := ghc.GetRepoClaLabelStatus(ctx, orgName, repoName) + + sha1 := "12345abcde" + name1 := "John Doe" + email1 := "john@example.com" + login1 := "john-doe" + + sha2 := "abcde24680" + name2 := "Jane Doe" + email2 := "jane@example.com" + login2 := "jane-doe" + + commits := []*github.RepositoryCommit{ + createCommit(sha1, name1, email1, login1), + createCommit(sha2, name2, email2, login2), + } + + claSigners := config.ClaSigners{ + People: []config.Account{ + { + Name: name1, + Email: email1, + Login: login1, + }, + }, + } + var prNum = 42 + var num *int + num = &prNum + var prTitle = "Fix Things" + var title *string + title = &prTitle + pull := &github.PullRequest{Number: num, Title: title} + + mockGhc.PullRequests.EXPECT().ListCommits(ctx, orgName, repoName, pullNumber, nil).Return(commits, nil, nil) + mockGhc.Issues.EXPECT().ListLabelsByIssue(ctx, orgName, repoName, prNum, nil).Return(nil, nil, nil) + mockGhc.Issues.EXPECT().AddLabelsToIssue(ctx, orgName, repoName, prNum, []string{"cla: no"}).Return(nil, nil, nil) + var reviewBody = "Committer of one or more commits is not listed as a CLA signer, either individual or as a member of an organization." + var body *string + body = &reviewBody + var reviewEvent = "REQUEST_CHANGES" + var event *string + event = &reviewEvent + var prReview = github.PullRequestReviewRequest{ + Body: body, + Event: event, + } + mockGhc.PullRequests.EXPECT().CreateReview(ctx, orgName, repoName, prNum, &prReview).Return(nil, nil, nil) + err := ghc.ProcessPullRequest(ctx, orgName, repoName, pull, claSigners, repoClaLabelStatus, true) + assert.Nil(t, err) +}