Conversation
There was a problem hiding this comment.
Pull request overview
This PR refines overlay-analysis disablement reporting by splitting previously broad disablement categories into more specific OverlayDisabledReason values, primarily to improve telemetry and diagnostics around why overlay analysis doesn’t run.
Changes:
- Expand
OverlayDisabledReasonto distinguish disk vs memory resource failures and feature-enablement sub-cases. - Refactor overlay enablement/resource checks to return a specific disablement reason (or
undefinedwhen enabled). - Update unit tests to expect the new disablement reasons.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/overlay/diagnostics.ts | Extends OverlayDisabledReason with more granular reason values used for telemetry. |
| src/config-utils.ts | Refactors overlay enablement/resource-check logic to return specific disablement reasons. |
| src/config-utils.test.ts | Updates overlay mode tests to assert the new disablement reasons. |
| lib/init-action.js | Generated output reflecting the TypeScript changes (not reviewed). |
mbg
left a comment
mbg
left a comment
There was a problem hiding this comment.
Thanks for working on this -- getting a more detailed reason for why overlay is disabled is useful!
That said, I am not overly fond of some of the details of the changes here, sorry! See my comments for the details.
| * languages and configuration, returning the specific reason it is disabled, | ||
| * or `undefined` if it is enabled. | ||
| */ | ||
| async function getOverlayFeatureDisabledReason( |
There was a problem hiding this comment.
I don't think this is a good re-naming for this function. The function name should be focused on what the function does, but the new name suggests this is a getter which just retrieves some existing value. But the function actually performs the checks, and only returns the reason why overlay analysis is disabled if one of those checks is negative. I view it returning the reason why overlay is disabled as secondary.
The old name (isOverlayAnalysisFeatureEnabled) is better in this regard, but just isOverlayAnalysisEnabled would also be good. (Or indeed isOverlayAnalysisDisabled.)
More broadly, I wonder if I'd prefer a { enabled: true } | { enabled: false, reason: OverlayDisabledReason } kind of type for these things, rather than OverlayDisabledReason | undefined. I don't particularly like using undefined to indicate a successful outcome / the absence of a non-standard condition.
| * Overlay analysis is restricted to code-scanning-only configs but the config uses custom | ||
| * queries or packs, disables default queries, or specifies query filters. |
There was a problem hiding this comment.
I know you have already changed this in response to Copilot's comment, but a more significant issue with this comment is the ambiguity over whether it refers to:
analysis-kind: code-scanning, with no customisation; or- "code-scanning-only configs" is supposed to mean no query customisations.
I.e. it is not clear if "code-scanning-only configs" is about analysis kinds or not.
| // if the default queries are not disabled, and no packs, queries, or | ||
| // query-filters are specified. | ||
| return ( | ||
| const isCodeScanningOnly = |
There was a problem hiding this comment.
See my other comment, but I am concerned about the naming here / relationship with analysis kinds. Since we have a specific code-scanning analysis kind, it is confusing that this seems to mean "no customisation".
| async function getResourceDisabledReason( | ||
| codeql: CodeQL, | ||
| diskUsage: DiskUsage | undefined, | ||
| ramInput: string | undefined, | ||
| logger: Logger, | ||
| useV2ResourceChecks: boolean, | ||
| ): Promise<boolean> { | ||
| ): Promise<OverlayDisabledReason | undefined> { |
There was a problem hiding this comment.
Same comments as for getOverlayFeatureDisabledReason
| (disabledReason = await getOverlayFeatureDisabledReason( | ||
| features, | ||
| codeql, | ||
| languages, | ||
| codeScanningConfig, | ||
| ) | ||
| )) === undefined |
There was a problem hiding this comment.
It would be good to move this assignment out of the condition.
| (resourceDisabledReason = await getResourceDisabledReason( | ||
| codeql, | ||
| diskUsage, | ||
| ramInput, | ||
| logger, | ||
| useV2ResourceChecks, | ||
| )) | ||
| )) !== undefined |
Break down the two most common categories to help us better understand why overlay isn't run.
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, Code Quality, ...).Products:
analysis-kinds: code-scanning.Environments:
github.comand/or GitHub Enterprise Cloud with Data Residency.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?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist