diff --git a/ghutil/ghutil.go b/ghutil/ghutil.go index 786f4da..5866fa3 100644 --- a/ghutil/ghutil.go +++ b/ghutil/ghutil.go @@ -307,19 +307,20 @@ func ProcessCommit(commit *github.RepositoryCommit, claSigners config.ClaSigners return } -func (ghc *GitHubClient) ProcessPullRequest(ctx context.Context, orgName string, repoName string, pull *github.PullRequest, claSigners config.ClaSigners, repoClaLabelStatus RepoClaLabelStatus, updateRepo bool) error { - logging.Infof("PR %d: %s", *pull.Number, *pull.Title) +func (ghc *GitHubClient) CheckPullRequestCompliance(ctx context.Context, orgName string, repoName string, pullNumber int, claSigners config.ClaSigners) (bool, string, error) { + var pullRequestIsCompliant bool + var pullRequestNonComplianceReason string + // List all commits for this PR - commits, _, err := ghc.PullRequests.ListCommits(ctx, orgName, repoName, *pull.Number, nil) + commits, _, err := ghc.PullRequests.ListCommits(ctx, orgName, repoName, pullNumber, nil) if err != nil { - logging.Error("Error finding all commits on PR", pull.Number) - return err + logging.Error("Error finding all commits on PR", pullNumber) + return pullRequestIsCompliant, pullRequestNonComplianceReason, err } // Start off with the base case that the PR is compliant and disqualify it if // anything is amiss. - pullRequestIsCompliant := true - var pullRequestNonComplianceReason string + pullRequestIsCompliant = true for _, commit := range commits { commitIsCompliant, commitNonComplianceReason := ProcessCommit(commit, claSigners) @@ -332,6 +333,16 @@ func (ghc *GitHubClient) ProcessPullRequest(ctx context.Context, orgName string, pullRequestIsCompliant = false } } + return pullRequestIsCompliant, pullRequestNonComplianceReason, nil +} + +func (ghc *GitHubClient) ProcessPullRequest(ctx context.Context, orgName string, repoName string, pull *github.PullRequest, claSigners config.ClaSigners, repoClaLabelStatus RepoClaLabelStatus, updateRepo bool) error { + logging.Infof("PR %d: %s", *pull.Number, *pull.Title) + + pullRequestIsCompliant, pullRequestNonComplianceReason, err := ghc.CheckPullRequestCompliance(ctx, orgName, repoName, *pull.Number, claSigners) + if err != nil { + return err + } if pullRequestIsCompliant { logging.Info(" PR is CLA-compliant") @@ -446,7 +457,10 @@ func (ghc *GitHubClient) ProcessOrgRepo(ctx context.Context, repoSpec GitHubProc // Process each pull request for author & commiter CLA status. repoClaLabelStatus := ghc.GetRepoClaLabelStatus(ctx, orgName, repoName) for _, pull := range pulls { - ghc.ProcessPullRequest(ctx, orgName, repoName, pull, claSigners, repoClaLabelStatus, repoSpec.UpdateRepo) + err := ghc.ProcessPullRequest(ctx, orgName, repoName, pull, claSigners, repoClaLabelStatus, repoSpec.UpdateRepo) + if err != nil { + logging.Errorf("Error processing PR %d: %s", *pull.Number, err) + } } } } diff --git a/ghutil/ghutil_test.go b/ghutil/ghutil_test.go index 90bfd8b..de14049 100644 --- a/ghutil/ghutil_test.go +++ b/ghutil/ghutil_test.go @@ -16,6 +16,7 @@ package ghutil_test import ( "context" + "errors" "testing" "github.com/golang/mock/gomock" @@ -61,8 +62,9 @@ var ( ) const ( - orgName = "org" - repoName = "repo" + orgName = "org" + repoName = "repo" + pullNumber = 42 ) func setUp(t *testing.T) { @@ -329,3 +331,114 @@ func TestGmailAddress_PeriodsDoNotMatchCLA(t *testing.T) { assert.True(t, ghutil.MatchAccount(account, accounts)) } } + +func TestCheckPullRequestCompliance_ListCommitsError(t *testing.T) { + setUp(t) + defer tearDown(t) + + err := errors.New("Invalid PR") + mockGhc.PullRequests.EXPECT().ListCommits(ctx, orgName, repoName, pullNumber, nil).Return(nil, nil, err) + + claSigners := config.ClaSigners{} + isCompliant, nonComplianceReason, retErr := ghc.CheckPullRequestCompliance(ctx, orgName, repoName, pullNumber, claSigners) + assert.False(t, isCompliant) + assert.Equal(t, "", nonComplianceReason) + assert.Equal(t, err, retErr) +} + +func createCommit(sha string, name string, email string, login string) *github.RepositoryCommit { + return &github.RepositoryCommit{ + SHA: &sha, + Commit: &github.Commit{ + Author: &github.CommitAuthor{ + Name: &name, + Email: &email, + }, + Committer: &github.CommitAuthor{ + Name: &name, + Email: &email, + }, + }, + Author: &github.User{ + Login: &login, + }, + Committer: &github.User{ + Login: &login, + }, + } +} + +func TestCheckPullRequestCompliance_TwoCompliantCommits(t *testing.T) { + setUp(t) + defer tearDown(t) + + 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), + } + mockGhc.PullRequests.EXPECT().ListCommits(ctx, orgName, repoName, pullNumber, nil).Return(commits, nil, nil) + + claSigners := config.ClaSigners{ + People: []config.Account{ + { + Name: name1, + Email: email1, + Login: login1, + }, + { + Name: name2, + Email: email2, + Login: login2, + }, + }, + } + isCompliant, nonComplianceReason, err := ghc.CheckPullRequestCompliance(ctx, orgName, repoName, pullNumber, claSigners) + assert.True(t, isCompliant) + assert.Equal(t, "", nonComplianceReason) + assert.Nil(t, err) +} + +func TestCheckPullRequestCompliance_OneCompliantOneNot(t *testing.T) { + setUp(t) + defer tearDown(t) + + 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), + } + mockGhc.PullRequests.EXPECT().ListCommits(ctx, orgName, repoName, pullNumber, nil).Return(commits, nil, nil) + + claSigners := config.ClaSigners{ + People: []config.Account{ + { + Name: name1, + Email: email1, + Login: login1, + }, + }, + } + isCompliant, nonComplianceReason, err := ghc.CheckPullRequestCompliance(ctx, orgName, repoName, pullNumber, claSigners) + assert.False(t, isCompliant) + 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) +}