-
Notifications
You must be signed in to change notification settings - Fork 369
Basic support for overlay PR analysis #2945
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
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
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
init
andanalyze
actions 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 (
uploadOverlayBaseDatabaseToCache
anddownloadOverlayBaseDatabaseFromCache
) 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]
getOverlayDatabaseMode
contains 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
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 have a few questions/comments, but overall it looks very sensible.
src/config-utils.ts
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 updated the code so that it checks for build mode only for traced languages.
"from the CODEQL_OVERLAY_DATABASE_MODE environment variable.", | ||
); | ||
} else if ( | ||
["github", "dsp-testing"].includes(repository.owner) && |
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.
Can you explain why we can't just use the feature flag to restrict overlay analysis to these orgs?
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.
This PR does not fully implement all the functionality that we planned, so it is considered an early prototype and suitable for internal testing only. So we want to be sure that we never attempt to perform overlay analysis on user repositories with this implementation, even if a user were to pin their workflow to this PR after merge, and then we enable all feature flags.
That is why I am using an explicit allowlist here instead of relying on feature flags.
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.
this seems to be worth a TODO
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.
There is also a potential concern here for outside of dotcom if this ends up in a release, since it is possible for customers on other instances to have organisations with specific names. Two options:
- If you want to get this merged, then I think it would be good to put this behind a FF now, which then also helps us going forward.
- If it's really for internal testing only, then you may not need to get this PR merged at all, if you are able to reference this branch directly in testing workflows
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.
Sorry, I only reviewing this fully now, so I think I missed that you have already added the FF and reasoned why you should have both checks. I am not sure we should really be concerned with users pinning feature branches. That's not a concern for default setup, and if they did it with an advanced setup, then they are probably aware that it is at their own risk (they could also just enable the environment variable).
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.
this seems to be worth a TODO comment 🙂
Yes, that is a good suggestion. I will do that.
There is also a potential concern here for outside of dotcom if this ends up in a release, since it is possible for customers on other instances to have organisations with specific names.
Thanks for bringing that up—I haven't considered that possibility. That said, we do have a feature flag on top of the allow list (your option 1 above), so GHES users should never reach this check.
If it's really for internal testing only, then you may not need to get this PR merged at all, if you are able to reference this branch directly in testing workflows
The internal testing can include staff-ship to internal repositories, and it would be ideal if we can do so through feature flags without requiring workflow file changes.
I am not sure we should really be concerned with users pinning feature branches.
Once this PR merges, a user could get this code by pinning a commit on main
, which I think is a use case we should support.
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 internal testing can include staff-ship to internal repositories, and it would be ideal if we can do so through feature flags without requiring workflow file changes.
Agreed.
Once this PR merges, a user could get this code by pinning a commit on main, which I think is a use case we should support.
I am a bit conflicted about this. On one hand, I agree that a user could do this and the "future FF enablement on an old pinned version" scenario is an interesting one to consider. On the other hand, I am not sure how much we have considered this scenario in the past -- possibly a question for @aeisenberg or @henrymercer. I'm slightly leaning towards thinking that the FF alone is probably enough, particularly since a user who pins this version could also just set the environment variable to get this prototype implementation, but no strong feelings.
FYI we also have a FF in default setup for internal things. That isn't propagated to the action right now in any way, but it could be. Perhaps if we feel that this kind of feature gating is necessary for experimental features, then we could instead do some work to start using that.
src/overlay-database-utils.ts
Outdated
} | ||
|
||
function generateCacheKey(config: Config, codeQlVersion: string): string { | ||
const sha = process.env.GITHUB_SHA || "unknown"; |
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.
If the GITHUB_SHA
env var is somehow not set, should we instead skip uploading to the actions cache?
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.
Good point. In addition to what you said, there is another reason why we should not be relying on GITHUB_SHA
: the environment variable, even if it exists, may not necessarily correspond to the commit that is analyzed. It is better to call getCommitOid()
instead.
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 updated the code to use getCommitOid()
.
|
||
const dbLocation = config.dbLocation; | ||
const codeQlVersion = (await codeql.getVersion()).version; | ||
const restoreKey = getCacheRestoreKey(config, codeQlVersion); |
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 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.
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.
This code relies on the actions cache feature of performing restores using prefix match on the cache key:
https://docs.github.com/en/actions/how-tos/writing-workflows/choosing-what-your-workflow-does/caching-dependencies-to-speed-up-workflows#matching-a-cache-key
Thank you for your feedback @nickrolfe! I will take the PR back to draft to address comments and resolve merge conflicts. Once that is done, I will mark the PR as ready for review again. |
fd2660c
to
bc3a47b
Compare
@nickrolfe The PR is ready for another look. |
// An overlay-base database should always use the 'overlay' cleanup level | ||
// to preserve the cached intermediate results. | ||
// | ||
// Note that we may be overriding the 'cleanup-level' input parameter. | ||
const cleanupLevel = | ||
actionsUtil.getOptionalInput("cleanup-level") || "brutal"; | ||
config.augmentationProperties.overlayDatabaseMode === | ||
OverlayDatabaseMode.OverlayBase | ||
? "overlay" | ||
: actionsUtil.getOptionalInput("cleanup-level") || "brutal"; |
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.
Wouldn't it be better to raise a configuration error if we explicitly set anything else than overlay
or nothing if we are using overlay database? I think we shouldn't silently ignore an explicit parameter by the user
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 agree with @redsun82. It might be surprising if the cleanup-level
option is silently overriden. An error if cleanup-level
is set and overlay databases are enabled would make sense.
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 am a bit conflicted about what to do here.
On the one hand, silently ignoring explicit user input (which I am doing here) is not considered proper behavior.
On the other hand, it is important for the action to maintain backward compatibility. If we were to throw an error for users setting cleanup-level
to a value other than overlay
, we would break those (currently working) workflows when we roll out overlay analysis. I can't think of any reason for users to set cleanup-level
to overlay
, especially since it is currently not even a documented allowed value.
Another option is to not build an overlay-base database if the user specifies a cleanup-level
that is not overlay
. There is no good way to do that because the decision to build the overlay-base database was made in the init
step, and the cleanup-level
input is in the analyze
step. We could obey the cleanup-level
input and not upload the overlay-base database if the input is not overlay
, which means that the workflow will not benefit from overlay analysis. It becomes a matter of which we think the user would care more about: getting the benefit of overlay analysis when we roll it out, or having the cleanup-level
input strictly obeyed?
Of these three options, I think the currently implemented behavior is the best choice. We can make the behavior change more explicit, by updating the cleanup-level
description to state that it is advisory, and (if you consider it appropriate) to add a changelog entry stating the same. At the end of the day, the original assumption when we added cleanup-level
(that database cleanup is a choice that has no impact on how the action itself functions) will no longer hold, and I think it is not unreasonable for the meaning of the input to change accordingly.
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 for the thoughtful discussion of this!
Firstly, I am thinking that, regardless of which behaviour we go with, we should implement a warning that warns of incompatible cleanup-level
and overlay database settings.
Beyond that, my feeling is that we should preserve existing behaviour, at least until we have given enough notice that a change is coming. That is to say, I'd probably have a preference for "We could obey the cleanup-level input and not upload the overlay-base database if the input is not overlay, which means that the workflow will not benefit from overlay analysis.".
I would say that is acceptable because:
- We will have a warning which notifies the user (but not an error that breaks existing workflows).
- We can assume that
cleanup-level
isn't super commonly used, and most users will still benefit from overlay databases anyway, particularly in default setup where nocleanup-level
is specified. - We don't override user-configured behaviour.
- Since
cleanup-level
is used in advanced setup, users can change this setting if they have it configured and they want to take advantage of overlay databases.
We can then, after some time, revisit this to decide whether we want to override the cleanup-level
setting if overlay databases are enabled.
"from the CODEQL_OVERLAY_DATABASE_MODE environment variable.", | ||
); | ||
} else if ( | ||
["github", "dsp-testing"].includes(repository.owner) && |
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.
this seems to be worth a TODO
comment 🙂
[Feature.OverlayAnalysis]: { | ||
defaultValue: false, | ||
envVar: "CODEQL_ACTION_OVERLAY_ANALYSIS", | ||
minimumVersion: CODEQL_OVERLAY_MINIMUM_VERSION, |
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.
Theoretically as stated on line 86, we should
Prefer using
ToolsFeature
s for future flags
Maybe we're not very thorough with that, and as far as I can see we did not add overlayDatabases
to the list of CLI features that are listed in codeql version --format=json
. We should have, right?
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.
Agreed that it would be better to use the CLI's features list than set a minimumVersion
here.
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 CLI interface for overlay analysis has not yet been completely finalized, which is one of the reasons why we have both the feature flag check and the explicit allowlist for internal orgs. We can use ToolsFeature
s, but that would require us to come up with a new ToolsFeature
if there should be an upcoming change that is not backwards compatible. Setting a minimum version number makes it easier to iterate.
There is also the question of how much we want to lean on the version or feature check. Support for overlay analysis is not just a feature of the CLI—it is a feature of the CLI and the compiled queries. ToolsFeature
tells us whether we can pass the overlay analysis flags to the CLI and not get an error. But it does not tell us whether the query packs are ready for overlay analysis. By setting a minimum version number, we can version not only the CLI but also the precompiled query packs included in the bundle, which helps us avoid unintended enablement.
(In the long run, we do plan to make the CLI robust enough so that it can correctly handle existing pre-compiled query packs for overlay analysis. For now internal testing focuses on query packs included in the bundle, and the minimum version serves as a useful check.)
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 I am happy with that justification for leaving this as is for now.
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 re-reviewed the parts I previously commented on, and I'm happy with the changes.
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.
Thank you for preparing this PR -- I am very excited for this functionality! 💖
As I suggested in my earlier comment, if the main goal is to test this internally for now, then it may be best to leave this PR unmerged and reference the PR branch explicitly in test workflows. Have you done any testing like that yet? If not, it would be good to do some of that in any case.
If we want to merge this, then the changes will end up in the next release. Before we do that, I think we should address a few points:
- Before we merge this, we should add some test coverage. At this stage, it would be good to gain confidence that none of the new logic interferes with existing functionality when the feature is disabled. The existing PR checks do that to an extent, but we should verify that we cover all cases. In the long run, we will want test coverage for when the feature is enabled as well.
- There are general concerns around us using the Actions cache, similar to what we have for TRAP caching and dependency caching. I.e. we might be eating away at the cache quota that may already be in use by other workflows in a given repository. Can we mitigate our impact here? For example, by ensuring that we only have one cache at a time or limiting the size of the caches we store. This may not need to be addressed at this point if we only plan to use this internally for now, but it should be addressed before we ship it more widely, especially if the goal is to have it on by default at some point.
* @returns the base and head branches of the pull request, or undefined if | ||
* we are not analyzing a pull request. | ||
*/ | ||
export function getPullRequestBranches(): PullRequestBranches | undefined { |
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.
It would be good to have unit tests for this which exercise the different conditions.
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.
This is not new code—it is existing code that was moved to allow greater reuse.
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 noticed -- we probably should have added some tests back then, so now may be a good opportunity to do that.
// An overlay-base database should always use the 'overlay' cleanup level | ||
// to preserve the cached intermediate results. | ||
// | ||
// Note that we may be overriding the 'cleanup-level' input parameter. | ||
const cleanupLevel = | ||
actionsUtil.getOptionalInput("cleanup-level") || "brutal"; | ||
config.augmentationProperties.overlayDatabaseMode === | ||
OverlayDatabaseMode.OverlayBase | ||
? "overlay" | ||
: actionsUtil.getOptionalInput("cleanup-level") || "brutal"; |
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 agree with @redsun82. It might be surprising if the cleanup-level
option is silently overriden. An error if cleanup-level
is set and overlay databases are enabled would make sense.
import * as actionsUtil from "./actions-util"; | ||
import { | ||
getRequiredInput, | ||
getTemporaryDirectory, | ||
PullRequestBranches, | ||
} from "./actions-util"; |
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: I note that this change results in two unrelated changes in this file. We have imports like import * as actionsUtil
in a lot of places already, so I am not sure there's a good reason for changing this here as part of this PR?
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 don't think there is a compelling reason for the change either. I am not even sure the change was intentional—it might have been made by copilot or IDE auto-completion, which I then let stand because it did not add that much noise to justify the additional work to preserve the existing import style, which seems just as arbitrary.
If this becomes a concern, we might be able to configure the linter to insist on one import style or the other.
[Feature.OverlayAnalysis]: { | ||
defaultValue: false, | ||
envVar: "CODEQL_ACTION_OVERLAY_ANALYSIS", | ||
minimumVersion: CODEQL_OVERLAY_MINIMUM_VERSION, |
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.
Agreed that it would be better to use the CLI's features list than set a minimumVersion
here.
const sourceRoot = path.resolve( | ||
getRequiredEnvParam("GITHUB_WORKSPACE"), | ||
getOptionalInput("source-root") || "", | ||
); |
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.
It may be worth adding a short comment here to explain that resolve
respects the intended semantics of source-root
(relative to the workspace root by default, but can be absolute).
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.
This is not new code—it is existing code that was moved to make sourceRoot
available for initConfig()
. But I can add the comment you requested.
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! Yes, I saw that it had been moved, but ultimately that doesn't change my thinking that this would be a useful thing to have!
"from the CODEQL_OVERLAY_DATABASE_MODE environment variable.", | ||
); | ||
} else if ( | ||
["github", "dsp-testing"].includes(repository.owner) && |
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.
Sorry, I only reviewing this fully now, so I think I missed that you have already added the FF and reasoned why you should have both checks. I am not sure we should really be concerned with users pinning feature branches. That's not a concern for default setup, and if they did it with an advanced setup, then they are probably aware that it is at their own risk (they could also just enable the environment variable).
// we are analyzing. However, since overlay-base databases are built from the | ||
// default branch and used in PR analysis, it is exceedingly unlikely that | ||
// the commit SHA will ever be the same, so we can just leave it out. | ||
const languages = [...config.languages].sort().join("_"); |
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.
Henry has an open PR to open up languages more, which might affect this in the future. Not necessarily something we need to consider here, but may need to consider there.
@mbg Thank you for your review!
I agree that more tests are a good idea. Can you say more about what you mean by "gain confidence that none of the new logic interferes with existing functionality" and "cover all cases", so that I have a better estimate of the effort required?
Thank you for bringing that up. I added a note to the internal tracking issue for consideration later. |
Broadly, we should check that |
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.
a8a0cc8
to
f32db97
Compare
f32db97
to
55ea4c3
Compare
I made some of the requested changes (diff from force pushes sans rebase).
These are the changes I consider (a) in-scope for this PR and (b) there is clear consensus on how to proceed. The main unresolved issue is how we want to handle the
What do you think? |
Merge / deployment checklist