[paul-hristea]

Update all GitHub workflow actions to latest versions.

Fixes #485

[github-actions]

Test Results

129 files  ±0  129 suites  ±0   4m 11s ⏱️ + 1m 47s
737 tests ±0  737 ✅ ±0  0 💤 ±0  0 ❌ ±0 
827 runs  ±0  821 ✅ ±0  6 💤 ±0  0 ❌ ±0 

Results for commit 0d867b9. ± Comparison against base commit 6d348a0.

♻️ This comment has been updated with latest results.

[kryskool]

Hi @paul-hristea

to prevent supply chain attack, please use commit instead of tag

Pinning GitHub Actions to a full-length commit SHA instead of a mutable version tag (@v4) prevents supply chain attacks. Version tags can be silently moved or re-tagged by repository maintainers or attackers to point to malicious code, whereas a commit SHA is immutable

https://www.stepsecurity.io/blog/pinning-github-actions-for-enhanced-security-a-complete-guide

Regards,

[kryskool]

Use commit instead of tag

[tonygermano]

Pinning GitHub Actions is good advice. The document shared by @kryskool also shows how Renovate (which we already use) can be used to keep these pinned actions up to date.

[paul-hristea]

Pinned to current latest version hashes. Thanks for the suggestion!

Edit: Also added Renovate config entry for github-actions, with helpers:pinGitHubActionDigests to keep the same version pinning strategy (see docs)

[kryskool]

Now it's ok for me, thanks @paul-hristea

[tonygermano]

Thanks for switching to digest pins. Claude checked each SHA against the upstream tags: they're correct and resolve to checkout v7.0.1, setup-buildx-action v4.4.1, ghaction-github-runtime v4.0.0, upload-artifact v7.0.2, download-artifact v8.0.2 and publish-unit-test-result-action v2.25.0.

Requested changes:

• Please add the version as a trailing comment on each pinned line, e.g. uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1. Renovate reads that comment to find the current version. Without it, Renovate can't offer version updates for these pins, and reviewers can't tell what each SHA is. See Renovate docs for more info.
• The commit message is missing a blank line after the subject, so both sentences end up in the subject line. Since this is a major-version bump for most of these actions (checkout v4→v7, upload-artifact v4→v7, download-artifact v4→v8, buildx/runtime v3→v4), please say that in the commit body and the PR description.

Notes, no change needed:

• upload_test_results.yaml runs on workflow_run, which always uses the workflow file from the default branch. The test-results comment on this PR therefore came from download-artifact@v4, and the v8 change will first run after merge. v8 fails on digest mismatches and skips decompressing non-zip files by default. I don't expect either to affect us, but we should watch the first run on main.
• Is github an existing label, or did you mean github-actions?

---

Drafted with Claude Code

[paul-hristea]

Changes applied.

upload_test_results.yaml runs on workflow_run, which always uses the workflow file from the default branch. The test-results comment on this PR therefore came from download-artifact@v4, and the v8 change will first run after merge. v8 fails on digest mismatches and skips decompressing non-zip files by default. I don't expect either to affect us, but we should watch the first run on main.

Meanwhile, I can confirm the forked workflow jobs all run successfully after having merged this into main.

Is github an existing label, or did you mean github-actions?

Renamed the label to github actions to match the formatting of other existing labels (using spaces rather than hyphens). Is that agreeable?

[mgaffigan]

Let's not get too crazy on the review. The PR was not to overhaul GHA - just get up to date.

Unless there is something wrong let's push it to a follow up issue/PR.

[tonygermano]

Thank you, Paul, looks great!

[tonygermano]

Let's not get too crazy on the review. The PR was not to overhaul GHA - just get up to date.

Unless there is something wrong let's push it to a follow up issue/PR.

The missing comments were wrong. Without them Renovate disables upgrades. I don't see the benefit in pushing that through only to need to immediately turn around and submit another PR or miss it entirely.

The commit messages are important. You should be able to tell from the git log the context and intent of a commit without needing to do another code review to infer it, and he was going to be updating the commit, anyway, for the comments. A force update which only changes the commit message and not the content of the PR does not dismiss existing approvals in github.

The other two notes were marked "no change needed."

[mgaffigan]

@tonygermano

The comment was regarding the expansion of "let's get up to date" to:

1. Let's get up to date
2. Let's pin our dependencies
3. Let's add renovate for GHA

each of those things are good and reasonable, but 2 and 3 were not defects of the PR reasonably doing 1.

Enforcing a PR to do a bunch of drive-by improvements discourages opening PRs in the first place, since it creates demoralizing obligations beyond the scope of the change.

The threshold for a PR merging should be "does this move us closer to the goal without new problems". The original change did. The rest could have been follow on.

PR messages fall under "code style" and "comment practices". Squishy things like that need to be enforced by written P&P/linters. They should not rejected unless unambiguously wrong. They create contention and delays without delivering business value.

Happy developers that feel like they are able to get code merged are more likely to return.

[paul-hristea]

No worries on my side. Happy to help! 🤝