-
Notifications
You must be signed in to change notification settings - Fork 422
Overlay: Fall back to full analysis if runner disk space is low #3310
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.
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
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.
One comment about the order of checks, otherwise I am happy to accept this as a conservative guard against regressions that might result from the increased space requirements.
182eebd to
4eccb37
Compare
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 for addressing the feedback! I think is good to merge now.
One style comment, but not worth addressing here.
| } else { | ||
| if (isAnalyzingPullRequest()) { | ||
| overlayDatabaseMode = OverlayDatabaseMode.Overlay; | ||
| useOverlayDatabaseCaching = true; | ||
| logger.info( | ||
| `Setting overlay database mode to ${overlayDatabaseMode} ` + | ||
| "with caching because we are analyzing a pull request.", | ||
| ); | ||
| } else if (await isAnalyzingDefaultBranch()) { | ||
| overlayDatabaseMode = OverlayDatabaseMode.OverlayBase; | ||
| useOverlayDatabaseCaching = true; | ||
| logger.info( | ||
| `Setting overlay database mode to ${overlayDatabaseMode} ` + | ||
| "with caching because we are analyzing the default branch.", | ||
| ); | ||
| } |
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: This could now just be two else if blocks rather than an if/else inside an else block.
This PR introduces a fallback from overlay analysis to normal analysis if available runner disk space is low when deciding whether to perform overlay analysis during the
initaction. The fallback only applies when overlay analysis is enabled by feature flag and is ignored if overlay analysis is enabled by environment variable. The hardcoded limit of 20GB available disk space has been chosen based on telemetry from thecodeql-actionand will allow most +3m analysis runs to run in overlay mode.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
analysis-kinds: code-scanning).analysis-kinds: code-quality).How did/will you validate this change?
.test.tsfiles).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?
Merge / deployment checklist