Update SAST and CITest with Repoclient API (#842)

Co-authored-by: Azeem Shaikh <azeems@google.com>
ossf · Aug 11, 2021 · eeb563b · eeb563b
1 parent 5bcc1fd
commit eeb563b
Show file tree

Hide file tree

Showing 6 changed files with 33 additions and 23 deletions.
diff --git a/checks/ci_tests.go b/checks/ci_tests.go
@@ -21,6 +21,7 @@ import (
 	"github.com/google/go-github/v32/github"
 
 	"github.com/ossf/scorecard/v2/checker"
+	"github.com/ossf/scorecard/v2/clients"
 	sce "github.com/ossf/scorecard/v2/errors"
 )
 
@@ -43,19 +44,18 @@ func init() {
 
 // CITests runs CI-Tests check.
 func CITests(c *checker.CheckRequest) checker.CheckResult {
-	prs, _, err := c.Client.PullRequests.List(c.Ctx, c.Owner, c.Repo, &github.PullRequestListOptions{
-		State: "closed",
-	})
+	prs, err := c.RepoClient.ListMergedPRs()
 	if err != nil {
-		e := sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("Client.PullRequests.List: %v", err))
+		e := sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListMergedPRs: %v", err))
 		return checker.CreateRuntimeErrorResult(CheckCITests, e)
 	}
 
 	usedSystem := unknown
 	totalMerged := 0
 	totalTested := 0
-	for _, pr := range prs {
-		if pr.MergedAt == nil {
+	for index := range prs {
+		pr := &prs[index]
+		if pr.MergedAt.IsZero() {
 			continue
 		}
 		totalMerged++
@@ -90,7 +90,7 @@ func CITests(c *checker.CheckRequest) checker.CheckResult {
 		}
 
 		if !foundCI {
-			c.Dlogger.Debug("merged PR without CI test: %d", pr.GetNumber())
+			c.Dlogger.Debug("merged PR without CI test: %d", pr.Number)
 		}
 	}
 
@@ -103,8 +103,8 @@ func CITests(c *checker.CheckRequest) checker.CheckResult {
 }
 
 // PR has a status marked 'success' and a CI-related context.
-func prHasSuccessStatus(pr *github.PullRequest, c *checker.CheckRequest) (bool, error) {
-	statuses, _, err := c.Client.Repositories.ListStatuses(c.Ctx, c.Owner, c.Repo, pr.GetHead().GetSHA(),
+func prHasSuccessStatus(pr *clients.PullRequest, c *checker.CheckRequest) (bool, error) {
+	statuses, _, err := c.Client.Repositories.ListStatuses(c.Ctx, c.Owner, c.Repo, pr.HeadSHA,
 		&github.ListOptions{})
 	if err != nil {
 		//nolint
@@ -116,7 +116,7 @@ func prHasSuccessStatus(pr *github.PullRequest, c *checker.CheckRequest) (bool,
 			continue
 		}
 		if isTest(status.GetContext()) {
-			c.Dlogger.Debug("CI test found: pr: %d, context: %success, url: %success", pr.GetNumber(),
+			c.Dlogger.Debug("CI test found: pr: %d, context: %success, url: %success", pr.Number,
 				status.GetContext(), status.GetURL())
 			return true, nil
 		}
@@ -125,8 +125,8 @@ func prHasSuccessStatus(pr *github.PullRequest, c *checker.CheckRequest) (bool,
 }
 
 // PR has a successful CI-related check.
-func prHasSuccessfulCheck(pr *github.PullRequest, c *checker.CheckRequest) (bool, error) {
-	crs, _, err := c.Client.Checks.ListCheckRunsForRef(c.Ctx, c.Owner, c.Repo, pr.GetHead().GetSHA(),
+func prHasSuccessfulCheck(pr *clients.PullRequest, c *checker.CheckRequest) (bool, error) {
+	crs, _, err := c.Client.Checks.ListCheckRunsForRef(c.Ctx, c.Owner, c.Repo, pr.HeadSHA,
 		&github.ListCheckRunsOptions{})
 	if err != nil {
 		//nolint
@@ -145,7 +145,7 @@ func prHasSuccessfulCheck(pr *github.PullRequest, c *checker.CheckRequest) (bool
 			continue
 		}
 		if isTest(cr.GetApp().GetSlug()) {
-			c.Dlogger.Debug("CI test found: pr: %d, context: %success, url: %success", pr.GetNumber(),
+			c.Dlogger.Debug("CI test found: pr: %d, context: %success, url: %success", pr.Number,
 				cr.GetApp().GetSlug(), cr.GetURL())
 			return true, nil
 		}

diff --git a/checks/sast.go b/checks/sast.go
@@ -104,23 +104,21 @@ func SAST(c *checker.CheckRequest) checker.CheckResult {
 
 // nolint
 func sastToolInCheckRuns(c *checker.CheckRequest) (int, error) {
-	prs, _, err := c.Client.PullRequests.List(c.Ctx, c.Owner, c.Repo, &github.PullRequestListOptions{
-		State: "closed",
-	})
+	prs, err := c.RepoClient.ListMergedPRs()
 	if err != nil {
 		//nolint
 		return checker.InconclusiveResultScore,
-			sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("Client.PullRequests.List: %v", err))
+			sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListMergedPRs: %v", err))
 	}
 
 	totalMerged := 0
 	totalTested := 0
 	for _, pr := range prs {
-		if pr.MergedAt == nil {
+		if pr.MergedAt.IsZero() {
 			continue
 		}
 		totalMerged++
-		crs, _, err := c.Client.Checks.ListCheckRunsForRef(c.Ctx, c.Owner, c.Repo, pr.GetHead().GetSHA(),
+		crs, _, err := c.Client.Checks.ListCheckRunsForRef(c.Ctx, c.Owner, c.Repo, pr.HeadSHA,
 			&github.ListCheckRunsOptions{})
 		if err != nil {
 			return checker.InconclusiveResultScore,

diff --git a/clients/githubrepo/graphql.go b/clients/githubrepo/graphql.go
@@ -60,6 +60,7 @@ type graphqlData struct {
 		PullRequests struct {
 			Nodes []struct {
 				Number      githubv4.Int
+				HeadRefOid  githubv4.String
 				MergeCommit struct {
 					AuthoredByCommitter githubv4.Boolean
 				}
@@ -130,6 +131,7 @@ func pullRequestFrom(data *graphqlData) []clients.PullRequest {
 	for i, pr := range data.Repository.PullRequests.Nodes {
 		toAppend := clients.PullRequest{
 			Number:   int(pr.Number),
+			HeadSHA:  string(pr.HeadRefOid),
 			MergedAt: pr.MergedAt.Time,
 			MergeCommit: clients.Commit{
 				AuthoredByCommitter: bool(pr.MergeCommit.AuthoredByCommitter),

diff --git a/clients/pull_request.go b/clients/pull_request.go
@@ -24,6 +24,7 @@ type PullRequest struct {
 	MergedAt    time.Time
 	MergeCommit Commit
 	Number      int
+	HeadSHA     string
 	Labels      []Label
 	Reviews     []Review
 }

diff --git a/e2e/ci_tests_test.go b/e2e/ci_tests_test.go
@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+// nolint: dupl
 package e2e
 
 import (
@@ -22,18 +23,22 @@ import (
 
 	"github.com/ossf/scorecard/v2/checker"
 	"github.com/ossf/scorecard/v2/checks"
+	"github.com/ossf/scorecard/v2/clients/githubrepo"
 	scut "github.com/ossf/scorecard/v2/utests"
 )
 
-var _ = Describe("E2E TEST:CITests", func() {
+var _ = Describe("E2E TEST:"+checks.CheckCITests, func() {
 	Context("E2E TEST:Validating use of CI tests", func() {
 		It("Should return use of CI tests", func() {
 			dl := scut.TestDetailLogger{}
+			repoClient := githubrepo.CreateGithubRepoClient(context.Background(), ghClient, graphClient)
+			err := repoClient.InitRepo("apache", "airflow")
+			Expect(err).Should(BeNil())
 			req := checker.CheckRequest{
 				Ctx:         context.Background(),
 				Client:      ghClient,
 				HTTPClient:  httpClient,
-				RepoClient:  nil,
+				RepoClient:  repoClient,
 				Owner:       "apache",
 				Repo:        "airflow",
 				GraphClient: graphClient,

diff --git a/e2e/sast_test.go b/e2e/sast_test.go
@@ -22,18 +22,22 @@ import (
 
 	"github.com/ossf/scorecard/v2/checker"
 	"github.com/ossf/scorecard/v2/checks"
+	"github.com/ossf/scorecard/v2/clients/githubrepo"
 	scut "github.com/ossf/scorecard/v2/utests"
 )
 
-var _ = Describe("E2E TEST:SAST", func() {
+var _ = Describe("E2E TEST:"+checks.CheckSAST, func() {
 	Context("E2E TEST:Validating use of SAST tools", func() {
 		It("Should return use of SAST tools", func() {
 			dl := scut.TestDetailLogger{}
+			repoClient := githubrepo.CreateGithubRepoClient(context.Background(), ghClient, graphClient)
+			err := repoClient.InitRepo("apache", "airflow")
+			Expect(err).Should(BeNil())
 			req := checker.CheckRequest{
 				Ctx:         context.Background(),
 				Client:      ghClient,
 				HTTPClient:  httpClient,
-				RepoClient:  nil,
+				RepoClient:  repoClient,
 				Owner:       "apache",
 				Repo:        "airflow",
 				GraphClient: graphClient,