-
Notifications
You must be signed in to change notification settings - Fork 1
Add release #43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add release #43
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces an automated release workflow for two npm packages (compact-tools-cli and compact-tools-simulator) with semantic version bumping, testing, and publishing to npm. Package visibility is updated by removing private flags, enabling public distribution. Release documentation guides maintainers through the manual workflow-dispatch trigger and automated steps. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
RELEASING.md (1)
7-8: Use hyphens for compound adjectives.Compound adjectives modifying nouns should be hyphenated. Apply this diff:
- - **Patch** - Backward compatible bug fixes. + - **Patch** - Backward-compatible bug fixes. - - **Minor** - New functionality in a backward compatible way. + - **Minor** - New functionality in a backward-compatible way.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/release.yml(1 hunks)RELEASING.md(1 hunks)packages/cli/package.json(0 hunks)packages/simulator/package.json(0 hunks)
💤 Files with no reviewable changes (2)
- packages/simulator/package.json
- packages/cli/package.json
🧰 Additional context used
🪛 LanguageTool
RELEASING.md
[uncategorized] ~7-~7: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...(https://semver.org/): - Patch - Backward compatible bug fixes. - Minor - New functio...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~8-~8: Use a hyphen to join words.
Context: ...inor** - New functionality in a backward compatible way. - Major - Breakin...
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (5)
.github/workflows/release.yml (5)
63-73: Verify yarn version command syntax.The command at line 67 uses a positional argument, but
yarn versionexpects a named flag. Verify that the correct syntax is used:- yarn version ${{ inputs.version_bump }} + yarn version --${{ inputs.version_bump }}This would produce
yarn version --patch,yarn version --minor, oryarn version --majordepending on the input.
78-83: Confirm git-auto-commit-action pushes tags.The git-auto-commit-action is configured to commit and tag the version bump. Verify that the action also pushes the tag to the remote repository, as the workflow doesn't explicitly call
git pushafter tagging.
60-61: Build artifacts must be committed or available for publishing.The build step (line 60-61) compiles the package, but the subsequent commit (line 78-83) only includes
package.json. Verify that the compiled artifacts (dist, lib, or equivalent) are either:
- Generated at build time and included in the npm package via the
filesfield in package.json, or- Committed to the repository
Without this, publishing may fail or distribute incomplete artifacts.
37-91: LGTM! Workflow structure is sound.The overall workflow design is well-structured:
- Environment approval gate (
compact-npm-prod) provides security oversight for releases- Action versions are pinned to commit hashes (good supply-chain security practice)
- Comprehensive steps: test → build → version → verify → tag → publish
- NPM_TOKEN is correctly isolated in the publish step using
env- The package directory mapping correctly handles multiple packages from a monorepo
- Dry-run pack verification catches issues before publishing
Once the yarn version syntax is confirmed, this should provide a reliable release pipeline.
85-91: All npm publish configuration is correct.The
--provenanceflag is fully supported in Yarn 4.10.3 (which exceeds the required Yarn v4.9.0+), and the flag works correctly in GitHub Actions contexts. Thecompact-npm-prodenvironment is properly configured with approval requirements, and theNPM_TOKENsecret is correctly passed to the publish step.
andrew-fleming
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good @emnul! Just left a question
.github/workflows/checks.yml
Outdated
| permissions: read-all | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be read-all? read is more restrictive unless I'm missing something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're totally right. I misunderstood what read-all does
| @@ -1,6 +1,5 @@ | |||
| { | |||
| "name": "@openzeppelin/compact-tools-cli", | |||
| "private": true, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we should still remove this. Because I think the name of the package will be a bit misleading, I was thinking to to leave this private and if we are we follow the proposal I had regarding copmact-tools, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss this in a separate PR / Issue to avoid blocking it.
| @@ -1,6 +1,5 @@ | |||
| { | |||
| "name": "@openzeppelin/compact-tools-simulator", | |||
| "private": true, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same here, but more specifically for those packages I think its better to remove the -tools- word from the name to be @openzeppelin/compact-simulator wdyt?
son-oz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there's anything else that needs to be added function-wise but LGTM.
Fixes #6. Supersedes #19.
Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.