Rewrite legacy SARIF categories for CQ - #3003
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR adds logic to rewrite legacy SARIF categories for Code Quality (CQ) analyses in Default Setup workflows. The change ensures backwards compatibility by mapping old language category identifiers to new ones that match CQ's expectations.
Key changes:
- Introduces a fixCodeQualityCategory function that maps legacy language categories to their modern equivalents
- Updates the quality analysis workflow to use the adjusted category when running interpret results
- Adds comprehensive test coverage for the category mapping functionality
Reviewed Changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/analyze.ts | Implements the fixCodeQualityCategory function and integrates it into the quality analysis workflow |
| src/analyze.test.ts | Adds unit tests covering various category mapping scenarios |
| lib/analyze.js | Compiled JavaScript version of the TypeScript changes |
| lib/analyze.test.js | Compiled JavaScript version of the test changes |
Sorry, something went wrong.
henrymercer
left a comment
There was a problem hiding this comment.
LGTM to unblock code quality — I don't love more hardcoding of information about languages, but this set shouldn't grow as we add new languages, and therefore there shouldn't be any additional work involved in adding a new language as a result of this PR.
Sorry, something went wrong.
Co-authored-by: Henry Mercer <henrymercer@github.com>
starcke
left a comment
There was a problem hiding this comment.
Seems pretty sensible and lightweight to me. But I'll defer to someone more knowledge on the action for the full review.
Sorry, something went wrong.
| return getPullRequestBranches() !== undefined; | ||
| } | ||
| // A mapping from old categories to new ones. | ||
| const qualityCategoryMapping = { |
There was a problem hiding this comment.
I think it is enough to convert the cases where result of the old mapping is different from the new mapping.
So I think it is enough to do the following three mappings:
cpp->c-cpp java->java-kotlin javascript->javascript-typescript
But it could be that I am missing something.
Sorry, something went wrong.
| // and before this workaround is removed. | ||
| if ( | ||
| category !== undefined && | ||
| (isDefaultSetup() || isInTestMode()) && |
There was a problem hiding this comment.
Could you explain the reasoning for enabling for all PR checks? If we need this to be enabled in a PR check, would an explicit environment variable work better to keep tests as close to prod as possible?
Sorry, something went wrong.
There was a problem hiding this comment.
It's impossible to test if isDefaultSetup() == true is a mandatory condition.
fixCodeQualityCategory is only called for Code Quality SARIF files / uploads, so it shouldn't affect any other PR checks.
I am open to adding a separate environment variable though.
Sorry, something went wrong.
There was a problem hiding this comment.
From a unit test, I think we could mock isDefaultSetup, or set the GITHUB_EVENT_NAME environment variable. From an integration test, I think a separate environment variable probably makes the most sense.
Sorry, something went wrong.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
In some Default Setup workflows, an old language category identifier may be given to the Action to ensure backwards compatibility for Code Scanning analyses. This doesn't apply to CQ analyses. However, since there's only one workflow for both, the best place where this can be addressed is in the Action. This PR modifies the category for CQ SARIF files to match CQ's expectations.
Merge / deployment checklist