Fix upload-sarif not uploading non-.sarif files - #3157
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the upload-sarif action to extract the core upload logic into a reusable module and fixes an issue where non-.sarif files were not being uploaded properly.
- Extracts upload logic from upload-sarif-action.ts into a dedicated upload-sarif.ts module for better separation of concerns
- Updates the Code Scanning predicate to upload files that are not intended for Code Quality (including non-.sarif files)
- Adds comprehensive test coverage for the new module with various file types and scenarios
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/upload-sarif.ts | New module containing extracted upload logic with updated file filtering |
| src/upload-sarif.test.ts | New test file providing comprehensive coverage for the upload module |
| src/upload-sarif-action.ts | Refactored to use the new upload module with simplified logic |
| lib/upload-sarif-action.js | Generated JavaScript code updated to match TypeScript changes |
Sorry, something went wrong.
esbena
left a comment
There was a problem hiding this comment.
This LGTM semantically, but I worry about erosion that the new predicate juggling introduces.
Sorry, something went wrong.
| analyses.CodeScanning, | ||
| // Upload anything to Code Scanning that is not intended for Code Quality. | ||
| (name) => !analyses.CodeQuality.sarifPredicate(name), |
There was a problem hiding this comment.
I'm concerned about the addition of this argument (also, there's missing jsdoc for the new parameter).
Inside findAndUpload it is not clear that you should no longer use analysis.sarifPredicate, and that it only is subtly wrong to do so.
I'd prefer that we change the config object to have a different sarifPredicate:
| analyses.CodeScanning, | |
| // Upload anything to Code Scanning that is not intended for Code Quality. | |
| (name) => !analyses.CodeQuality.sarifPredicate(name), | |
| { | |
| ... analyses.CodeScanning, | |
| // Upload anything to Code Scanning that is not intended for Code Quality. | |
| sarifPredicate: (name) => !analyses.CodeQuality.sarifPredicate(name) | |
| }, |
Sorry, something went wrong.
There was a problem hiding this comment.
I'd prefer that we change the config object to have a different sarifPredicate:
I agree. I wasn't particularly happy with this extra parameter when I implemented it last night either, but it was the easiest fix. Having thought about it more over night, I reached the same conclusion that a separate configuration where sarifPredicate is overriden is better.
But there is a further issue, which you haven't flagged up as such. Using !analyses.CodeQuality.sarifPredicate(name) as sarifPredicate for the call to upload_lib.findSarifFilesInDir in findAndUpload also subtly changes the behaviour to what we had before: it now uploads all files (other than .quality.sarif) from the directory, rather than just .sarif ones (which was the previous behaviour, but isn't adequate anymore because we need to filter out the .quality.sarif ones). I think that may have been my original reasoning for the extra sarifPredicate parameter so that I could use one for the file case and another for the directory case (but then forgot and used it for both).
I'll think about a better solution.
Sorry, something went wrong.
| if (pathStats.isDirectory()) { | ||
| sarifFiles = upload_lib.findSarifFilesInDir(sarifPath, sarifPredicate); | ||
| } else if (pathStats.isFile() && sarifPredicate(sarifPath)) { | ||
| sarifFiles = [sarifPath]; | ||
| } else { |
There was a problem hiding this comment.
This is subtly different from the sibling file finding logic in upload-lib.ts
codeql-action/src/upload-lib.ts
Lines 448 to 459 in 245e2e5
Concretely, the latter's lack of sarifPredicate(sarifPath) in the else is intentional as per your .json comment, but there's no jsdoc to document that the isSarif is unused when single files are provided..
Sorry, something went wrong.
There was a problem hiding this comment.
I agree that we should unify things and it's been on my radar to do that / get rid of uploadFiles since we worked on #3123. This probably is as good an opportunity to do that, in combination with coming up for a good solution for the other issue here.
Sorry, something went wrong.
Sorry, something went wrong.
Fixes #3156.
In #3123, we inadvertently introduced a regression by using the same logic for uploading SARIF files as in the analyze Action: if a file ends in .quality.sarif, we upload it to Code Quality, and if it ends in .sarif (but not .quality.sarif), we upload it to Code Scanning.
However, that didn't account for the possibility that sarif-path is set to a file or directory containing files with other extensions (such as .json).
This PR restores this behaviour by using !analyses.CodeQuality.sarifPredicate(name) as the file predicate for Code Scanning in the upload-sarif Action.
I have also refactored upload-sarif-action slightly to move some parts to a new upload-sarif module that I have added some basic tests for.
Risk assessment
For internal use only. Please select the risk level of this change:
Merge / deployment checklist