FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Implement dumping of processed sarif file by redsun82 · Pull Request #3089 · github/codeql-action · GitHub

/ codeql-action Public

Implement dumping of processed sarif file - #3089

Closed
redsun82 wants to merge 4 commits into
mainfrom
redsun82/dump-sarif
ConversationCommits (4)ChecksFiles changed
Closed

Implement dumping of processed sarif file#3089
redsun82 wants to merge 4 commits into
mainfrom
redsun82/dump-sarif

Conversation

redsun82 commented Sep 9, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

This introduces a CODEQL_ACTION_SARIF_DUMP_DIR environment variable for internal use that causes the processed SARIF file that codeql-action would upload to Code Scanning or Code Quality to be dumped locally. Crucially, this will also happen on analyze with upload: never.

This will allow us to test what is actually sent for upload after the SARIF processing that upload-lib.ts does (validating, merging, filtering, adding fingerprints etc.).

This SARIF file will be dumped in the directory specified by the environment variable, with upload.sarif or upload.quality.sarif as name. There is a risk of naming conflict if an output SARIF file has that name (which depends on the category), but I'm deeming it low risk (and not happening for our testing usage, where the file names are language names).

Risk assessment

For internal use only. Please select the risk level of this change:

  • Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Merge / deployment checklist

  • Run analysis with this on a test repository
  • Confirm this change is backwards compatible with existing workflows.
  • Consider adding a changelog entry for this change.
  • Confirm the readme and docs have been updated if necessary.

redsun82 force-pushed the redsun82/dump-sarif branch 2 times, most recently from 2c3bb3f to a9b127f Compare September 9, 2025 09:16
esbena reviewed Sep 9, 2025
View reviewed changes
Comment thread src/environment.ts Outdated
mbg reviewed Sep 9, 2025
View reviewed changes

mbg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I have had an initial look over this. It looks like a more involved change than I had hoped -- I was hoping we could just check the environment variable once we have the post-processed SARIF, just before uploading it, and then dump it to a file/folder.

This part of the Action is evolving quite a bit at the moment due to the work on Code Quality and I was planning to make a bunch of changes here in the next few days. They probably wouldn't interact nicely with these changes, though.

Comment thread src/analyses.ts Outdated

redsun82 commented Sep 9, 2025

Copy link
Copy Markdown
Contributor Author

I have had an initial look over this. It looks like a more involved change than I had hoped -- I was hoping we could just check the environment variable once we have the post-processed SARIF, just before uploading it, and then dump it to a file/folder.

This part of the Action is evolving quite a bit at the moment due to the work on Code Quality and I was planning to make a bunch of changes here in the next few days. They probably wouldn't interact nicely with these changes, though.

I had hoped that as well, but:

  • we want to do the processing and dump the sarif even if upload: false, so we need a way to arrive at the dump point but skip the uploading
  • another unfortunately subtle point, is where to dump this file. At the moment we would do the dumping in the old version of the code we would only have a bunch of input sarif files. By unifying the access to the uploading to only one function with one parameter (so avoiding the uploadFiles/uploadSpecifiedFiles duality), we can decide a bit more clearly where to put the new file (in the directory, if the input is a directory, or as a sibling of the input, if it is a file). I could have turned CODEQL_ACTION_DUMP_SARIF into the desired output path, but decided against that because of the new Code Scanning/Code Quality duality (we would have needed two separate env variables...)

redsun82 commented Sep 9, 2025

Copy link
Copy Markdown
Contributor Author
  • but decided against that because of the new Code Scanning/Code Quality duality (we would have needed two separate env variables...)

but in light of the other comments here, it makes sense to make that point to a directory instead

esbena commented Sep 9, 2025

Copy link
Copy Markdown
Contributor

but decided against that because of the new Code Scanning/Code Quality duality (we would have needed two separate env variables...)

We could still have code-scanning/code-quality folders inside our desired output dir. It's still a smaller dependency on internal path choices.

redsun82 commented Sep 9, 2025

Copy link
Copy Markdown
Contributor Author

but decided against that because of the new Code Scanning/Code Quality duality (we would have needed two separate env variables...)

We could still have code-scanning/code-quality folders inside our desired output dir. It's still a smaller dependency on internal path choices.

we already have the .sarif / .quality.sarif discriminant for that. Pointing the env var to a directory is a good choice I think

redsun82 force-pushed the redsun82/dump-sarif branch from a9b127f to fea2428 Compare September 9, 2025 13:07
Introduce CODEQL_ACTION_SARIF_DUMP_DIR
a7fb336 
Setting it will cause the SARIF files that would be uploaded to be
dumped to the specified directory as `upload.sarif` or
`upload.quality.sarif`. Crucially, this happens even if uploads are
disabled, which is useful for testing.
redsun82 force-pushed the redsun82/dump-sarif branch from fea2428 to a7fb336 Compare September 9, 2025 13:17

redsun82 commented Sep 9, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@mbg I tried a more surgical approach now (I recommend hiding whitespace in the diff view). I did like the fact that I had only one function in the lib processing the sarifs (as opposed to uploadFiles/uploadSpecifiedFiles), but I can understand it's more pragmatic to limit the changes done in the area, especially as it is a moving target with the Code Quality work.

I still had to change the analyze action as well, because of the requirement to dump the SARIF file if requested even if upload: false.

redsun82 marked this pull request as ready for review September 9, 2025 13:30
redsun82 requested a review from a team as a code owner September 9, 2025 13:30
Copilot AI review requested due to automatic review settings September 9, 2025 13:30
Copilot AI reviewed Sep 9, 2025
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Pull Request Overview

This PR implements a new internal testing feature that allows dumping of processed SARIF files to a local directory via the CODEQL_ACTION_SARIF_DUMP_DIR environment variable. The feature works regardless of whether upload is enabled or disabled, making it useful for testing what gets sent to Code Scanning or Code Quality after SARIF processing.

Key changes:

  • Added CODEQL_ACTION_SARIF_DUMP_DIR environment variable support for dumping processed SARIF files
  • Refactored upload functions to conditionally process and upload based on upload kind and dump directory settings
  • Modified analyze action to use the new conditional upload mechanism

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

File Description
src/upload-lib.ts Refactored upload functions to support conditional processing and added SARIF dumping functionality
src/environment.ts Added new environment variable for SARIF dump directory
src/analyze-action.ts Updated to use conditional upload functions and handle optional upload results

Comment thread src/upload-lib.ts
Comment thread src/upload-lib.ts
mbg reviewed Sep 9, 2025
View reviewed changes

mbg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for simplifying this, it's really appreciated!

I think there are fundamentally two things going on in this PR:

  • Dumping the post-processed SARIF based on the value of the environment variable
  • Refactoring upload-lib to support this when the upload input is not always

Perhaps it would be sensible to split these two aspects up into separate changes:

  • One PR which implements the environment variable controlling where the post-processed SARIF is written to, but which will only do this just before uploading (i.e. the simple case we hoped for). Hopefully that is an easy change that we can extract from this PR and quickly review and merge.
  • Then we can think about what the best way to refactor upload-lib to support this in the case where we wouldn't be uploading the SARIF is. I think that's a change that we want to think about more and, possibly, at the same time as changes I should make in the context of the Code Quality work.

redsun82 commented Sep 10, 2025

Copy link
Copy Markdown
Contributor Author
  • One PR which implements the environment variable controlling where the post-processed SARIF is written to, but which will only do this just before uploading (i.e. the simple case we hoped for). Hopefully that is an easy change that we can extract from this PR and quickly review and merge.

Fair enough, here's the PR 🙂 :

redsun82 commented Sep 12, 2025

Copy link
Copy Markdown
Contributor Author

@mbg should we maybe chat about this again?

I thought that maybe we could add a special dry-run value to the upload option to drive this, independently of the internal dump variable. Passing dry-run would mean the same thing as always, save for skipping the actual final download.

redsun82 mentioned this pull request Oct 6, 2025
Merged

redsun82 commented Oct 14, 2025

Copy link
Copy Markdown
Contributor Author

Obsoleted by:

redsun82 closed this Oct 14, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mbg mbg left review comments
esbena esbena left review comments
Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL