-
Notifications
You must be signed in to change notification settings - Fork 433
Report queried feature flags in status reports #3437
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
This PR adds visibility into which CodeQL feature flags were queried during an action step and what values they evaluated to, and includes that data in status reports.
Changes:
- Extend
Featuresto record every queried feature flag and its resolved value. - Extend
createStatusReportBaseto accept queried feature outcomes and emit them asfeature_flagsin status reports. - Plumb
Features(orundefined) through action entrypoints and update unit tests accordingly.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/feature-flags.ts | Record queried feature flags and expose them for reporting. |
| src/feature-flags.test.ts | Add coverage to verify queried feature flags are recorded. |
| src/status-report.ts | Add feature_flags to the status report payload and accept queried feature data. |
| src/status-report.test.ts | Update tests for the new createStatusReportBase signature and payload. |
| src/analyze-action.ts | Pass queried feature information into status reports where available. |
| src/init-action.ts | Pass queried feature information into status reports where available. |
| src/init-action-post.ts | Pass queried feature information into status reports where available. |
| src/setup-codeql-action.ts | Pass queried feature information into status reports where available. |
| src/upload-sarif-action.ts | Pass queried feature information into status reports where available. |
| src/autobuild-action.ts | Update status report creation call signature (no features tracked in this step). |
| src/resolve-environment-action.ts | Update status report creation call signature (no features tracked in this step). |
| src/start-proxy-action.ts | Update status report creation call signature (no features tracked in this step). |
| lib/upload-sarif-action.js | Generated JS output corresponding to TS changes. |
| lib/start-proxy-action.js | Generated JS output corresponding to TS changes. |
| lib/setup-codeql-action.js | Generated JS output corresponding to TS changes. |
| lib/resolve-environment-action.js | Generated JS output corresponding to TS changes. |
| lib/init-action.js | Generated JS output corresponding to TS changes. |
| lib/init-action-post.js | Generated JS output corresponding to TS changes. |
| lib/autobuild-action.js | Generated JS output corresponding to TS changes. |
| lib/analyze-action.js | Generated JS output corresponding to TS changes. |
Comments suppressed due to low confidence (1)
src/status-report.ts:646
sendUnhandledErrorStatusReportpasses{}forqueriedFeatures, which will currently reportfeature_flags: []even though this path has noFeaturesinstance. To avoid misrepresenting availability, passundefinedhere (and/or rely oncreateStatusReportBaseomitting the field whenqueriedFeaturesisundefined).
const statusReport = await createStatusReportBase(
actionName,
"failure",
actionStartedAt,
undefined,
{},
undefined,
henrymercer
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.
High-level comment: What do you think about reporting all the feature flags, with whether they were queried or not being an attribute? Something like:
{ "feature_name": { enabled: true, queried: false }}
I ask because currently we don't have feature flag status in Kusto, and it is useful to be able to filter based on feature values even if those features weren't queried in that Action directly. For example, a feature that impacts analyze, but is queried in init.
|
I think those are essentially two separate points:
The approach I went with here is the most conservative in terms of not really performing any extra work and only reporting what was current for a given step, while getting the actual value of a FF for a given step. |
I don't think this is true. There are a bunch of flags that we get the value of in
We save the feature flags API response to disk. If there's overhead in getting the value of environment variables, we could cache those too since feature flags should be stable across a job.
That's fair — while this is a bad pattern, it's very possible that there are workflows out there that do this. |
It is useful to know which feature flags are affecting a particular CodeQL workflow. Although this is typically determined by querying the
/featuresAPI, it can also be influenced by environment variables, CLI versions, etc.This PR modifies the
Featuresclass to track which feature flags are queried during a particular step and what they evaluated to. This data is then included in status reports when possible.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.upload-sarifaction.Environments:
github.comand/or GitHub Enterprise Cloud with Data Residency.How did/will you validate this change?
.test.tsfiles).pr-checks).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