Add workflow to release upgradeable package #6015
Conversation
|
WalkthroughIntroduces a new GitHub Actions workflow that automates the release process for upgradeable OpenZeppelin contracts. The workflow triggers manually via workflow_dispatch, validates that the upgradeable commit references the correct vanilla commit, reads the version from package.json, determines appropriate npm and git tags based on semantic versioning, publishes the package to npm with the correct tag, and creates a GitHub Release with accompanying release notes. The workflow includes environment variable configuration and security measures for token handling. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/release-upgradeable.yml (3)
12-12: Update UPGRADEABLE_REPO to production repository before merging.Line 12 explicitly points to a test fork (
james-toussaint/openzeppelin-contracts-upgradeable) with a TODO comment. This must be updated to the canonical OpenZeppelin repository.Apply this diff:
- UPGRADEABLE_REPO: james-toussaint/openzeppelin-contracts-upgradeable # TODO: Update repo before merging + UPGRADEABLE_REPO: OpenZeppelin/openzeppelin-contracts-upgradeableThis aligns with the past review suggestion.
70-70: Fix release notes tag reference—dependency on test-only block.Line 70 uses
${OLD_GIT_TAG}to fetch release notes from the vanilla repo. However,OLD_GIT_TAGis only defined within the test block at line 47. When the test block is removed (per the previous comment), this variable will be undefined, and the release creation will fail.Replace
OLD_GIT_TAGwithGIT_TAG(after the test block is removed):- --notes="$(gh release view ${OLD_GIT_TAG} --repo="${VANILLA_REPO}" --json body -q .body)" `#TODO: Update tag before merging` \ + --notes="$(gh release view ${GIT_TAG} --repo="${VANILLA_REPO}" --json body -q .body)" \This aligns with the past review suggestion and resolves the SC2034 warning about
OLD_GIT_TAGbeing unused.
45-51: Remove test/development block before merging.Lines 45–51 contain temporary modifications for testing (timestamped versions, custom package scope, modified package.json) and are explicitly marked TODO: Remove block before merging. This code must not ship to production.
Remove this entire block:
- ### [START BLOCK] TODO: Remove block before merging - TIMESTAMPED_VERSION="${VERSION}-$(date +%s)" - OLD_GIT_TAG="${GIT_TAG}" - GIT_TAG="${GIT_TAG}-$(date +%s)" # incremental git tag for testing - sed -i'' -e 's/openzeppelin\/contracts-upgradeable/james-toussaint\/contracts-upgradeable/g' contracts/package.json # custom scope for testing - sed -i'' -e "s/${VERSION}/${TIMESTAMPED_VERSION}/g" contracts/package.json && head contracts/package.json # incremental npm package version for testing - ### [END BLOCK]This removal will cascade to fixing line 70 (see next comment).
🧹 Nitpick comments (1)
.github/workflows/release-upgradeable.yml (1)
52-62: Address shellcheck quoting warnings.Static analysis flags several SC2086 warnings (double quote to prevent globbing and word splitting) at lines 52–62. While these are typically informational, adding quotes around variable expansions improves robustness:
Apply quotes for consistency and safety:
- npm ci - bash scripts/git-user-config.sh - git tag -m {,}${GIT_TAG} - CI=true git push origin tag ${GIT_TAG} + npm ci + bash scripts/git-user-config.sh + git tag -m {,}"${GIT_TAG}" + CI=true git push origin tag "${GIT_TAG}"Also add quotes around the cd command context:
- cd contracts + cd "contracts"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release-upgradeable.yml(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: lint workflows
.github/workflows/release-upgradeable.yml
[error] 20-20: shellcheck reported issue in this script: SC2086:info:1:56: Double quote to prevent globbing and word splitting [shellcheck]
🪛 GitHub Check: lint
.github/workflows/release-upgradeable.yml
[failure] 66-66:
shellcheck reported issue in this script: SC2086:info:5:3: Double quote to prevent globbing and word splitting
[failure] 66-66:
shellcheck reported issue in this script: SC2086:info:4:30: Double quote to prevent globbing and word splitting
[failure] 30-30:
shellcheck reported issue in this script: SC2086:info:30:30: Double quote to prevent globbing and word splitting
[failure] 30-30:
shellcheck reported issue in this script: SC2086:info:25:29: Double quote to prevent globbing and word splitting
[failure] 30-30:
shellcheck reported issue in this script: SC2086:info:24:15: Double quote to prevent globbing and word splitting
[failure] 30-30:
shellcheck reported issue in this script: SC2034:warning:17:1: OLD_GIT_TAG appears unused. Verify use (or export if used externally)
[failure] 30-30:
shellcheck reported issue in this script: SC2086:info:14:78: Double quote to prevent globbing and word splitting
[failure] 30-30:
shellcheck reported issue in this script: SC2091:warning:1:6: Remove surrounding $() to avoid executing output (or use eval if intentional)
[failure] 20-20:
shellcheck reported issue in this script: SC2086:info:1:56: Double quote to prevent globbing and word splitting
🔇 Additional comments (1)
.github/workflows/release-upgradeable.yml (1)
63-71: Confirm environment and token handling post-fixes.The GitHub release step uses
GH_TOKEN_UPGRADEABLEfor authentication. Once the test block and tag references are corrected, verify that the token and repository context are correctly scoped.After applying the above fixes, validate:
- The workflow runs end-to-end without errors
GIT_TAGis correctly resolved in the release creation step- Release notes are fetched from the vanilla repo using the correct tag
This will confirm no side effects from the test-block removal.
| env: | ||
| NPM_TOKEN: ${{ secrets.NPM_TOKEN }} |
There was a problem hiding this comment.
We wouldn't need the NPM_TOKEN if we're using trusted publishers. See #6009
I just added the trusted publisher for the upgradeable package:
| env: | |
| NPM_TOKEN: ${{ secrets.NPM_TOKEN }} |
Also, I think this would fail because the NPM_TOKEN is only available in the npm environment
There was a problem hiding this comment.
NPM_TOKEN has been removed.
Right now package.json url repository needs to be updated from upgradeable to vanilla to comply with provenance:
| run: | | ||
| gh release create "${GIT_TAG}" \ | ||
| --repo="${UPGRADEABLE_REPO}" \ | ||
| --title="${GIT_TAG}" \ | ||
| --notes="$(gh release view "${OLD_GIT_TAG}" --repo="${VANILLA_REPO}" --json body -q .body)" `# TODO: Update tag before merging` \ | ||
| "${ADDITIONAL_OPTION_IF_PRERELEASE}" |
There was a problem hiding this comment.
We need to document that the release will not include a customized readme and that we should copy paste the one from vanilla. Ideally, it would copy it directly from vanilla but that's too overkill imo
There was a problem hiding this comment.
Content is copied from vanilla release, see --notes.
| VERSION="$(jq -r .version package.json)" | ||
| GIT_TAG="v${VERSION}" | ||
| NPM_TAG="tmp" | ||
| ADDITIONAL_OPTION_IF_PRERELEASE="--prerelease" | ||
| if [[ "${GIT_TAG}" =~ ^v[0-9]+\.[0-9]+\.[0-9]+$ ]]; then | ||
| NPM_TAG="dev" | ||
| ADDITIONAL_OPTION_IF_PRERELEASE="" | ||
| elif [[ "${GIT_TAG}" =~ ^v[0-9]+\.[0-9]+\.[0-9]+-rc.[0-9]+$ ]]; then | ||
| NPM_TAG="next" | ||
| fi |
There was a problem hiding this comment.
In the vanilla release cycle workflow we pack first and then we publish. This allows to gather environmental values as output from the job (see the pack script). I would suggest doing the same here so that we can isolate the npm publish command and also run an integrity check
There was a problem hiding this comment.
Steps have been scoped, lmk.
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
| VANILLA_REPO: OpenZeppelin/openzeppelin-contracts | ||
| UPGRADEABLE_REPO: james-toussaint/openzeppelin-contracts-upgradeable # TODO: Update repo before merging | ||
| steps: | ||
| - run: echo "UPGRADEABLE_DIR=${GITHUB_WORKSPACE}/upgradeable" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
I would pass upgradeable as a workflow input (or anothe env variable along with VANILLA_REPO and UPGRADEABLE_REPO). Essentially:
on:
workflow_dispatch:
inputs:
upgradeable-path:
description: 'The path where the upgradeable repository will be cloned under $GITHUB_WORKSPACE'
required: true
type: string
default: 'upgradeable'So that here we can do:
| - run: echo "UPGRADEABLE_DIR=${GITHUB_WORKSPACE}/upgradeable" >> "$GITHUB_ENV" | |
| - run: echo "UPGRADEABLE_DIR=${GITHUB_WORKSPACE}/${{ github.event.inputs.upgradeable-path }}" >> "$GITHUB_ENV" |
And:
- uses: actions/checkout@v5
with:
repository: ${{ env.UPGRADEABLE_REPO }}
submodules: true
token: ${{ secrets.GH_TOKEN_UPGRADEABLE }}
ref: ${{ github.ref }}
path: ${{ github.event.inputs.upgradeable-path }}This way we keep the value consistent across where it's used
There was a problem hiding this comment.
Actually, on a second thought I think the path should be provided for the vanilla repository instead, this way we can echo "commit=$(git rev-parse --short HEAD)" >> "$GITHUB_OUTPUT" && cd "${UPGRADEABLE_DIR}", then just continue normally.
There was a problem hiding this comment.
See Use vanilla dir.
3 participants
Releases upgradeable package with actionable workflow.
PR Checklist