-
Notifications
You must be signed in to change notification settings - Fork 453
Support SHA-256 Git object hashes (64-char OIDs) #3872
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -166,8 +166,8 @@ export const determineBaseBranchHeadCommitOid = async function ( | |||||
| // Let's confirm our assumptions: We had a merge commit and the parsed parent data looks correct | ||||||
| if ( | ||||||
| commitOid === mergeSha && | ||||||
| headOid.length === 40 && | ||||||
| baseOid.length === 40 | ||||||
| (headOid.length === 40 || headOid.length === 64) && | ||||||
| (baseOid.length === 40 || baseOid.length === 64) | ||||||
| ) { | ||||||
| return baseOid; | ||||||
| } | ||||||
|
|
@@ -296,7 +296,7 @@ export const getFileOidsUnderPath = async function ( | |||||
| // 100644 4c51bc1d9e86cd86e01b0f340cb8ce095c33b283 0\tsrc/git-utils.test.ts | ||||||
| // 100644 6b792ea543ce75d7a8a03df591e3c85311ecb64f 0\tsrc/git-utils.ts | ||||||
| // The fields are: <mode> <oid> <stage>\t<path> | ||||||
| const regex = /^[0-9]+ ([0-9a-f]{40}) [0-9]+\t(.+)$/; | ||||||
| const regex = /^[0-9]+ ([0-9a-f]{40,64}) [0-9]+\t(.+)$/; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we usually prefer a regex that only allows 40 or 64 character SHAs, something like this:
Suggested change
|
||||||
| for (const line of stdout.split("\n")) { | ||||||
| if (line) { | ||||||
| const match = line.match(regex); | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -160,6 +160,9 @@ export const DEFAULT_ACTIONS_VARS = { | |
| RUNNER_OS: "Linux", | ||
| } as const satisfies Record<string, string>; | ||
|
|
||
| /** A 64-character SHA-256 Git OID for use in SHA-256 repository test scenarios. */ | ||
| export const SHA256_GITHUB_SHA = "0".repeat(64); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: This is only used in Additionally, the name is misleading. There's nothing GitHub-specific about a SHA256 hash. |
||
|
|
||
| // Sets environment variables that make using some libraries designed for | ||
| // use only on actions safe to use outside of actions. | ||
| export function setupActionsVars( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ import * as api from "./api-client"; | |
| import * as diffUtils from "./diff-informed-analysis-utils"; | ||
| import { getRunnerLogger, Logger } from "./logging"; | ||
| import * as sarif from "./sarif"; | ||
| import { setupTests } from "./testing-utils"; | ||
| import { setupTests, SHA256_GITHUB_SHA } from "./testing-utils"; | ||
| import * as uploadLib from "./upload-lib"; | ||
| import { UploadPayload } from "./upload-lib/types"; | ||
| import { GitHubVariant, initializeEnvironment, withTmpDir } from "./util"; | ||
|
|
@@ -110,6 +110,35 @@ test.serial( | |
| }, | ||
| ); | ||
|
|
||
| test.serial( | ||
| "validate correct payload used for PR merge commit with SHA-256 OIDs", | ||
| async (t) => { | ||
| process.env["GITHUB_EVENT_NAME"] = "pull_request"; | ||
| process.env["GITHUB_SHA"] = SHA256_GITHUB_SHA; | ||
| process.env["GITHUB_BASE_REF"] = "master"; | ||
| process.env["GITHUB_EVENT_PATH"] = | ||
| `${__dirname}/../src/testdata/pull_request.json`; | ||
|
|
||
| const sha256MergeBase = "b".repeat(64); | ||
| const prMergePayload: any = uploadLib.buildPayload( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The addition of this test doesn't make sense: |
||
| SHA256_GITHUB_SHA, | ||
| "refs/pull/123/merge", | ||
| "key", | ||
| undefined, | ||
| "", | ||
| 1234, | ||
| 1, | ||
| "/opt/src", | ||
| undefined, | ||
| ["CodeQL", "eslint"], | ||
| sha256MergeBase, | ||
| ); | ||
| // Uploads for a merge commit use the merge base (SHA-256) | ||
| t.deepEqual(prMergePayload.base_ref, "refs/heads/master"); | ||
| t.deepEqual(prMergePayload.base_sha, sha256MergeBase); | ||
| }, | ||
| ); | ||
|
|
||
| test.serial("finding SARIF files", async (t) => { | ||
| await withTmpDir(async (tmpDir) => { | ||
| // include a couple of sarif files | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getRef()doesn't perform any validation, so the four new tests forgetRef()here pass without the changes in this PR.