From 7053e638ad2b51e21283fcae001b1a587dbef5af Mon Sep 17 00:00:00 2001 From: Azeem Shaikh Date: Wed, 2 Feb 2022 13:51:09 -0800 Subject: [PATCH] Remove ListMergedPRs API --- checks/ci_tests.go | 12 +-- checks/maintained.go | 4 +- checks/sast.go | 11 ++- checks/sast_test.go | 109 ++++++++++++++++++++------ clients/commit.go | 9 ++- clients/githubrepo/client.go | 5 -- clients/githubrepo/graphql.go | 77 ++++++------------ clients/localdir/client.go | 5 -- clients/mockclients/repo_client.go | 15 ---- clients/pull_request.go | 14 ++-- clients/repo_client.go | 1 - docs/checks/internal/validate/main.go | 4 +- 12 files changed, 139 insertions(+), 127 deletions(-) diff --git a/checks/ci_tests.go b/checks/ci_tests.go index 09f39c79ae2..82d42bdbef5 100644 --- a/checks/ci_tests.go +++ b/checks/ci_tests.go @@ -39,17 +39,19 @@ func init() { // CITests runs CI-Tests check. func CITests(c *checker.CheckRequest) checker.CheckResult { - prs, err := c.RepoClient.ListMergedPRs() + commits, err := c.RepoClient.ListCommits() if err != nil { - e := sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListMergedPRs: %v", err)) + e := sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListCommits: %v", err)) return checker.CreateRuntimeErrorResult(CheckCITests, e) } totalMerged := 0 totalTested := 0 - for index := range prs { - pr := &prs[index] - if pr.MergedAt.IsZero() { + for i := range commits { + pr := &commits[i].AssociatedMergeRequest + // TODO(#575): We ignore associated PRs if Scorecard is being run on a fork + // but the PR was created in the original repo. + if pr.MergedAt.IsZero() || pr.Repository != c.Repo.String() { continue } totalMerged++ diff --git a/checks/maintained.go b/checks/maintained.go index b4d7089af5e..7e33a962ad1 100644 --- a/checks/maintained.go +++ b/checks/maintained.go @@ -59,8 +59,8 @@ func IsMaintained(c *checker.CheckRequest) checker.CheckResult { return checker.CreateRuntimeErrorResult(CheckMaintained, e) } commitsWithinThreshold := 0 - for _, commit := range commits { - if commit.CommittedDate.After(threshold) { + for i := range commits { + if commits[i].CommittedDate.After(threshold) { commitsWithinThreshold++ } } diff --git a/checks/sast.go b/checks/sast.go index 30f39cc1708..301d735348e 100644 --- a/checks/sast.go +++ b/checks/sast.go @@ -108,17 +108,20 @@ func SAST(c *checker.CheckRequest) checker.CheckResult { // nolint func sastToolInCheckRuns(c *checker.CheckRequest) (int, error) { - prs, err := c.RepoClient.ListMergedPRs() + commits, err := c.RepoClient.ListCommits() if err != nil { //nolint return checker.InconclusiveResultScore, - sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListMergedPRs: %v", err)) + sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListCommits: %v", err)) } totalMerged := 0 totalTested := 0 - for _, pr := range prs { - if pr.MergedAt.IsZero() { + for _, commit := range commits { + pr := commit.AssociatedMergeRequest + // TODO(#575): We ignore associated PRs if Scorecard is being run on a fork + // but the PR was created in the original repo. + if pr.MergedAt.IsZero() || pr.Repository != c.Repo.String() { continue } totalMerged++ diff --git a/checks/sast_test.go b/checks/sast_test.go index d29b873b93c..06af92bdc0f 100644 --- a/checks/sast_test.go +++ b/checks/sast_test.go @@ -30,10 +30,12 @@ import ( func TestSAST(t *testing.T) { t.Parallel() - //nolint + + const testRepo = "test-repo" + // nolint: govet, goerr113 tests := []struct { name string - prs []clients.PullRequest + commits []clients.Commit err error searchresult clients.SearchResponse checkRuns []clients.CheckRun @@ -42,23 +44,26 @@ func TestSAST(t *testing.T) { }{ { name: "SAST checker should return failed status when no PRs are found", - prs: []clients.PullRequest{}, + commits: []clients.Commit{}, searchresult: clients.SearchResponse{}, checkRuns: []clients.CheckRun{}, }, { name: "SAST checker should return failed status when no PRs are found", err: errors.New("error"), - prs: []clients.PullRequest{}, + commits: []clients.Commit{}, searchresult: clients.SearchResponse{}, checkRuns: []clients.CheckRun{}, expected: checker.CheckResult{Score: -1, Pass: false}, }, { name: "Successful SAST checker should return success status", - prs: []clients.PullRequest{ + commits: []clients.Commit{ { - MergedAt: time.Now().Add(time.Hour - 1), + AssociatedMergeRequest: clients.PullRequest{ + Repository: testRepo, + MergedAt: time.Now().Add(time.Hour - 1), + }, }, }, searchresult: clients.SearchResponse{}, @@ -78,18 +83,30 @@ func TestSAST(t *testing.T) { }, { name: "Failed SAST checker should return success status", - prs: []clients.PullRequest{ + commits: []clients.Commit{ { - MergedAt: time.Now().Add(time.Hour - 1), + AssociatedMergeRequest: clients.PullRequest{ + Repository: testRepo, + MergedAt: time.Now().Add(time.Hour - 1), + }, }, { - MergedAt: time.Now().Add(time.Hour - 10), + AssociatedMergeRequest: clients.PullRequest{ + Repository: testRepo, + MergedAt: time.Now().Add(time.Hour - 10), + }, }, { - MergedAt: time.Now().Add(time.Hour - 20), + AssociatedMergeRequest: clients.PullRequest{ + Repository: testRepo, + MergedAt: time.Now().Add(time.Hour - 20), + }, }, { - MergedAt: time.Now().Add(time.Hour - 30), + AssociatedMergeRequest: clients.PullRequest{ + Repository: testRepo, + MergedAt: time.Now().Add(time.Hour - 30), + }, }, }, searchresult: clients.SearchResponse{Hits: 1, Results: []clients.SearchResult{{ @@ -110,18 +127,30 @@ func TestSAST(t *testing.T) { }, { name: "Failed SAST checker with checkRuns not completed", - prs: []clients.PullRequest{ + commits: []clients.Commit{ { - MergedAt: time.Now().Add(time.Hour - 1), + AssociatedMergeRequest: clients.PullRequest{ + Repository: testRepo, + MergedAt: time.Now().Add(time.Hour - 1), + }, }, { - MergedAt: time.Now().Add(time.Hour - 10), + AssociatedMergeRequest: clients.PullRequest{ + Repository: testRepo, + MergedAt: time.Now().Add(time.Hour - 10), + }, }, { - MergedAt: time.Now().Add(time.Hour - 20), + AssociatedMergeRequest: clients.PullRequest{ + Repository: testRepo, + MergedAt: time.Now().Add(time.Hour - 20), + }, }, { - MergedAt: time.Now().Add(time.Hour - 30), + AssociatedMergeRequest: clients.PullRequest{ + Repository: testRepo, + MergedAt: time.Now().Add(time.Hour - 30), + }, }, }, searchresult: clients.SearchResponse{}, @@ -139,9 +168,36 @@ func TestSAST(t *testing.T) { }, { name: "Failed SAST with PullRequest not merged", - prs: []clients.PullRequest{ + commits: []clients.Commit{ { - Number: 1, + AssociatedMergeRequest: clients.PullRequest{ + Repository: testRepo, + Number: 1, + }, + }, + }, + searchresult: clients.SearchResponse{}, + checkRuns: []clients.CheckRun{ + { + App: clients.CheckRunApp{ + Slug: "lgtm-com", + }, + }, + }, + expected: checker.CheckResult{ + Score: 0, + Pass: false, + }, + }, + { + name: "Merged PullRequest in a different repo", + commits: []clients.Commit{ + { + AssociatedMergeRequest: clients.PullRequest{ + Repository: "does-not-exist", + MergedAt: time.Now(), + Number: 1, + }, }, }, searchresult: clients.SearchResponse{}, @@ -167,20 +223,23 @@ func TestSAST(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) - mockRepo := mockrepo.NewMockRepoClient(ctrl) - - mockRepo.EXPECT().ListMergedPRs().DoAndReturn(func() ([]clients.PullRequest, error) { + mockRepoClient := mockrepo.NewMockRepoClient(ctrl) + mockRepoClient.EXPECT().ListCommits().DoAndReturn(func() ([]clients.Commit, error) { if tt.err != nil { return nil, tt.err } - return tt.prs, tt.err + return tt.commits, tt.err }) - mockRepo.EXPECT().ListCheckRunsForRef("").Return(tt.checkRuns, nil).AnyTimes() - mockRepo.EXPECT().Search(searchRequest).Return(tt.searchresult, nil).AnyTimes() + mockRepoClient.EXPECT().ListCheckRunsForRef("").Return(tt.checkRuns, nil).AnyTimes() + mockRepoClient.EXPECT().Search(searchRequest).Return(tt.searchresult, nil).AnyTimes() + + mockRepo := mockrepo.NewMockRepo(ctrl) + mockRepo.EXPECT().String().Return(testRepo).AnyTimes() dl := scut.TestDetailLogger{} req := checker.CheckRequest{ - RepoClient: mockRepo, + RepoClient: mockRepoClient, + Repo: mockRepo, Ctx: context.TODO(), Dlogger: &dl, } diff --git a/clients/commit.go b/clients/commit.go index fb8105cd0d3..3ff5cc13994 100644 --- a/clients/commit.go +++ b/clients/commit.go @@ -18,8 +18,9 @@ import "time" // Commit represents a Git commit. type Commit struct { - CommittedDate time.Time - Message string - SHA string - Committer User + CommittedDate time.Time + Message string + SHA string + Committer User + AssociatedMergeRequest PullRequest } diff --git a/clients/githubrepo/client.go b/clients/githubrepo/client.go index e21f3ca85f8..49a1b89ea5f 100644 --- a/clients/githubrepo/client.go +++ b/clients/githubrepo/client.go @@ -113,11 +113,6 @@ func (client *Client) GetFileContent(filename string) ([]byte, error) { return client.tarball.getFileContent(filename) } -// ListMergedPRs implements RepoClient.ListMergedPRs. -func (client *Client) ListMergedPRs() ([]clients.PullRequest, error) { - return client.graphClient.getMergedPRs() -} - // ListCommits implements RepoClient.ListCommits. func (client *Client) ListCommits() ([]clients.Commit, error) { return client.graphClient.getCommits() diff --git a/clients/githubrepo/graphql.go b/clients/githubrepo/graphql.go index 15b7de0ea31..639ef2d53db 100644 --- a/clients/githubrepo/graphql.go +++ b/clients/githubrepo/graphql.go @@ -122,7 +122,6 @@ type graphqlHandler struct { errSetup error owner string repo string - prs []clients.PullRequest commits []clients.Commit issues []clients.Issue archived bool @@ -154,7 +153,6 @@ func (handler *graphqlHandler) setup() error { return } handler.archived = bool(handler.data.Repository.IsArchived) - handler.prs = pullRequestsFrom(handler.data, handler.owner, handler.repo) handler.commits, handler.errSetup = commitsFrom(handler.data) if handler.errSetup != nil { return @@ -164,13 +162,6 @@ func (handler *graphqlHandler) setup() error { return handler.errSetup } -func (handler *graphqlHandler) getMergedPRs() ([]clients.PullRequest, error) { - if err := handler.setup(); err != nil { - return nil, fmt.Errorf("error during graphqlHandler.setup: %w", err) - } - return handler.prs, nil -} - func (handler *graphqlHandler) getCommits() ([]clients.Commit, error) { if err := handler.setup(); err != nil { return nil, fmt.Errorf("error during graphqlHandler.setup: %w", err) @@ -192,67 +183,48 @@ func (handler *graphqlHandler) isArchived() (bool, error) { return handler.archived, nil } -func pullRequestsFrom(data *graphqlData, repoOwner, repoName string) []clients.PullRequest { - var ret []clients.PullRequest +func commitsFrom(data *graphqlData) ([]clients.Commit, error) { + ret := make([]clients.Commit, 0) for _, commit := range data.Repository.DefaultBranchRef.Target.Commit.History.Nodes { + var committer string + if commit.Committer.User.Login != nil { + committer = *commit.Committer.User.Login + } + // TODO(#1543): Figure out a way to safely get committer if `User.Login` is `nil`. + var associatedPR clients.PullRequest for i := range commit.AssociatedPullRequests.Nodes { pr := commit.AssociatedPullRequests.Nodes[i] - if pr.MergeCommit.Oid != commit.Oid || - string(pr.Repository.Name) != repoName || - string(pr.Repository.Owner.Login) != repoOwner { + if pr.MergeCommit.Oid != commit.Oid { continue } - toAppend := clients.PullRequest{ - Number: int(pr.Number), - HeadSHA: string(pr.HeadRefOid), - MergedAt: pr.MergedAt.Time, + associatedRepo, err := MakeGithubRepo(fmt.Sprintf("%s/%s", + string(pr.Repository.Owner.Login), string(pr.Repository.Name))) + if err != nil { + return nil, sce.WithMessage(sce.ErrScorecardInternal, + fmt.Sprintf("associatedPR repository is invalid %s/%s", + string(pr.Repository.Owner.Login), string(pr.Repository.Name))) + } + associatedPR = clients.PullRequest{ + Repository: associatedRepo.String(), + Number: int(pr.Number), + HeadSHA: string(pr.HeadRefOid), + MergedAt: pr.MergedAt.Time, Author: clients.User{ Login: string(pr.Author.Login), }, - // TODO(#575): Use the original commit data here directly. - MergeCommit: clients.Commit{ - Committer: clients.User{ - Login: string(commit.Author.User.Login), - }, - SHA: string(pr.MergeCommit.Oid), - }, } - for _, label := range pr.Labels.Nodes { - toAppend.Labels = append(toAppend.Labels, clients.Label{ + associatedPR.Labels = append(associatedPR.Labels, clients.Label{ Name: string(label.Name), }) } for _, review := range pr.Reviews.Nodes { - r := clients.Review{ + associatedPR.Reviews = append(associatedPR.Reviews, clients.Review{ State: string(review.State), - } - - a := clients.User{ - Login: string(review.Author.Login), - } - if a.Login != "" { - r.Author = &a - } - toAppend.Reviews = append(toAppend.Reviews, r) + }) } - - ret = append(ret, toAppend) break } - } - return ret -} - -// nolint: unparam -func commitsFrom(data *graphqlData) ([]clients.Commit, error) { - ret := make([]clients.Commit, 0) - for _, commit := range data.Repository.DefaultBranchRef.Target.Commit.History.Nodes { - var committer string - if commit.Committer.User.Login != nil { - committer = *commit.Committer.User.Login - } - // TODO(#1543): Figure out a way to safely get committer if `User.Login` is `nil`. ret = append(ret, clients.Commit{ CommittedDate: commit.CommittedDate.Time, Message: string(commit.Message), @@ -260,6 +232,7 @@ func commitsFrom(data *graphqlData) ([]clients.Commit, error) { Committer: clients.User{ Login: committer, }, + AssociatedMergeRequest: associatedPR, }) } return ret, nil diff --git a/clients/localdir/client.go b/clients/localdir/client.go index 27e5c0f30e8..6f0e76d44f0 100644 --- a/clients/localdir/client.go +++ b/clients/localdir/client.go @@ -153,11 +153,6 @@ func (client *localDirClient) GetFileContent(filename string) ([]byte, error) { return getFileContent(client.path, filename) } -// ListMergedPRs implements RepoClient.ListMergedPRs. -func (client *localDirClient) ListMergedPRs() ([]clients.PullRequest, error) { - return nil, fmt.Errorf("ListMergedPRs: %w", clients.ErrUnsupportedFeature) -} - // ListBranches implements RepoClient.ListBranches. func (client *localDirClient) ListBranches() ([]*clients.BranchRef, error) { return nil, fmt.Errorf("ListBranches: %w", clients.ErrUnsupportedFeature) diff --git a/clients/mockclients/repo_client.go b/clients/mockclients/repo_client.go index c1ff27ff3f8..f6a70371ffe 100644 --- a/clients/mockclients/repo_client.go +++ b/clients/mockclients/repo_client.go @@ -212,21 +212,6 @@ func (mr *MockRepoClientMockRecorder) ListIssues() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListIssues", reflect.TypeOf((*MockRepoClient)(nil).ListIssues)) } -// ListMergedPRs mocks base method. -func (m *MockRepoClient) ListMergedPRs() ([]clients.PullRequest, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ListMergedPRs") - ret0, _ := ret[0].([]clients.PullRequest) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// ListMergedPRs indicates an expected call of ListMergedPRs. -func (mr *MockRepoClientMockRecorder) ListMergedPRs() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListMergedPRs", reflect.TypeOf((*MockRepoClient)(nil).ListMergedPRs)) -} - // ListReleases mocks base method. func (m *MockRepoClient) ListReleases() ([]clients.Release, error) { m.ctrl.T.Helper() diff --git a/clients/pull_request.go b/clients/pull_request.go index e4700f74098..34b35aa68a5 100644 --- a/clients/pull_request.go +++ b/clients/pull_request.go @@ -21,13 +21,13 @@ import ( // PullRequest struct represents a PR as returned by RepoClient. // nolint: govet type PullRequest struct { - MergedAt time.Time - MergeCommit Commit - Number int - HeadSHA string - Labels []Label - Reviews []Review - Author User + Number int + MergedAt time.Time + HeadSHA string + Repository string + Author User + Labels []Label + Reviews []Review } // Label represents a PR label. diff --git a/clients/repo_client.go b/clients/repo_client.go index 97dc59b07e5..5f78f46b017 100644 --- a/clients/repo_client.go +++ b/clients/repo_client.go @@ -27,7 +27,6 @@ type RepoClient interface { IsArchived() (bool, error) ListFiles(predicate func(string) (bool, error)) ([]string, error) GetFileContent(filename string) ([]byte, error) - ListMergedPRs() ([]PullRequest, error) ListBranches() ([]*BranchRef, error) GetDefaultBranch() (*BranchRef, error) ListCommits() ([]Commit, error) diff --git a/docs/checks/internal/validate/main.go b/docs/checks/internal/validate/main.go index 734c23943d4..2fd38d33594 100644 --- a/docs/checks/internal/validate/main.go +++ b/docs/checks/internal/validate/main.go @@ -40,7 +40,6 @@ var ( "IsArchived": {"GitHub"}, "ListFiles": {"GitHub", "local"}, "GetFileContent": {"GitHub", "local"}, - "ListMergedPRs": {"GitHub"}, "ListBranches": {"GitHub"}, "GetDefaultBranch": {"GitHub"}, "ListCommits": {"GitHub"}, @@ -67,7 +66,8 @@ func listCheckFiles() (map[string]string, error) { } for _, file := range files { - if !strings.HasSuffix(file.Name(), ".go") || file.IsDir() { + if !strings.HasSuffix(file.Name(), ".go") || + strings.HasSuffix(file.Name(), "_test.go") || file.IsDir() { continue }