Record the job that published an overlay status#3537
Record the job that published an overlay status#3537henrymercer wants to merge 2 commits intomainfrom
Conversation
This makes it easier to find the job that produced the status.
There was a problem hiding this comment.
Pull request overview
This PR adds job identification metadata to the overlay analysis status file stored in the GitHub Actions cache. When an overlay-base analysis fails to complete, the action now records the workflow run ID, run attempt number, and job name alongside the failure status, making it easier to trace which job produced the status.
Changes:
- New
JobInfointerface and optionaljobfield inOverlayStatus, plus acreateOverlayStatusfactory function that auto-populates job details from environment variables - Refactored
recordOverlayStatusin the post-helper to usecreateOverlayStatusinstead of an object literal - Updated tests to set the new required env vars and assert that job details are included in the saved overlay status
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/overlay/status.ts |
Defines JobInfo interface, adds optional job field to OverlayStatus, adds createOverlayStatus factory |
src/init-action-post-helper.ts |
Uses new createOverlayStatus helper when recording overlay failure status |
src/init-action-post-helper.test.ts |
Sets GITHUB_RUN_ID, GITHUB_RUN_ATTEMPT, GITHUB_JOB env vars and asserts job info in saved status |
lib/init-action-post.js |
Auto-generated JS reflecting the TS source changes (not reviewed per convention) |
mbg
left a comment
mbg
left a comment
There was a problem hiding this comment.
Looks good. Just one small suggestion, but it's not really specific to this PR and would be out-of-place as a change here.
| process.env["GITHUB_REPOSITORY"] = "github/codeql-action-fake-repository"; | ||
| process.env["GITHUB_RUN_ID"] = "12345"; | ||
| process.env["GITHUB_RUN_ATTEMPT"] = "1"; | ||
| process.env["GITHUB_JOB"] = "analyze"; | ||
| process.env["RUNNER_TEMP"] = tmpDir; |
There was a problem hiding this comment.
Minor: It might be nice to extend setupActionsVars with all environment variables that we'd typically expect in an Actions environment and modify it to accept an argument with a (partial) mapping of specific environment variables we want to set. Not necessarily something for this PR.
There was a problem hiding this comment.
Good idea. I'll take a look at this in a separate PR.
| workflowRunId: getWorkflowRunID(), | ||
| workflowRunAttempt: getWorkflowRunAttempt(), | ||
| name: getRequiredEnvParam("GITHUB_JOB"), | ||
| ...(checkRunId !== undefined && { checkRunId }), |
There was a problem hiding this comment.
I think we had a similar point on another PR recently: this is mostly equivalent to just checkRunId, except that with this the JobInfo object doesn't even contain the key? Do you think it's beneficial to explicitly not have the key, rather than it being set to undefined here? If so, it would be good to document why. Otherwise, change it to just checkRunId.
| attemptedToBuildOverlayBaseDatabase: true, | ||
| builtOverlayBaseDatabase: false, | ||
| }, | ||
| Number.isNaN(checkRunId) ? undefined : checkRunId, |
There was a problem hiding this comment.
Should we also check that this is >= 0 (or similar)?
This makes it easier to find the job that produced the status.
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