Basic support for overlay PR analysis #2945
Conversation
This commit adds overlayDatabaseMode to AugmentationProperties and creates a placeholder getOverlayDatabaseMode() function, with the necessary inputs, to populate it.
This commit populates getOverlayDatabaseMode() in config-utils with the same code from getOverlayDatabaseMode() in init.
This commit changes databaseInitCluster() to use overlayDatabaseMode from AugmentationProperties instead of the overlayDatabaseMode parameter. There is no behavior change because both overlayDatabaseMode values are computed the same way. The commit then cleans up the overlayDatabaseMode parameter and the code paths that feed into it.
This commit changes getOverlayDatabaseMode so that, when Feature.OverlayAnalysis is enabled, it calculates the overlay database mode automatically based on analysis metadata. If we are analyzing the default branch, use OverlayBase, and if we are analyzing a PR, use Overlay. If CODEQL_OVERLAY_DATABASE_MODE is set to a valid overlay database mode, that environment variable still takes precedence.
This commit adds useOverlayDatabaseCaching to AugmentationProperties to indicate whether the action should upload overlay-base databases to the actions cache and to download a cached overlay-base database when creating an overlay database.
There was a problem hiding this comment.
Pull Request Overview
Adds foundational support for overlay database analysis, including caching of the overlay-base database and integration into init/analyze workflows.
- Introduces functions to upload/download overlay-base DB to Actions cache.
- Updates configuration logic to determine overlay modes and caching behavior.
- Integrates overlay mode handling into
initandanalyzeactions and feature flags.
Reviewed Changes
Copilot reviewed 28 out of 42 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/overlay-database-utils.ts | Implements check, upload, and download for overlay-base DB cache |
| src/config-utils.ts | Adds getOverlayDatabaseMode and new augmentation properties |
| src/init-action.ts | Passes sourceRoot to init and applies cache download logic |
| src/analyze-action.ts | Overrides cleanup level and uploads overlay-base DB to cache |
| src/feature-flags.ts | Introduces OverlayAnalysis feature and min CLI version check |
Comments suppressed due to low confidence (2)
src/overlay-database-utils.ts:170
- The new overlay database caching functions (
uploadOverlayBaseDatabaseToCacheanddownloadOverlayBaseDatabaseFromCache) are not covered by existing tests. Consider adding unit tests to validate their guard conditions and successful cache interactions.
export async function uploadOverlayBaseDatabaseToCache(
src/config-utils.ts:791
- [nitpick]
getOverlayDatabaseModecontains complex branching logic. Consider adding or expanding its JSDoc to outline the decision flow and environment variable precedence for better readability and future reference.
async function getOverlayDatabaseMode(
| logger: Logger, | ||
| ): Promise<boolean> { | ||
| const overlayDatabaseMode = config.augmentationProperties.overlayDatabaseMode; | ||
| if (overlayDatabaseMode !== OverlayDatabaseMode.OverlayBase) { |
There was a problem hiding this comment.
[nitpick] The guard logic for overlay database caching (checking mode, caching flag, and test mode) is duplicated in both upload and download functions. Consider extracting this into a shared helper to reduce duplication and simplify future maintenance.
Copilot uses AI. Check for mistakes.
| if (buildMode !== BuildMode.None) { | ||
| logger.warning( | ||
| `Cannot build an ${overlayDatabaseMode} database because ` + | ||
| `build-mode is set to "${buildMode}" instead of "none". ` + | ||
| "Falling back to creating a normal full database instead.", | ||
| ); | ||
| return nonOverlayAnalysis; |
There was a problem hiding this comment.
I think we need to check whether PR analyses for dynamic languages like Ruby actually have the build mode defined as None, or if it's left undefined.
| "from the CODEQL_OVERLAY_DATABASE_MODE environment variable.", | ||
| ); | ||
| } else if ( | ||
| ["github", "dsp-testing"].includes(repository.owner) && |
There was a problem hiding this comment.
Can you explain why we can't just use the feature flag to restrict overlay analysis to these orgs?
| } | ||
|
|
||
| function generateCacheKey(config: Config, codeQlVersion: string): string { | ||
| const sha = process.env.GITHUB_SHA || "unknown"; |
There was a problem hiding this comment.
If the GITHUB_SHA env var is somehow not set, should we instead skip uploading to the actions cache?
|
|
||
| const dbLocation = config.dbLocation; | ||
| const codeQlVersion = (await codeql.getVersion()).version; | ||
| const restoreKey = getCacheRestoreKey(config, codeQlVersion); |
There was a problem hiding this comment.
I don't follow how this can work, since the saveKey computation appends either a SHA or unknown to the result of getCacheRestoreKey(), and we're not appending that suffix here.
Merge / deployment checklist