-
Notifications
You must be signed in to change notification settings - Fork 422
Upload overlay base DBs to GitHub API behind FF #3316
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
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 functionality to upload overlay base databases to the GitHub API, controlled by a new feature flag UploadOverlayDbToApi. The implementation introduces a conditional cleanup strategy where databases are cleaned at the "overlay" level (preserving more data) instead of the "clear" level (complete cleanup) when the feature flag is enabled and the database mode is set to overlay-base.
Key Changes
- Introduced a new
CleanupLevelenum to replace string literals for database cleanup levels - Added a new feature flag
UploadOverlayDbToApito control the conditional upload behavior - Modified database upload logic to conditionally use overlay-level cleanup based on feature flag and database mode
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/util.ts | Added CleanupLevel enum with Clear and Overlay values for type-safe cleanup level specification |
| src/feature-flags.ts | Added UploadOverlayDbToApi feature flag configuration with environment variable CODEQL_ACTION_UPLOAD_OVERLAY_DB_TO_API |
| src/overlay-database-utils.ts | Renamed function to cleanupAndUploadOverlayBaseDatabaseToCache and updated to use CleanupLevel enum |
| src/database-upload.ts | Renamed function to cleanupAndUploadDatabases, added conditional logic to select cleanup level based on feature flag and overlay database mode |
| src/database-upload.test.ts | Updated all test calls to include the new features parameter required by the refactored function signature |
| src/codeql.ts | Updated databaseCleanupCluster method signature to accept CleanupLevel enum instead of string |
| src/analyze-action.ts | Updated function calls to use renamed functions and pass features parameter |
| package-lock.json | Contains updates to dependency metadata, including some unintentional changes to dev dependency markers |
| lib/*.js | Auto-generated JavaScript files reflecting the TypeScript changes |
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.
Broadly look good -- just one question.
| const cleanupLevel = | ||
| config.overlayDatabaseMode === OverlayDatabaseMode.OverlayBase && | ||
| (await features.getValue(Feature.UploadOverlayDbToApi)) | ||
| ? CleanupLevel.Overlay |
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'd like to understand more about the implications of this for the db upload - let's chat about that elsewhere.
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.Environments:
github.com.How did/will you validate this change?
I plan to validate this on some test repos using the FF.
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?
Observe on test repos first.
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist