Allow Code Quality only analysis #3064
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
mbg
force-pushed
the
mbg/cq/allow-cq-only-analysis
branch
4 times, most recently
from
e28ecb0 to
603a572
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
mbg
force-pushed
the
mbg/cq/allow-cq-only-analysis
branch
from
5b9c46f to
a99145b
Compare
src/config-utils.ts
Outdated
| logger.warning( | ||
| "Query customizations will be ignored, because only `code-quality` analysis is enabled.", | ||
| ); |
There was a problem hiding this comment.
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.
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.
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.
I think there are two separate points here:
- Whether we can swap over from using
quality-queriestoanalysis-kindsand remove thequeriesinput in CQ-only mode in one go. (Probably "yes" and this being a warning is just overly defensive.) - That modifying
config.originalUserInputis 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.
src/config-utils.ts
Outdated
| * | ||
| * @returns Returns `CodeScanning` if `AnalysisKind.CodeScanning` is enabled; otherwise `CodeQuality`. | ||
| */ | ||
| export function getDbAnalysisConfig(config: Config): AnalysisConfig { |
There was a problem hiding this comment.
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.
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.
I don't think this is true, since in CQ-only mode the database is specifically initialised with
code-qualityqueries. WhatgetDbAnalysisConfigtells 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.
mbg
force-pushed
the
mbg/cq/allow-cq-only-analysis
branch
from
f8915bb to
d2e5cc8
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements the ability to run Code Quality analysis independently from Code Scanning analysis, while maintaining backwards compatibility for combined analysis. The main purpose is to provide a "standalone" implementation where the database can be initialized specifically for Code Quality analysis by disabling default queries and configuring quality-specific queries.
Key changes include:
- Refactored upload targets into a new
analyses.tsmodule with dedicated configurations for both Code Scanning and Code Quality - Added logic to detect when only Code Quality analysis is enabled and configure the database accordingly
- Updated upload functions to use the new analysis configuration objects
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/analyses.ts | New module defining analysis configurations and endpoints for Code Scanning and Code Quality |
| src/config-utils.ts | Enhanced with functions to determine database analysis kind and handle Code Quality-only configurations |
| src/util.ts | Added isDefined utility function for type narrowing |
| src/upload-lib.ts | Refactored to use new analysis configurations instead of hardcoded upload targets |
| src/analyze.ts | Updated SARIF file generation logic to handle different analysis configurations |
| Various test files | Updated test cases to use new analysis configuration objects |
| Workflow files | Updated to use analysis-kinds input instead of deprecated quality-queries |
mbg
force-pushed
the
mbg/cq/allow-cq-only-analysis
branch
from
d2e5cc8 to
38f1a70
Compare
mbg
force-pushed
the
mbg/cq/allow-cq-only-analysis
branch
from
1ff29bf to
7916994
Compare
mbg
force-pushed
the
mbg/cq/allow-cq-only-analysis
branch
from
7916994 to
e75b5d3
Compare
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-qualitysuite into therun-queriescall. The Code Quality SARIF is then generated by an extra call tointerpret-results.code-qualityqueries.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