Basic support for overlay databases #2801
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
cklin
force-pushed
the
cklin/overlay-databases
branch
2 times, most recently
from
a083032 to
6ced3c5
Compare
cklin
force-pushed
the
cklin/overlay-databases
branch
from
6ced3c5 to
abbb392
Compare
|
I thought about it over the weekend and decided to make some changes. So taking the PR back to draft mode for now. |
When a user specifies "cleanup-level: overlay", it suggests that the user wishes to preserve the evaluation cache for future use. So in this case we should not set --expect-discarded-cache when running queries.
cklin
force-pushed
the
cklin/overlay-databases
branch
from
abbb392 to
95bbe5b
Compare
This commit adds a OverlayDatabaseMode parameter to databaseInitCluster(). The parameter controls the "codeql database init" flags concerning overlay database creation. There is no behavior change in this commit because we always pass OverlayDatabaseMode.None to databaseInitCluster(). That will change in the next commit.
This commit adds support for creating overlay-base and overlay databases, controlled via the CODEQL_OVERLAY_DATABASE_MODE environment variable.
cklin
force-pushed
the
cklin/overlay-databases
branch
from
95bbe5b to
0efe12d
Compare
|
I split the large "Support overlay database creation" commit into smaller commits. In addition to making the PR easier to review, the split also allows me to remove the code changes for file change detection, which is a functionality not used in this PR. As a result the PR as a whole is now smaller too. |
| const cleanupLevel = | ||
| actionsUtil.getOptionalInput("cleanup-level") || "brutal"; |
There was a problem hiding this comment.
Is it meaningful for users to enable overlay databases with a cleanup level that isn't overlay?
Should this default to overlay if overlay mode is enabled and a cleanup mode isn't specified? Note that this would also require changing analyze/action.yml as this default is unfortunately currently specified twice.
There was a problem hiding this comment.
When creating an overlay-base database, the only cleanup levels that make sense are overlay and none. So there is room for selecting a more sensible default cleanup level based on the overlay mode.
At this point we have not decided how we want the overlay database mode selection to work for the long run, which would affect how we communicate the selected overlay database mode to the analyze action. For the purpose of this PR, we are assuming that the invoking workflow handles all the complexity so that we can minimize the action changes, both in terms of new code and new design assumptions / commitments.
So I will keep your suggestion in mind and leave this PR as-is.
| ): Promise<OverlayDatabaseMode> { | ||
| const overlayDatabaseMode = process.env.CODEQL_OVERLAY_DATABASE_MODE; | ||
|
|
||
| if ( |
There was a problem hiding this comment.
Optional nit: consider using a switch statement with assertNever so if we add new cases to the OverlayDatabaseMode enum, the type checker will remind us to cover them here.
It's optional since we probably won't be adding new cases to this enum.
There was a problem hiding this comment.
Thanks for the suggestion! I looked at both places and decided that the benefit does not quite justify the increased code verbosity. Hopefully, if we should add a new value to the OverlayDatabaseMode enum, it would be apparent that we need to change where we make the OverlayDatabaseMode selection (here) and where we use the OverlayDatabaseMode value (in databaseInitCluster).
| @@ -606,12 +609,20 @@ export async function getCodeQLForCmd( | |||
| ? "--force-overwrite" | |||
| : "--overwrite"; | |||
|
|
|||
| if (overlayDatabaseMode === OverlayDatabaseMode.Overlay) { | |||
There was a problem hiding this comment.
Thanks—responded above.
This PR implements basic support for overlay databases. It does not handle the uploading and downloading of overlay-base databases; for now we are assuming that the workflow will provide the necessary functionality.
Merge / deployment checklist