-
Notifications
You must be signed in to change notification settings - Fork 376
Rewrite legacy SARIF categories for CQ #3003
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
7a5b389
to
c649cc1
Compare
c649cc1
to
7277034
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Co-authored-by: Henry Mercer <[email protected]>
b7b000a
to
9fb8f2d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems pretty sensible and lightweight to me. But I'll defer to someone more knowledge on the action for the full review.
@@ -392,4 +393,35 @@ function getPullRequestBranches() { | |||
function isAnalyzingPullRequest() { | |||
return getPullRequestBranches() !== undefined; | |||
} | |||
// A mapping from old categories to new ones. | |||
const qualityCategoryMapping = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/actions-util.ts
Outdated
// and before this workaround is removed. | ||
if ( | ||
category !== undefined && | ||
(isDefaultSetup() || isInTestMode()) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
8fb489c
to
1b76c0b
Compare
Co-authored-by: Copilot <[email protected]>
675cd6f
to
2d08245
Compare
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