Add a repository property for disabling overlay #3507
Add a repository property for disabling overlay #3507henrymercer wants to merge 9 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new repository property that allows org-owned repositories to disable overlay analysis without disabling default setup, integrating this property into overlay mode selection and strengthening typing for repository properties.
Changes:
- Introduce
github-codeql-disable-overlayrepository property and parse repository properties into typed values (boolean/string). - Update overlay database mode selection to honor the new property (with env var taking precedence).
- Add/adjust unit tests to cover new overlay selection behavior and updated API response typing.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/feature-flags/properties.ts | Adds new property name, typed property mapping/parsing, and updates API response typing. |
| src/feature-flags/properties.test.ts | Updates tests to align with the new exported API response type. |
| src/config-utils.ts | Threads repository properties into overlay mode computation and disables overlay when the property is set. |
| src/config-utils.test.ts | Adds tests verifying overlay is disabled/enabled based on the repository property and that env var precedence remains. |
| lib/init-action.js | Generated output updated to match TypeScript changes (not reviewed). |
src/feature-flags/properties.ts
Outdated
| } | ||
|
|
||
| /** | ||
| * The API returns a list of `RepositoryProperty` objects. |
There was a problem hiding this comment.
The JSDoc still says the API returns a list of RepositoryProperty objects, but that type no longer exists and the response type is now GitHubRepositoryProperty[]/GitHubPropertiesResponse. Update the comment to match the actual types to avoid confusion for future maintainers.
| * The API returns a list of `RepositoryProperty` objects. | |
| * The API returns `GitHubPropertiesResponse`, a list of `GitHubRepositoryProperty` objects. |
src/feature-flags/properties.ts
Outdated
| function isKnownPropertyName(value: string): value is RepositoryPropertyName { | ||
| return Object.values(RepositoryPropertyName).includes( | ||
| value as RepositoryPropertyName, | ||
| ); |
There was a problem hiding this comment.
isKnownPropertyName recomputes Object.values(RepositoryPropertyName) on every call, and it's called once per remote property in loadPropertiesFromApi. Consider using a module-level Set<string> of known names (or caching the array) so the per-property lookup is O(1) without repeated allocations.
| function isKnownPropertyName(value: string): value is RepositoryPropertyName { | |
| return Object.values(RepositoryPropertyName).includes( | |
| value as RepositoryPropertyName, | |
| ); | |
| const KNOWN_REPOSITORY_PROPERTY_NAMES = new Set<string>( | |
| Object.values(RepositoryPropertyName), | |
| ); | |
| function isKnownPropertyName(value: string): value is RepositoryPropertyName { | |
| return KNOWN_REPOSITORY_PROPERTY_NAMES.has(value); |
src/feature-flags/properties.ts
Outdated
| const mapRepositoryProperties: { | ||
| [K in RepositoryPropertyName]: (value: string) => AllRepositoryProperties[K]; | ||
| } = { | ||
| [RepositoryPropertyName.DISABLE_OVERLAY]: (value) => value === "true", | ||
| [RepositoryPropertyName.EXTRA_QUERIES]: (value) => value, |
There was a problem hiding this comment.
New behavior maps DISABLE_OVERLAY from the API string value to a boolean (value === "true"), but there is no unit test exercising this parsing/mapping in loadPropertiesFromApi. Add a test case that returns github-codeql-disable-overlay from the mocked API and asserts the resulting RepositoryProperties contains the correct boolean.
mbg
left a comment
mbg
left a comment
There was a problem hiding this comment.
Looks good, glad to see this coming together! A bunch of minor points / questions, but nothing critical.
src/feature-flags/properties.ts
Outdated
| const mapRepositoryProperties: { | ||
| [K in RepositoryPropertyName]: (value: string) => AllRepositoryProperties[K]; | ||
| } = { | ||
| [RepositoryPropertyName.DISABLE_OVERLAY]: (value) => value === "true", |
There was a problem hiding this comment.
Minor: Since this is new, consider making this check case-insensitive?
There was a problem hiding this comment.
This is consistent with other boolean checks, e.g. in environment variables. Also the repository properties UI has support for boolean properties, which will be set to either "true" or "false".
There was a problem hiding this comment.
Agreed that it is consistent, but is it intuitive? Do we gain anything from being this strict here?
For the repo properties UI, doesn't that require each org to individually configure the property as a boolean? If so, it's easy to imagine someone accidentally configuring it as a string and then using True etc.
There was a problem hiding this comment.
It's a valid point, but on the flip side if we allow True (or 1, etc) here, it's surprising when that isn't accepted elsewhere. We can make it clear in our instructions that this should be configured as a boolean property.
github-actions
bot
added
size/L
henrymercer
force-pushed
the
henrymercer/overlay-repo-property
branch
from
4f816a2 to
70db156
Compare
mbg
left a comment
mbg
left a comment
There was a problem hiding this comment.
This seems good to merge. A couple of comments, but they aren't blocking.
| type AllRepositoryProperties = { | ||
| [RepositoryPropertyName.DISABLE_OVERLAY]: boolean; | ||
| [RepositoryPropertyName.EXTRA_QUERIES]: string; | ||
| }; |
There was a problem hiding this comment.
I didn't comment on this in my initial review, but I quite like this mapping of properties to expected types and the associated machinery below.
src/config-utils.ts
Outdated
| let skippedDueToCachedStatus = false; | ||
| let disabledByRepositoryProperty = false; |
There was a problem hiding this comment.
Minor: We have this growing list of causes why overlay analysis is skipped/disabled, but presumably only one of these can be true at a given time. (At least based on the current logic, disabledByRepositoryProperty seems to imply skippedDueToCachedStatus is false.) Not blocking for this PR, but perhaps this should be refactored into an enum or similar that indicates the reason why overlay analysis was skipped/disabled. Otherwise, we might be mislead into thinking that e.g. skippedDueToCachedStatus would be true if we'd be skipping overlay analysis based on cache status, even if we already decided to disable it based on the repo property.
There was a problem hiding this comment.
Yeah, great point, let's make this an enum.
src/config-utils.ts
Outdated
| ); | ||
| } | ||
|
|
||
| if (overlayDisabledByRepositoryProperty) { |
There was a problem hiding this comment.
(Similarly to my comment above, the way these diagnostics are emitted might suggest that all of these could be generated at the same time.)
github-actions
bot
added
size/XL
Add a repository property that can be used to disable overlay for repositories that are owned by an organisation.
This means repositories that are owned by an organisation can disable overlay without needing to disable default setup too.
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