Skip to content
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

Add commit sha to database upload #1922

Merged
merged 5 commits into from
Oct 12, 2023

Conversation

norascheuch
Copy link
Contributor

@norascheuch norascheuch commented Oct 5, 2023

Merge / deployment checklist

This PR adds the commit_oid to the database upload, so that users know - when using the database later - which version of the code it is based on.

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@norascheuch norascheuch requested a review from a team as a code owner October 5, 2023 09:52
@@ -44,6 +44,9 @@ export async function uploadDatabases(
const bundledDb = await bundleDb(config, language, codeql, language);
const bundledDbSize = fs.statSync(bundledDb).size;
const bundledDbReadStream = fs.createReadStream(bundledDb);
const commitOid = actionsUtil.getCommitOid(
actionsUtil.getRequiredInput("checkout_path"),
);
try {
await client.request(
`POST https://uploads.github.com/repos/:owner/:repo/code-scanning/codeql/databases/:language?name=:name`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`POST https://uploads.github.com/repos/:owner/:repo/code-scanning/codeql/databases/:language?name=:name`,
`POST https://uploads.github.com/repos/:owner/:repo/code-scanning/codeql/databases/:language?name=:name&commit_oid=:commit_oid`,

Oops, I missed we'll need to include commit_oid here so that it knows how to include it when making the HTTP request. The body of the request is just the database contents, so the commit oid needs to be included as a URL param.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol makes sense 🤦‍♀️

@norascheuch norascheuch force-pushed the nora/add-commit-sha-to-database-upload branch from 1677e55 to aed2e7f Compare October 6, 2023 13:16
aeisenberg
aeisenberg previously approved these changes Oct 6, 2023
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@norascheuch norascheuch force-pushed the nora/add-commit-sha-to-database-upload branch from 7273335 to e62d946 Compare October 10, 2023 08:48
@robertbrignull
Copy link
Contributor

Code LGTM, but this shouldn't be merged until we've merged the API change and then tested this PR.

@norascheuch norascheuch merged commit b584cf8 into main Oct 12, 2023
372 checks passed
@norascheuch norascheuch deleted the nora/add-commit-sha-to-database-upload branch October 12, 2023 15:50
@github-actions github-actions bot mentioned this pull request Oct 13, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants