✨ Commit depth feature

[latortuga71]

What kind of change does this PR introduce?

Feature to implement issue to add a flag for --commit-depth : #2224

• PR title follows the guidelines defined in our pull request documentation

What is the current behavior?

No --commit-depth flag exists to modify number of commits to use in checks.

What is the new behavior (if this is a feature change)?**

--commit-depth flag is added, which allows the number of commits to be used in checks to be increased/decreased.

• Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

#2224

Special notes for your reviewer

Does this PR introduce a user-facing change?

For user-facing changes, please add a concise, human-readable release note to
the release-note

(In particular, describe what changes users might need to make in their
application as a result of this pull request.)

Added --commit-depth optional flag to allow users to configure the amount of commits scorecard analyzes. 

[github-actions]

Integration tests success for
[903dbcc]
(https://github.com/ossf/scorecard/actions/runs/3338702742)

[github-actions]

Integration tests success for
[0f61203]
(https://github.com/ossf/scorecard/actions/runs/3338767103)

[laurentsimon]

Maybe add some unit tests.
Also think of adding the results in the final results (pkg/json_results.go) and pkg/raw_results...

[codecov]

Codecov Report

Merging #2407 (0f61203) into main (ef79b94) will decrease coverage by 0.56%.
The diff coverage is 0.00%.

❗ Current head 0f61203 differs from pull request most recent head 7d33043. Consider uploading reports for the commit 7d33043 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2407      +/-   ##
==========================================
- Coverage   40.97%   40.40%   -0.57%     
==========================================
  Files         113      112       -1     
  Lines        9270     8878     -392     
==========================================
- Hits         3798     3587     -211     
+ Misses       5196     5030     -166     
+ Partials      276      261      -15     

[latortuga71]

Maybe add some unit tests. Also think of adding the results in the final results (pkg/json_results.go) and pkg/raw_results...

@laurentsimon
for unit tests, where should they be placed? in graphql_e2e_test.go?
or a new file like graphql_test.go i dont see a file that contains tests for other functions like commitsFrom for example.

[laurentsimon]

Maybe add some unit tests. Also think of adding the results in the final results (pkg/json_results.go) and pkg/raw_results...

@laurentsimon for unit tests, where should they be placed? in graphql_e2e_test.go?

I think this file should be OK. You may need a demo repo that never changes so we can keep testing the same thing over time. Shall I create a repo under https://github.com/ossf-tests/? Maybe scorecard-check-ghclient-graphql-e2e?

[latortuga71]

Maybe add some unit tests. Also think of adding the results in the final results (pkg/json_results.go) and pkg/raw_results...

@laurentsimon for unit tests, where should they be placed? in graphql_e2e_test.go?

I think this file should be OK. You may need a demo repo that never changes so we can keep testing the same thing over time. Shall I create a repo under https://github.com/ossf-tests/? Maybe scorecard-check-ghclient-graphql-e2e?

Sure but before you do that, can you check the following changes i added to the e2e_test.go file. I added some tests for the populateCommits function but ive never used this testing framework so i could have made mistakes. I just added a new context and have my tests in there. Does that work?

var _ = Describe("E2E TEST: githubrepo.graphqlHandler", func() {
	var graphqlhandler *graphqlHandler

	BeforeEach(func() {
		graphqlhandler = &graphqlHandler{
			client: graphClient,
		}
	})
        // -> my test below
	Context("E2E TEST: Confirm Paging Commits Works", func() {
		It("Should only have 1 commit", func() {
			_repourl := &repoURL{
				owner:     "ossf",
				repo:      "scorecard",
				commitSHA: clients.HeadSHA,
			}
			_vars := map[string]interface{}{
				"owner":                  githubv4.String("ossf"),
				"name":                   githubv4.String("scorecard"),
				"pullRequestsToAnalyze":  githubv4.Int(1),
				"issuesToAnalyze":        githubv4.Int(30),
				"issueCommentsToAnalyze": githubv4.Int(30),
				"reviewsToAnalyze":       githubv4.Int(30),
				"labelsToAnalyze":        githubv4.Int(30),
				"commitsToAnalyze":       githubv4.Int(1),
				"commitExpression":       githubv4.String("heads/main"),
				"historyCursor":          (*githubv4.String)(nil),
			}
			_ctx := context.Background()
			_logger := log.NewLogger(log.DebugLevel)
			_rt := roundtripper.NewTransport(_ctx, _logger)
			_httpClient := &http.Client{
				Transport: _rt,
			}
			_graphClient := githubv4.NewClient(_httpClient)
			_handler := &graphqlHandler{
				client: _graphClient,
			}
			_handler.init(context.Background(), _repourl)
			commits, err := populateCommits(_handler, _vars)
			Expect(err).To(BeNil())
			Expect(len(commits)).Should(BeEquivalentTo(1))
		})
	})

	Context("E2E TEST: Validate query cost", func() {
		It("Should not have increased for HEAD query", func() {
			repourl := &repoURL{
				owner:     "ossf",
				repo:      "scorecard",
				commitSHA: clients.HeadSHA,
			}
			graphqlhandler.init(context.Background(), repourl)
			Expect(graphqlhandler.setup()).Should(BeNil())
			Expect(graphqlhandler.data).ShouldNot(BeNil())
			Expect(graphqlhandler.data.RateLimit.Cost).ShouldNot(BeNil())
			Expect(*graphqlhandler.data.RateLimit.Cost).Should(BeNumerically("<=", 1))
		})
		It("Should not have increased for commit query", func() {
			repourl := &repoURL{
				owner:     "ossf",
				repo:      "scorecard",
				commitSHA: "de5224bbc56eceb7a25aece55d2d53bbc561ed2d",
			}
			graphqlhandler.init(context.Background(), repourl)
			Expect(graphqlhandler.setup()).Should(BeNil())
			Expect(graphqlhandler.data).ShouldNot(BeNil())
			Expect(graphqlhandler.data.RateLimit.Cost).ShouldNot(BeNil())
			Expect(*graphqlhandler.data.RateLimit.Cost).Should(BeNumerically("<=", 1))
		})
		It("Should not have increased for check run query", func() {
			repourl := &repoURL{
				owner:     "ossf",
				repo:      "scorecard",
				commitSHA: clients.HeadSHA,
			}
			graphqlhandler.init(context.Background(), repourl)
			Expect(graphqlhandler.setupCheckRuns()).Should(BeNil())
			Expect(graphqlhandler.checkData).ShouldNot(BeNil())
			Expect(graphqlhandler.checkData.RateLimit.Cost).ShouldNot(BeNil())
			Expect(*graphqlhandler.checkData.RateLimit.Cost).Should(BeNumerically("<=", 1))
		})
	})
})

[laurentsimon]

LGTM at a high-level. Let's see if the tests pass :)
Fyi, not an expert either on the testing framework :)

[github-actions]

Integration tests success for
[97a77b5]
(https://github.com/ossf/scorecard/actions/runs/3415122634)

[latortuga71]

LGTM at a high-level. Let's see if the tests pass :) Fyi, not an expert either on the testing framework :)

Are there next steps ?
Also there is an issue with 3 checks specifically the SAST, CI-BestPracticesCheck and CI-Tests Checks. When the number of commits is set to greater than 100, those checks will print a cache miss informational error. due to the setupCheckRuns function being called. And that number not matching the number of commits saved by the client in the setup command.

func (handler *graphqlHandler) setupCheckRuns() error {
	handler.setupCheckRunsOnce.Do(func() {
		commitExpression := handler.commitExpression()
		vars := map[string]interface{}{
			"owner":                 githubv4.String(handler.repourl.owner),
			"name":                  githubv4.String(handler.repourl.repo),
			"pullRequestsToAnalyze": githubv4.Int(pullRequestsToAnalyze),
			"commitsToAnalyze":      githubv4.Int(CommitsToAnalyze),
			"commitExpression":      githubv4.String(commitExpression),
			"checksToAnalyze":       githubv4.Int(checksToAnalyze),
		}
		// TODO:
		// sast and ci checks causes cache miss if commits dont match number of check runs.
		// paging for this needs to be implemented if using higher than 100 --number-of-commits
		if CommitsToAnalyze > 99 { // <- paging not supported to maxed out to 99
			vars["commitsToAnalyze"] = githubv4.Int(99)
		}
		if err := handler.client.Query(handler.ctx, handler.checkData, vars); err != nil {
			// quit early without setting crsErrSetup for "Resource not accessible by integration" error
			// for whatever reason, this check doesn't work with a GITHUB_TOKEN, only a PAT
			if strings.Contains(err.Error(), "Resource not accessible by integration") {
				return
			}
			handler.errSetupCheckRuns = err
			return
		}
		handler.checkRuns = parseCheckRuns(handler.checkData)
	})
	return handler.errSetupCheckRuns
}

to fix this i think the graphql query for setupcheckruns would need to be rewritten to have paging. But currently it doesnt seem to be a big issue.

[azeemshaikh38]

Sorry about my late review, I was ooo. PR looks great, I have left some comments to be addressed. Once the comments are resolved will work on getting this merged.

Thanks a lot for working on this, this will be a super helpful feature!

[azeemshaikh38]

Are there next steps ?
Also there is an issue with 3 checks specifically the SAST, CI-BestPracticesCheck and CI-Tests Checks. When the number of commits is set to greater than 100, those checks will print a cache miss informational error. due to the setupCheckRuns function being called. And that number not matching the number of commits saved by the client in the setup command.

Thanks for pointing this out. Ok to fix this cache miss in a follow-up PR and submit this PR with its current functionality. My only ask would be to leave #2224 open so that we can track this work. Also, if you would like to work on this follow-up too let us know by commenting on #2224, else one of the maintainers or someone else can take this up.

[azeemshaikh38]

Looks good to merge.

[github-actions]

Integration tests success for
[70bd5c1]
(https://github.com/ossf/scorecard/actions/runs/3500637682)

[github-actions]

Integration tests success for
[32e9330]
(https://github.com/ossf/scorecard/actions/runs/3500691626)

[latortuga71]

Looks good to merge.

@azeemshaikh38 let me know if that is ok, all the tests are passing and seems to work fine, i added 0 in all cases where 30 was and used <= instead of ==

i thought i signed the commit, but i guess it didnt so it told me to rebase. that causes conflicts im guessing lmk what i need to do to fix any problems Thanks!

[latortuga71]

Are there next steps ?
Also there is an issue with 3 checks specifically the SAST, CI-BestPracticesCheck and CI-Tests Checks. When the number of commits is set to greater than 100, those checks will print a cache miss informational error. due to the setupCheckRuns function being called. And that number not matching the number of commits saved by the client in the setup command.

Thanks for pointing this out. Ok to fix this cache miss in a follow-up PR and submit this PR with its current functionality. My only ask would be to leave #2224 open so that we can track this work. Also, if you would like to work on this follow-up too let us know by commenting on #2224, else one of the maintainers or someone else can take this up.

ill probably take a shot a it. may need help will ask in the issue

[github-actions]

Integration tests success for
[32e9330]
(https://github.com/ossf/scorecard/actions/runs/3508958817)

[azeemshaikh38]

i thought i signed the commit, but i guess it didnt so it told me to rebase. that causes conflicts im guessing lmk what i need to do to fix any problems Thanks!

Thanks. We need to resolve the conflicts before merging. Could you please do that? I'm also looking into it on my side.

[latortuga71]

i thought i signed the commit, but i guess it didnt so it told me to rebase. that causes conflicts im guessing lmk what i need to do to fix any problems Thanks!

Thanks. We need to resolve the conflicts before merging. Could you please do that? I'm also looking into it on my side.

Just completed. lmk if there are any issues!

[github-actions]

Integration tests success for
[7d33043]
(https://github.com/ossf/scorecard/actions/runs/3524708764)

[azeemshaikh38]

i thought i signed the commit, but i guess it didnt so it told me to rebase. that causes conflicts im guessing lmk what i need to do to fix any problems Thanks!

Thanks. We need to resolve the conflicts before merging. Could you please do that? I'm also looking into it on my side.

Just completed. lmk if there are any issues!

Thanks a lot! Very much appreciate your effort on this PR :) Will work on getting this merged.

[latortuga71]

i thought i signed the commit, but i guess it didnt so it told me to rebase. that causes conflicts im guessing lmk what i need to do to fix any problems Thanks!

Thanks. We need to resolve the conflicts before merging. Could you please do that? I'm also looking into it on my side.

Just completed. lmk if there are any issues!

Thanks a lot! Very much appreciate your effort on this PR :) Will work on getting this merged.

No thank you guys for helping me out through this process, it was my first time contributing to any project and it was a great experience :)

[gal-legit]

FYI the change in pkg/scorecard.go was a breaking change, should've gone into a major semver