Fix "Debug artifacts after failure" PR check not checking any artifacts - #4273
henrymercer merged 1 commit into
Conversation
Sorry, something went wrong.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The version list matches the upload matrix and restores existing validation without changing production behavior.
No open issues 🎉
What changed in this PRRestores artifact validation in the “Debug artifacts after failure” PR check, which previously skipped its validation loop because VERSIONS was unset.
Changes:
- Defines VERSIONS to match the upload job’s four-version matrix.
- Enables the existing per-version and per-language artifact checks.
| File | Description |
|---|---|
| .github/workflows/debug-artifacts-failure-safe.yml | Initializes the version list used by the artifact-validation loop. |
Review effort: Max
Sorry, something went wrong.
mbg
left a comment
There was a problem hiding this comment.
Thanks for noticing this and addressing it! I wonder how this happened...
Just one optional suggestion; happy for this to be merged as-is.
Sorry, something went wrong.
| uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 | ||
| - name: Check expected artifacts exist | ||
| run: | | ||
| VERSIONS="stable-v2.20.7 default linked nightly-latest" |
There was a problem hiding this comment.
Minor: Rather than having this as a local variable here, would it be easier to spot/update if it was in an env object for the step/job/workflow?
Sorry, something went wrong.
There was a problem hiding this comment.
I'm not sure that this would be much easier to spot, and it might increase the chance of the variable being unset again.
It would also be nice to have a single source of truth for the versions, but per https://docs.github.com/en/enterprise-cloud@latest/actions/reference/workflows-and-actions/contexts#context-availability we can't use env to define a matrix and would therefore need another job to determine the matrix.
I'll leave this as is for now.
Sorry, something went wrong.
There was a problem hiding this comment.
Would it make sense to migrate these workflows to pr-checks/checks and generate them with sync.ts? Then we could perhaps move towards a single source of truth for the versions.
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, now that we have validationJobs, I think it should be possible to autogenerate these workflows.
Sorry, something went wrong.
The "Check expected artifacts exist" step of the "Debug artifacts after failure" PR check loops over $VERSIONS, but nothing in the workflow sets it. This PR sets VERSIONS to the upload job's matrix versions, as the "Debug artifact upload" check does in debug-artifacts-safe.yml.
Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Environments:
How did/will you validate this change?
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