Improve logs for combined SARIF debug artifact #2762
Conversation
Don't start a "Uploading combined SARIF debug artifact" log group if we aren't going to do the upload.
There was a problem hiding this comment.
PR Overview
This PR improves logging for the combined SARIF debug artifact upload process by ensuring that a log group is only created when an upload is actually attempted. The changes remove the redundant withGroup wrapping in the analyze-action-post files and move the log grouping logic inside the uploadCombinedSarifArtifacts functions in both the src and lib locations.
- Removed withGroup import and call from analyze-action-post files.
- Added withGroup wrapping in uploadCombinedSarifArtifacts functions in both JavaScript and TypeScript implementations.
- Ensured that the log group is triggered only when the CODEQL_ACTION_DEBUG_COMBINED_SARIF environment variable is set.
Changes
| File | Description |
|---|---|
| src/analyze-action-post.ts | Removed withGroup usage to avoid creating an unnecessary log group. |
| lib/debug-artifacts.js | Added withGroup wrapping inside uploadCombinedSarifArtifacts to control logging. |
| src/debug-artifacts.ts | Added withGroup wrapping inside uploadCombinedSarifArtifacts to control logging. |
| lib/analyze-action-post.js | Removed the withGroup wrapping for consistency with the refactored logic. |
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
| .filter((f) => f.endsWith(".sarif")); | ||
| for (const sarifFile of sarifFiles) { | ||
| toUpload.push(path.resolve(baseTempDir, outputDir, sarifFile)); | ||
| await (0, logging_1.withGroup)("Uploading combined SARIF debug artifact", async () => { |
There was a problem hiding this comment.
Consider checking if there are artifacts to upload (for example, verifying that the 'toUpload' array is non-empty) before starting the log group, so that empty log groups are not created.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| logger.info( | ||
| "Uploading available combined SARIF files as Actions debugging artifact...", | ||
| ); | ||
| await withGroup("Uploading combined SARIF debug artifact", async () => { |
There was a problem hiding this comment.
Consider verifying that there are artifacts to upload prior to initiating the log group to fully align with the goal of avoiding empty log groups when no upload occurs.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
There was a problem hiding this comment.
Kind of a fair comment — we could log this only after checking if toUpload isn't empty in ~line 69.
There was a problem hiding this comment.
With the way it's currently set up I believe we'll see a log warning if toUpload is empty, as we expect it to contain at least something. There are definitely further improvements we could make to the code but I think I'll leave it as is for now.
| logger.info( | ||
| "Uploading available combined SARIF files as Actions debugging artifact...", | ||
| ); | ||
| await withGroup("Uploading combined SARIF debug artifact", async () => { |
There was a problem hiding this comment.
Kind of a fair comment — we could log this only after checking if toUpload isn't empty in ~line 69.
Avoid starting a "Uploading combined SARIF debug artifact" log group if we aren't going to do the upload.
Merge / deployment checklist