-
Notifications
You must be signed in to change notification settings - Fork 428
Determine CodeQL version from feature flags on GHEC-DR #3351
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
Determine CodeQL version from feature flags on GHEC-DR #3351
Conversation
This more closely reflects the published naming https://docs.github.com/en/enterprise-cloud@latest/admin/data-residency/about-github-enterprise-cloud-with-data-residency
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
This PR renames the GitHub variant GHE_DOTCOM to GHEC_DR (GitHub Enterprise Cloud with Data Residency) and enables feature flag support for determining the CodeQL version on GHEC-DR environments. This allows faster rollback of bad CodeQL versions in GHEC-DR by using feature flags, similar to how it works for GitHub.com.
Key Changes
- Renamed
GHE_DOTCOMenum variant toGHEC_DRwith string values for better clarity - Enabled feature flag support for GHEC-DR to determine CodeQL CLI versions
- Updated CodeQL source selection logic to be more precise (checking for GHES specifically instead of "not DOTCOM")
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/util.ts |
Renamed enum variant from GHE_DOTCOM to GHEC_DR and changed enum values from numeric to string literals |
src/util.test.ts |
Updated test cases to use renamed GHEC_DR variant |
src/setup-codeql.ts |
Fixed logic to check for GHES specifically instead of "not DOTCOM", enabling GHEC-DR to use default tooling |
src/feature-flags.ts |
Added supportsFeatureFlags helper function that includes both DOTCOM and GHEC-DR, enabling feature flags for GHEC-DR |
src/feature-flags.test.ts |
Renamed test from "Proxima" to "GHEC-DR" and parameterized tests to cover both DOTCOM and GHEC-DR variants |
src/database-upload.ts |
Updated variant check to use renamed GHEC_DR constant |
src/api-client.ts |
Updated return value to use GHEC_DR when detecting "ghe.com" header |
src/api-client.test.ts |
Updated test to use GHEC_DR variant name |
.github/pull_request_template.md |
Updated documentation to clarify that "Dotcom" environment includes GHEC-DR |
lib/**/*.js |
Auto-generated JavaScript files reflecting TypeScript changes (not reviewed per guidelines) |
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.
Changes look good and make sense to me! I added a couple of very minor style suggestions, but feel free to ignore those.
src/util.ts
Outdated
| GHE_DOTCOM, | ||
| DOTCOM = "GitHub.com", | ||
| GHES = "GHES", | ||
| GHEC_DR = "GHEC-DR", |
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: Should the string value be spelled out as e.g. GHEC (with Data Residency) or similar?
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've expanded this.
| switch (version.type) { | ||
| case util.GitHubVariant.DOTCOM: | ||
| return "dotcom"; | ||
| case util.GitHubVariant.GHE_DOTCOM: | ||
| return "GHE dotcom"; | ||
| case util.GitHubVariant.GHEC_DR: | ||
| return "GHEC-DR"; | ||
| case util.GitHubVariant.GHES: | ||
| return `GHES ${version.version}`; | ||
| default: |
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: How about something like this now that GitHubVariant has string values associated with the elements?
| switch (version.type) { | |
| case util.GitHubVariant.DOTCOM: | |
| return "dotcom"; | |
| case util.GitHubVariant.GHE_DOTCOM: | |
| return "GHE dotcom"; | |
| case util.GitHubVariant.GHEC_DR: | |
| return "GHEC-DR"; | |
| case util.GitHubVariant.GHES: | |
| return `GHES ${version.version}`; | |
| default: | |
| switch (version.type) { | |
| case util.GitHubVariant.DOTCOM: | |
| case util.GitHubVariant.GHEC_DR: | |
| return `${version.type}`; | |
| case util.GitHubVariant.GHES: | |
| return `GHES ${version.version}`; | |
| default: |
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.
Now that we're using the full names above, I think it's preferable to keep the abbreviated names here to keep test titles concise.
This lets us roll back a bad version of CodeQL more quickly in GitHub Enterprise Cloud with data residency.
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, CCR, ...).Products:
analysis-kinds: code-scanning.analysis-kinds: code-quality.Environments:
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