Skip to content

Commit 1fe9644

Browse files
azeemshaikh38azeemshaikh38
committed
Refactor githubrepo CheckRun logic
Signed-off-by: Azeem Shaikh <[email protected]>
1 parent 8add330 commit 1fe9644

6 files changed

Lines changed: 235 additions & 203 deletions

File tree

‎clients/githubrepo/checkruns.go‎

Lines changed: 125 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,31 +17,151 @@ package githubrepo
1717
import (
1818
"context"
1919
"fmt"
20+
"strings"
21+
"sync"
2022

2123
"github.com/google/go-github/v38/github"
24+
"github.com/shurcooL/githubv4"
2225

2326
"github.com/ossf/scorecard/v4/clients"
2427
sce "github.com/ossf/scorecard/v4/errors"
28+
"github.com/ossf/scorecard/v4/log"
2529
)
2630

31+
//nolint:govet
32+
type checkRunsGraphqlData struct {
33+
Repository struct {
34+
Object struct {
35+
Commit struct {
36+
History struct {
37+
Nodes []struct {
38+
AssociatedPullRequests struct {
39+
Nodes []struct {
40+
HeadRefOid githubv4.String
41+
Commits struct {
42+
Nodes []struct {
43+
Commit struct {
44+
CheckSuites struct {
45+
Nodes []struct {
46+
App struct {
47+
Slug githubv4.String
48+
}
49+
Conclusion githubv4.CheckConclusionState
50+
Status githubv4.CheckStatusState
51+
}
52+
} `graphql:"checkSuites(first: $checksToAnalyze)"`
53+
}
54+
}
55+
} `graphql:"commits(last:1)"`
56+
}
57+
} `graphql:"associatedPullRequests(first: $pullRequestsToAnalyze)"`
58+
}
59+
} `graphql:"history(first: $commitsToAnalyze)"`
60+
} `graphql:"... on Commit"`
61+
} `graphql:"object(expression: $commitExpression)"`
62+
} `graphql:"repository(owner: $owner, name: $name)"`
63+
RateLimit struct {
64+
Cost *int
65+
}
66+
}
67+
68+
type checkRunsByRef = map[string][]clients.CheckRun
69+
2770
type checkrunsHandler struct {
28-
client *github.Client
29-
ctx context.Context
30-
repourl *repoURL
71+
client *github.Client
72+
graphClient *githubv4.Client
73+
ctx context.Context
74+
repourl *repoURL
75+
commitDepth int
76+
logger *log.Logger
77+
checkData *checkRunsGraphqlData
78+
checkRunsByRef checkRunsByRef
79+
setupOnce *sync.Once
80+
errSetup error
3181
}
3282

33-
func (handler *checkrunsHandler) init(ctx context.Context, repourl *repoURL) {
83+
func (handler *checkrunsHandler) init(ctx context.Context, repourl *repoURL, commitDepth int) {
3484
handler.ctx = ctx
3585
handler.repourl = repourl
86+
handler.commitDepth = commitDepth
87+
handler.logger = log.NewLogger(log.DefaultLevel)
88+
handler.checkData = new(checkRunsGraphqlData)
89+
handler.setupOnce = new(sync.Once)
90+
handler.checkRunsByRef = checkRunsByRef{}
91+
}
92+
93+
func (handler *checkrunsHandler) setup() error {
94+
handler.setupOnce.Do(func() {
95+
commitExpression := handler.repourl.commitExpression()
96+
vars := map[string]interface{}{
97+
"owner": githubv4.String(handler.repourl.owner),
98+
"name": githubv4.String(handler.repourl.repo),
99+
"pullRequestsToAnalyze": githubv4.Int(pullRequestsToAnalyze),
100+
"commitsToAnalyze": githubv4.Int(handler.commitDepth),
101+
"commitExpression": githubv4.String(commitExpression),
102+
"checksToAnalyze": githubv4.Int(checksToAnalyze),
103+
}
104+
// TODO(#2224):
105+
// sast and ci checks causes cache miss if commits dont match number of check runs.
106+
// paging for this needs to be implemented if using higher than 100 --number-of-commits
107+
if handler.commitDepth > 99 {
108+
vars["commitsToAnalyze"] = githubv4.Int(99)
109+
}
110+
if err := handler.graphClient.Query(handler.ctx, handler.checkData, vars); err != nil {
111+
// quit early without setting crsErrSetup for "Resource not accessible by integration" error
112+
// for whatever reason, this check doesn't work with a GITHUB_TOKEN, only a PAT
113+
if strings.Contains(err.Error(), "Resource not accessible by integration") {
114+
return
115+
}
116+
handler.errSetup = err
117+
return
118+
}
119+
handler.checkRunsByRef = parseCheckRuns(handler.checkData)
120+
})
121+
return handler.errSetup
36122
}
37123

38124
func (handler *checkrunsHandler) listCheckRunsForRef(ref string) ([]clients.CheckRun, error) {
125+
if err := handler.setup(); err != nil {
126+
return nil, fmt.Errorf("error during graphqlHandler.setupCheckRuns: %w", err)
127+
}
128+
if crs, ok := handler.checkRunsByRef[ref]; ok {
129+
return crs, nil
130+
}
131+
msg := fmt.Sprintf("listCheckRunsForRef cache miss: %s/%s:%s", handler.repourl.owner, handler.repourl.repo, ref)
132+
handler.logger.Info(msg)
133+
39134
checkRuns, _, err := handler.client.Checks.ListCheckRunsForRef(
40135
handler.ctx, handler.repourl.owner, handler.repourl.repo, ref, &github.ListCheckRunsOptions{})
41136
if err != nil {
42137
return nil, sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("ListCheckRunsForRef: %v", err))
43138
}
44-
return checkRunsFrom(checkRuns), nil
139+
handler.checkRunsByRef[ref] = checkRunsFrom(checkRuns)
140+
return handler.checkRunsByRef[ref], nil
141+
}
142+
143+
func parseCheckRuns(data *checkRunsGraphqlData) checkRunsByRef {
144+
checkCache := checkRunsByRef{}
145+
for _, commit := range data.Repository.Object.Commit.History.Nodes {
146+
for _, pr := range commit.AssociatedPullRequests.Nodes {
147+
var crs []clients.CheckRun
148+
for _, c := range pr.Commits.Nodes {
149+
for _, checkRun := range c.Commit.CheckSuites.Nodes {
150+
crs = append(crs, clients.CheckRun{
151+
// the REST API returns lowercase. the graphQL API returns upper
152+
Status: strings.ToLower(string(checkRun.Status)),
153+
Conclusion: strings.ToLower(string(checkRun.Conclusion)),
154+
App: clients.CheckRunApp{
155+
Slug: string(checkRun.App.Slug),
156+
},
157+
})
158+
}
159+
}
160+
headRef := string(pr.HeadRefOid)
161+
checkCache[headRef] = crs
162+
}
163+
}
164+
return checkCache
45165
}
46166

47167
func checkRunsFrom(data *github.ListCheckRunsResults) []clients.CheckRun {

‎clients/githubrepo/checkruns_e2e_test.go‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
// Copyright 2021 OpenSSF Scorecard Authors
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package githubrepo
16+
17+
import (
18+
"context"
19+
20+
. "github.com/onsi/ginkgo/v2"
21+
. "github.com/onsi/gomega"
22+
23+
"github.com/ossf/scorecard/v4/clients"
24+
)
25+
26+
var _ = Describe("E2E TEST: githubrepo.checkrunsHandler", func() {
27+
var checkrunshandler *checkrunsHandler
28+
29+
BeforeEach(func() {
30+
checkrunshandler = &checkrunsHandler{
31+
graphClient: graphClient,
32+
}
33+
})
34+
35+
// TODO: Add e2e tests for commit depth.
36+
37+
Context("E2E TEST: Validate query cost", func() {
38+
It("Should not have increased query cost", func() {
39+
repourl := &repoURL{
40+
owner: "ossf",
41+
repo: "scorecard",
42+
commitSHA: clients.HeadSHA,
43+
}
44+
checkrunshandler.init(context.Background(), repourl, 30)
45+
Expect(checkrunshandler.setup()).Should(BeNil())
46+
Expect(checkrunshandler.checkData).ShouldNot(BeNil())
47+
Expect(checkrunshandler.checkData.RateLimit.Cost).ShouldNot(BeNil())
48+
Expect(*checkrunshandler.checkData.RateLimit.Cost).Should(BeNumerically("<=", 1))
49+
})
50+
})
51+
})

‎clients/githubrepo/client.go‎

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ func (client *Client) InitRepo(inputRepo clients.Repo, commitSHA string, commitD
103103
client.workflows.init(client.ctx, client.repourl)
104104

105105
// Setup checkrunsHandler.
106-
client.checkruns.init(client.ctx, client.repourl)
106+
client.checkruns.init(client.ctx, client.repourl, commitDepth)
107107

108108
// Setup statusesHandler.
109109
client.statuses.init(client.ctx, client.repourl)
@@ -207,15 +207,7 @@ func (client *Client) ListSuccessfulWorkflowRuns(filename string) ([]clients.Wor
207207

208208
// ListCheckRunsForRef implements RepoClient.ListCheckRunsForRef.
209209
func (client *Client) ListCheckRunsForRef(ref string) ([]clients.CheckRun, error) {
210-
cachedCrs, err := client.graphClient.listCheckRunsForRef(ref)
211-
if errors.Is(err, errNotCached) {
212-
crs, err := client.checkruns.listCheckRunsForRef(ref)
213-
if err == nil {
214-
client.graphClient.cacheCheckRunsForRef(ref, crs)
215-
}
216-
return crs, err
217-
}
218-
return cachedCrs, err
210+
return client.checkruns.listCheckRunsForRef(ref)
219211
}
220212

221213
// ListStatuses implements RepoClient.ListStatuses.
@@ -276,7 +268,8 @@ func CreateGithubRepoClientWithTransport(ctx context.Context, rt http.RoundTripp
276268
client: client,
277269
},
278270
checkruns: &checkrunsHandler{
279-
client: client,
271+
client: client,
272+
graphClient: graphClient,
280273
},
281274
statuses: &statusesHandler{
282275
client: client,

0 commit comments

Comments
 (0)