Perform SARIF post-processing independently of upload #3222
Conversation
d50f5d8 to
a457f8e
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the SARIF upload process to separate post-processing from uploading, allowing processed SARIF files to be written to disk independently of whether they are uploaded. The key change is that SARIF files are now always post-processed regardless of the upload input value, with the actual upload being conditional on uploadKind === "always". A new post-process-output input enables users to specify a directory for writing processed SARIF files.
Key Changes
- Extracted post-processing logic from
uploadSpecifiedFilesinto separatepostProcessSarifFilesanduploadProcessedFilesfunctions - Modified
uploadSarifto conditionally upload based onuploadKindparameter while always performing post-processing - Added new
post-process-outputinput to the analyze action for specifying where to write processed SARIF files
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/upload-sarif.ts | Main logic change: splits processing and uploading, adds conditional upload based on uploadKind |
| src/upload-lib.ts | Refactors uploadSpecifiedFiles into postProcessSarifFiles, writeProcessedFiles, and uploadProcessedFiles |
| src/upload-sarif.test.ts | Updates tests to mock new function signatures and adds tests for new behavior |
| src/analyze-action.ts | Updates to pass uploadKind and post-process-output parameters to uploadSarif |
| src/upload-sarif-action.ts | Updates to pass uploadKind parameter (hardcoded to "always") |
| analyze/action.yml | Adds documentation for new post-process-output input parameter |
| pr-checks/checks/quality-queries.yml | Adds test case for the new post-process-output functionality |
| lib/*.js | Generated JavaScript files mirroring TypeScript changes |
There was a problem hiding this comment.
Thanks for splitting into individual commits.
I only have some wording comments, none are hard blockers.
analyze/action.yml
Outdated
| description: Whether to upload the resulting CodeQL database | ||
| required: false | ||
| default: "true" | ||
| post-process-output: |
There was a problem hiding this comment.
Is the order of these input declarations chosen strongly based on discoverability for the user? If not, then I'd like this to be right next to the output input declared near the top in the name of coherence.
Second, and I'm sorry if this naming has been discussed at length in related PRs earlier: post-process-output reads like a boolean toggle, not a path. post-processed-output sounds more like a path. Additionally we always do the post-processing regardless of wether we save them to disk..
There was a problem hiding this comment.
post-processed-outputsounds more like a path.
I agree this would be a better name.
There was a problem hiding this comment.
Second, and I'm sorry if this naming has been discussed at length in related PRs earlier:
post-process-outputreads like a boolean toggle, not a path. post-processed-output sounds more like a path. Additionally we always do the post-processing regardless of wether we save them to disk..
Ironically, I think this kind of bad naming is actually something I complained about on @redsun82's PR previously, which he then changed to something better. I agree that this isn't a good name and have changed it to processed-sarif-path for now. That makes it consistent with other path inputs (other than output here). I don't think think that post-processed-output is really any better than post-process-output.
Is the order of these input declarations chosen strongly based on discoverability for the user? If not, then I'd like this to be right next to the output input declared near the top in the name of coherence.
Generally, I think the order is roughly based on how relevant the input is to a user. (See e.g. that we have the ones that are automatically populated or used internally only at the bottom.)
There was a problem hiding this comment.
Given the classification as high risk, have you considered manually comparing CodeQL and post-processed SARIF output before/after this PR to make sure there are no changes?
analyze/action.yml
Outdated
| description: Whether to upload the resulting CodeQL database | ||
| required: false | ||
| default: "true" | ||
| post-process-output: |
There was a problem hiding this comment.
post-processed-outputsounds more like a path.
I agree this would be a better name.
a457f8e to
8ff870a
Compare
This PR is currently based on #3206, which should be merged first.
Conceptually, this PR follows on from #3097 to allow processed SARIF files to be written to disk, if required.
This is accomplished by:
uploadSpecifiedFilesinto separate functions.uploadSpecifiedFilesinuploadSarifwith calls to the newpostProcessSarifFilesanduploadProcessedFiles.UploadKindcheck intouploadSarifand making the call touploadProcessedFilesconditional on it. Important: This is a change in behaviour, which means that we now always post-process SARIF files, even ifuploadis notalways.post-process-outputinput to theanalyzeaction which, if set, causeswriteProcessedFilesto write the processed SARIF files to disk between the calls topostProcessSarifFilesanduploadProcessedFiles.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
analysis-kinds: code-scanning).analysis-kinds: code-quality).upload-sarif).How did/will you validate this change?
.test.tsfiles).pr-checks).If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Merge / deployment checklist