-
Notifications
You must be signed in to change notification settings - Fork 379
Allow Code Quality only analysis #3064
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
e28ecb0
to
603a572
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.
Pull Request Overview
This PR adds support for Code Quality only analysis by refactoring how analysis configurations are handled and enabling standalone Code Quality database initialization. Previously, Code Quality could only run as an "add-on" to Code Scanning analysis.
Key changes:
- Consolidates analysis configuration constants into a new
analyses.ts
module - Enables Code Quality to run independently without Code Scanning
- Deprecates the
quality-queries
input in favor of theanalysis-kinds
input - Adds helper utilities for working with different analysis configurations
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
src/analyses.ts | New module defining analysis configurations and constants |
src/config-utils.ts | Major logic changes to support standalone Code Quality analysis initialization |
src/util.ts | Added isDefined utility function |
src/start-proxy.ts | Refactored to use centralized isDefined function |
src/init-action.ts | Added deprecation warning for quality-queries input |
src/analyze.ts | Updated query execution logic to handle both standalone and add-on Code Quality |
src/upload-*.ts | Updated to use new analysis configuration objects |
pr-checks/checks/*.yml | Updated test configurations to use analysis-kinds instead of quality-queries |
init/action.yml | Marked quality-queries input as deprecated |
config.originalUserInput["disable-default-queries"] = true; | ||
config.originalUserInput.queries = queries; | ||
config.originalUserInput["query-filters"] = []; |
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.
Direct mutation of config.originalUserInput
is inconsistent with the pattern used elsewhere in the codebase. Consider using cloneObject
or a similar approach to avoid mutating the original input, as this could cause unexpected side effects if the config object is used elsewhere.
config.originalUserInput["disable-default-queries"] = true; | |
config.originalUserInput.queries = queries; | |
config.originalUserInput["query-filters"] = []; | |
const originalUserInputClone = cloneObject(config.originalUserInput); | |
originalUserInputClone["disable-default-queries"] = true; | |
originalUserInputClone.queries = queries; | |
originalUserInputClone["query-filters"] = []; | |
config.originalUserInput = originalUserInputClone; |
Copilot uses AI. Check for mistakes.
5b9c46f
to
a99145b
Compare
|
||
// If Code Scanning analysis is disabled, then we initialise the database for Code Quality. | ||
// That entails disabling the default queries and only running quality queries. | ||
if (!isCodeScanningEnabled(config)) { |
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 like that we're looking at things that won't work proactively, but would consider changing the condition here. It works since the analysis kinds are only code scanning and code quality, but I think in general the logic we want is more like config.analysisKinds.length === 1 && isCodeQualityEnabled(config)
.
logger.warning( | ||
"Query customizations will be ignored, because only `code-quality` analysis is enabled.", | ||
); |
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.
Should we actually error in these cases? It's easy to miss Actions warnings, and users could think they're analysing something that doesn't actually work. That also avoids us the slight confusion of then having to override config.originalUserInput
.
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 wasn't sure about this, but decided to go with a warning because in Default Setup we provide inputs to queries
and config
. We can probably change that there for when Code Quality is enabled, but Code Scanning isn't, but I didn't want to make this an error just yet in case it causes us problems during the transition there.
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 appreciate wanting to make the transition smooth, but I have some concerns about modifying config.originalUserInput
, particularly since we use this to generate telemetry.
My understanding is today we have analyses with a quality-queries
input and without an analysis-kinds
input. This means we'll run code scanning, and therefore shouldn't hit this case. Then the transition can move to an explicit analysis-kinds
input with the config
and other customization inputs removed.
Is there anything I'm missing here?
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 there are two separate points here:
- Whether we can swap over from using
quality-queries
toanalysis-kinds
and remove thequeries
input in CQ-only mode in one go. (Probably "yes" and this being a warning is just overly defensive.) - That modifying
config.originalUserInput
is an issue for telemetry.
Regarding 2., I agree that this isn't a very nice approach. I'll have a look at refactoring this tomorrow so that originalUserInput
can remain as-is and we set up the configuration for a CQ-only database differently.
@@ -943,7 +913,7 @@ function handleProcessingResultForUnsuccessfulExecution( | |||
|
|||
export function validateUniqueCategory( | |||
sarif: SarifFile, | |||
sentinelPrefix: string = CodeScanningTarget.sentinelPrefix, | |||
sentinelPrefix: string = analyses.CodeScanning.sentinelPrefix, |
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.
Do we need a default value here? Removing it might decrease the potential for errors.
* | ||
* @returns Returns `CodeScanning` if `AnalysisKind.CodeScanning` is enabled; otherwise `CodeQuality`. | ||
*/ | ||
export function getDbAnalysisConfig(config: Config): AnalysisConfig { |
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.
This seems like a detail of how saving SARIF results currently works, in particular the fact that when both modes are on, we currently produce a code scanning file, then filter that to produce a code quality file. I don't really think it's a property of the database, since we'll ask the database to analyse both code scanning and code quality results in that situation.
I'd consider removing this in favour of some more explicit logic concerning the SARIF extension in analyze
, unless there are more situations where this notion of "primary consumer" matters.
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.
This seems like a detail of how saving SARIF results currently works, in particular the fact that when both modes are on, we currently produce a code scanning file, then filter that to produce a code quality file. I don't really think it's a property of the database, since we'll ask the database to analyse both code scanning and code quality results in that situation.
I don't think this is true, since in CQ-only mode the database is specifically initialised with code-quality
queries. What getDbAnalysisConfig
tells us is whether the database is initialised with a Code Scanning configuration (in CS only or CS+CQ mode) or a Code Quality configuration (in CQ only mode).
It's true though that this is currently only relevant when we are generating the "primary" SARIF file.
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 don't think this is true, since in CQ-only mode the database is specifically initialised with
code-quality
queries. WhatgetDbAnalysisConfig
tells us is whether the database is initialised with a Code Scanning configuration (in CS only or CS+CQ mode) or a Code Quality configuration (in CQ only mode).
I'm unsure how much meaning this has in practice. For instance, if I really wanted to, I could create a code scanning configuration that runs the code quality queries only by disabling the default queries and running a custom query pack. I'd expect the database to be identical to a database with analysis-kinds: code-quality
. The only difference I can see is the name of the SARIF file and the upload API.
This is a first stab at enabling Code Quality analysis to be able to run on its own, while also maintaining the ability for it to run in addition to a Code Scanning analysis.
Fundamentally, this means that there are two separate ways in which Code Quality analysis can run:
code-quality
suite into therun-queries
call. The Code Quality SARIF is then generated by an extra call tointerpret-results
.code-quality
queries.43d9bc8 and 5d95d46 implement the bulk of the changes needed for the "add-on" implementation that result from needing to check whether Code Quality is enabled in addition to or instead of Code Scanning.
1746aed implements the bulk of the changes needed for the "standalone" implementation. Concretely, this checks whether Code Quality is enabled on its own, and then mutates the configuration as necessary.
Probably best reviewed commit-by-commit.
Risk assessment
For internal use only. Please select the risk level of this change:
Merge / deployment checklist