-
Notifications
You must be signed in to change notification settings - Fork 433
Add feature flag to skip computing baseline file coverage information on PRs #3424
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
8fc92e6 to
18c2cfc
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 introduces a feature flag that allows skipping baseline file coverage computation for PR analyses (currently gated for internal GitHub usage), wires that through the configuration, and cleans up an unused CLI method, along with a small dependency update.
Changes:
- Add
Feature.SkipFileCoverageOnPrsand thread a newenableFileCoverageInformationboolean throughInitConfigInputsandConfig, defaulting totrueand computed ininit-actionbased on debug mode, PR detection, repository owner, and the new feature flag. - Update
databaseInitClusterincodeql.tsto conditionally pass baseline-related CLI flags based onconfig.enableFileCoverageInformation, and log an informational message inanalyze.tswhen file coverage is disabled for a run. - Remove the unused
databasePrintBaselinemethod from theCodeQLinterface and stub, adjust tests and test utilities accordingly, and bumpcaniuse-liteinpackage-lock.json.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/testing-utils.ts | Extends createTestConfig to set enableFileCoverageInformation: true by default so tests construct a complete Config including the new flag. |
| src/init-action.ts | Imports isAnalyzingPullRequest, computes debugMode once, and derives enableFileCoverageInformation from debug mode, PR vs non-PR, repository owner, and Feature.SkipFileCoverageOnPrs before passing it into initConfig. |
| src/feature-flags.ts | Adds the SkipFileCoverageOnPrs feature enum and its featureConfig entry, backed by the CODEQL_ACTION_SKIP_FILE_COVERAGE_ON_PRS env var and currently without a minimum CLI version. |
| src/config-utils.ts | Extends Config and InitConfigInputs with enableFileCoverageInformation and ensures it is carried through initActionState to the returned Config. |
| src/config-utils.test.ts | Updates createTestInitConfigInputs to include enableFileCoverageInformation: true, keeping tests aligned with the extended InitConfigInputs shape. |
| src/codeql.ts | Removes the databasePrintBaseline method from the CodeQL interface and stub, and makes databaseInitCluster conditionally include baseline-related CLI flags via a new baselineFilesOptions array and ignoringOptions for extra env options. |
| src/analyze.ts | Guards logging of analysisSummary / qualityAnalysisSummary against empty strings and logs a clear informational message when config.enableFileCoverageInformation is false. |
| src/analyze.test.ts | Simplifies the createStubCodeQL usage for the status report test by dropping the now-removed databasePrintBaseline stub. |
| package-lock.json | Bumps caniuse-lite to 1.0.30001766 (including integrity, URL, and license metadata) to eliminate build warnings. |
| lib/upload-sarif-action.js | Updates generated JS to reflect the new feature flag and baseline option wiring used by the SARIF upload action. |
| lib/upload-sarif-action-post.js | Regenerates JS to include the SkipFileCoverageOnPrs feature config for the upload post action. |
| lib/upload-lib.js | Regenerated library JS mirroring the new feature flag and conditional baseline CLI options. |
| lib/start-proxy-action.js | Generated JS updated to include the new SkipFileCoverageOnPrs entry in featureConfig. |
| lib/start-proxy-action-post.js | Generated JS updated similarly for the proxy post action’s feature configuration. |
| lib/setup-codeql-action.js | Regenerated setup action JS to mirror the new baseline option handling and feature flag definition. |
| lib/resolve-environment-action.js | Generated JS updated with the new feature flag entry. |
| lib/init-action.js | Generated JS reflecting the new enableFileCoverageInformation computation and propagation into initConfig, plus conditional baseline flags. |
| lib/init-action-post.js | Regenerated JS including the SkipFileCoverageOnPrs feature config and baseline option handling. |
| lib/autobuild-action.js | Generated JS updated to add the SkipFileCoverageOnPrs feature to the shared featureConfig. |
| lib/analyze-action.js | Generated JS mirroring the updated runQueries logging and use of config.enableFileCoverageInformation. |
| lib/analyze-action-post.js | Generated JS updated with the new feature flag config and baseline option wiring for the analyze post action. |
mbg
left a comment
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.
Thanks! A few comments, mainly about test coverage and some awkward wording in a log message.
Should we also add some telemetry here to measure what impact this has?
| // Always enable file coverage information in debug mode | ||
| debugMode || | ||
| // We're most interested in speeding up PRs, and we want to keep | ||
| // submitting file coverage information for the default branch since | ||
| // it is used to populate the status page. | ||
| !isAnalyzingPullRequest() || | ||
| // For now, restrict this feature to the GitHub org | ||
| repositoryNwo.owner !== "github" || | ||
| !(await features.getValue(Feature.SkipFileCoverageOnPrs)), |
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 move this to a separate function and add some unit tests for it?
| } | ||
| if (!config.enableFileCoverageInformation) { | ||
| logger.info( | ||
| "File coverage information is disabled for this PR analysis for performance reasons. " + |
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.
Minor: the wording of this could be interpreted as being specific to a given PR, rather than PRs in general. It could lead a user to ask why it is disabled for their PR specifically. Can we word this differently?
For example, if we (eventually) plan to do this for all pull requests:
| "File coverage information is disabled for this PR analysis for performance reasons. " + | |
| "File coverage information is disabled for pull requests. " + |
| if (!config.enableFileCoverageInformation) { | ||
| logger.info( | ||
| "File coverage information is disabled for this PR analysis for performance reasons. " + | ||
| "It will still be enabled for analyses triggered by a push or a schedule.", |
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.
The future tense here suggests that this is something we will implement in the future, but that wouldn't happen right now.
Additionally, "by a push" is misleading, because file coverage information would not be generated if a user pushed a commit to a PR branch. You're talking about push events. Talking about the events may be confusing for dynamic workflows though, where these rules don't apply in the same way.
| "It will still be enabled for analyses triggered by a push or a schedule.", | |
| "It is available for analyses on the main branch.", |
| const baselineFilesOptions = config.enableFileCoverageInformation | ||
| ? [ | ||
| "--calculate-language-specific-baseline", | ||
| "--sublanguage-file-coverage", |
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 this is supported by all versions of GHES that we support now (GitHub Enterprise Server v3.12.0+ according to the CLI). A few questions about this:
- I suppose that once we have a tools feature for
enableFileCoverageInformation, that it implies the availability of--sublanguage-file-coverageso we don't need any other checks here. - Are there any downstream implications of this option that we need to / should account for?
- Any tests that need updating, or should be added?
| ...extraArgs, | ||
| ...getExtraOptionsFromEnv(["database", "init"], { | ||
| ignoringOptions: ["--overwrite"], | ||
| ignoringOptions: ["--overwrite", ...baselineFilesOptions], |
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 we add a comment here for why these options in particular cannot be overridden?
codeql database interpret-resultscurrently expects baseline information to always be present, and displays a log message "The database provided was made with a CLI version before 2.11.2 which did not record file baseline information, so file baseline information will be absent." when it is missing. We'll address this and put this feature behind a tools feature flag. At this point we'll also add a changelog entry. I'm proposing merging this now to make more incremental code changes and to allow us to test this internally.caniuse-litepackage to avoid a pesky warning during builds.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
dynamicworkflows (Default Setup, CCR, ...).Products:
analysis-kinds: code-scanning.analysis-kinds: code-quality.Environments:
github.comand/or GitHub Enterprise Cloud with Data Residency.How did/will you validate this change?
See https://github.com/github/codeql-action/actions/runs/21356271329
If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist