ci: attach release assets before publishing and verify the set - #237
ci: attach release assets before publishing and verify the set#237re-gius wants to merge 3 commits into
Conversation
CI Summary
Labelsother, P0, type: docs, type: bug |
re-gius
changed the title
re-gius
changed the title
Superseded by an inline review referencing the specific lines.
There was a problem hiding this comment.
Nice work, the draft, verify, publish order is the right call for immutable releases. Reading the version through an env var and checking it before use keeps injection out, and building the draft only after forge test passes is spot on.
One thing to sort before merge: the concurrency key does not quite do what its comment says, detail is on the line. Could we also prove it in CI on this PR rather than a manual dispatch? Wire the beta workflow to run against this branch so it cuts a v0.5.5-rc pre-release here, then we confirm from the run that the pre-release has every ABI plus the zip, it published cleanly, and creating the tag did not start a second release run. The rest of the inline notes are small hardening bits, not blockers.
| # Two runs for the same version would race to attach assets to the same draft. On a | ||
| # dispatch `github.ref` is the branch, so key on the requested version instead. | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ inputs.version || github.ref }} |
There was a problem hiding this comment.
Blocking: this does not serialise the two triggers for the same version, which is what the comment above it promises. On a tag push inputs.version is empty, so the group is ...-refs/tags/v0.5.5; on a dispatch it is ...-v0.5.5. The two strings differ, so a dispatch and a tag push for the same version are not mutually exclusive: both clear the pre-published guard while neither is published, both build, both attach to the same draft tag, and both reach publish, where immutable releases fail the second one or the asset sets race. Use github.ref_name instead of github.ref. On a tag push it is the tag v0.5.5; on a dispatch inputs.version wins. Both then collapse to ...-v0.5.5.
| # Two runs for the same version would race to attach assets to the same draft. On a | ||
| # dispatch `github.ref` is the branch, so key on the requested version instead. | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ inputs.version || github.ref }} |
There was a problem hiding this comment.
Same blocking issue as the release workflow. Switch github.ref to github.ref_name so a dispatch and a tag push for the same pre-release version share one concurrency group.
| else | ||
| TAG="$GITHUB_REF_NAME" | ||
| fi | ||
| if [[ ! "$TAG" =~ ^v[0-9]+\.[0-9]+\.[0-9]+-.+$ ]]; then |
There was a problem hiding this comment.
-.+$ accepts anything after the hyphen. Not exploitable, since $RELEASE_TAG is quoted everywhere downstream and a newline cannot reach $GITHUB_ENV through the anchored match, but it lets junk suffixes through. Tighten to -[0-9A-Za-z.]+$.
| ) | ||
|
|
||
| for name in "${contracts[@]}"; do | ||
| while IFS= read -r name || [ -n "$name" ]; do |
There was a problem hiding this comment.
Hardening: a CRLF-saved abi-contracts.txt puts a trailing carriage return in name, breaks the artifact path, and fails the release. Add a pre-commit hook that rejects CR line endings in this file so a bad checkout cannot reach the workflow. Pinning it to LF in .gitattributes covers the checkout side too, and a run-loop strip (name="${name%$'\r'}") is a last resort, but the hook stops it at the source.
|
|
||
| Do not create releases through the GitHub UI's release form, or with `gh release create`. Both publish immediately, and because this repository has immutable releases enabled, a published release can no longer accept assets: only its title and notes stay editable. A release made that way carries no ABIs at all. The workflow rejects an already-published version before building, so the mistake fails in seconds rather than silently shipping an empty release. | ||
|
|
||
| If a run fails partway, re-run it from the Actions tab; the draft is updated rather than duplicated. If the version has already been published, use a different one, since its assets cannot be changed. |
There was a problem hiding this comment.
softprops never deletes assets, so this holds only when the asset set is unchanged. Remove a contract from abi-contracts.txt and re-run against an existing draft, and the stale asset survives; the verify step then refuses to publish, which is correct but not what this line implies. Add a note to delete the draft first when the contract list changed.
|
Could we also please prove it in CI on this PR rather than a manual dispatch? Wire the beta workflow to run against this branch so it cuts a v0.5.5-rc pre-release automatically here, and we read the result straight from the PR run: the pre-release has every ABI plus the zip, it published cleanly, and creating the tag did not start a second release run. Both workflows share the fix, so a green pre-release covers the real release too. Thanks for putting this together! |
2 participants
Description
Releases were occasionally. published without ABI assets because assets were being attached to an already-published release. This repository has immutable releases enabled, so GitHub refuses that upload. Both publish workflows now create the release as a draft, verify its asset set, and publish only afterwards, with a guard that rejects a pre-published version before the build rather than after it. Releases are also cut from the Actions tab with a version input, so nobody has to create a tag or a release by hand.
Type
Scope
Related Issues
Part of #222 — the release-workflow half. The SDK half (
dotns-sdk'sscripts/sync-abis.mjskeeping stale ABIs when assets are missing) is a separate PR in that repository.Unblocks #118, which cannot attach
deployments.jsonas a release asset until the publication order is fixed. Downstream of that, #221 and #223 stop transcribing addresses by handFixes
Changes, applied to both
publish-release.ymlandpublish-prerelease.yml:.github/abi-contracts.txtand writesrelease/expected-assets.txtrecording what the build produced.gh release edit --draft=false, so publication happens only after verification.softpropskeepsdraft: true.workflow_dispatchwith aversioninput, alongside the existing tag-push trigger. AResolve release tagstep takes the version from either source, validates it against the same pattern as the trigger, and exportsRELEASE_TAGfor the downstream steps. The input is read through an env var rather than interpolated into the script.inputs.version || github.ref, becausegithub.refis the branch on a dispatch and two versions from one branch would otherwise queue behind each other..github/abi-contracts.txt. The 32-name list was duplicated verbatim in both workflows, so a new contract had to be added twice or the release silently omitted it, and the verification step would have needed a third copy to check against. It now lives in one file that both read, and verification compares against what the build actually emitted rather than against a list.See #222 for more context.
Checklist
Code
forge buildpassesforge testpassesTesting
Security
selfdestructordelegatecallDocumentation
Breaking Changes
Breaking changes:
Breaking changes: no API or contract change, but the release process changes. Releases can no longer be created through the GitHub UI's release form or with
gh release create, because doing so publishes immediately and the workflow can then no longer attach assets. Cutting a release is now either "Publish Release Package" from the Actions tab with a version input, or pushing a matching tag; both run the same workflow, which creates, verifies and publishes. The guard turns the old habit into an immediate failure.How to test
Dispatch the workflow from the Actions tab.
Not yet exercised end to end. It should be tested on a fork, not here.
Notes
A release from current master is still needed.