Skip to content

Add a review to a PR instead of a comment (issue #14) #36

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add a review to a PR instead of a comment (issue #14) #36

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eedb219
Added function to addReview to PR with approve/request_changes/commen…
su7edja Oct 14, 2019
9f15b27
Replaced addComment with addReview
su7edja Oct 14, 2019
a31da84
Merge branch 'master' into issue-14
su7edja Oct 15, 2019
74d3991
Adding approve if PR is compliant
su7edja Oct 15, 2019
f0c5882
Added test for adding reviews
su7edja Oct 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 14 additions & 12 deletions ghutil/ghutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ const (
LabelClaYes = "cla: yes"
LabelClaNo = "cla: no"
LabelClaExternal = "cla: external"

ReviewRequestChanges = "REQUEST_CHANGES"
ReviewApprove = "APPROVE"
)

// OrganizationsService is the subset of `github.OrganizationsService` used by
Expand All @@ -61,6 +64,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)
}
Expand Down Expand Up @@ -380,15 +384,16 @@ 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)
addReview := func(review string, event string) {
logging.Infof(" Adding %s review to repo '%s/%s/ PR %d: %s", event, orgName, repoName, *pull.Number, review)
if updateRepo {
issueComment := github.IssueComment{
Body: &comment,
prReview := github.PullRequestReviewRequest{
Body: &review,
Event: &event,
}
_, _, err := ghc.Issues.CreateComment(ctx, orgName, repoName, *pull.Number, &issueComment)
_, _, err := ghc.PullRequests.CreateReview(ctx, orgName, repoName, *pull.Number, &prReview)
if err != nil {
logging.Errorf(" Error leaving comment on PR %d: %v", *pull.Number, err)
logging.Errorf(" Error leaving review on PR %d: %v", *pull.Number, err)
}
} else {
logging.Info(" ... but -update-repo flag is disabled; skipping")
Expand All @@ -409,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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If "ReviewApprove" is going to add an "approval" to the PR in the sense of getting a check mark on the PR, that's a bit of an issue, because the bot having commit permissions on the repo (which appears to be required to add/remove labels) means that just passing the CLA check gets a user a valid green check, and thus making it look like it's ready for merge.

Is there another state we can have the bot take which just resolves their comments on the PR, without approving the PR for the contents, since it's not actually reviewing the meaning of the PR, just CLA compliance?

} 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 {
addComment(pullRequestNonComplianceReason)
}
addReview(pullRequestNonComplianceReason, ReviewRequestChanges)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing I just realized: this is a big change of behavior for crbot, so it would be nice to control this with a command-line flag, and to be able to keep the old behavior (adding labels) as an option as well.

In fact, it would probably be safest to keep the old behavior as default, and make the new behavior something users can enable with a command-line flag, so that folks can try it out, without being suddenly surprised with a new behavior when they sync + rebuild their binaries.

}
}
return nil
Expand Down
100 changes: 100 additions & 0 deletions ghutil/ghutil_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: "[email protected]",
Login: "john-doe",
},
},
}
var prNum = 42
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're using prNum here, but there's already a global const pullNumber which has the same value, and you're using both of them below, so I think you can just drop this line. Similarly in the other test method.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just inline &"" below where you use body? Or if that's not possible, just inline &reviewBody? Similarly below and in the other test method.

var reviewEvent = "APPROVE"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use the constant from the ghutil package here and in the other test method instead of copying it here?

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 := "[email protected]"
login1 := "john-doe"

sha2 := "abcde24680"
name2 := "Jane Doe"
email2 := "[email protected]"
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)
}