-
Notifications
You must be signed in to change notification settings - Fork 15
Fix prep release and release #317
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
Fix prep release and release #317
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 WalkthroughUpdates GitHub Actions release workflows to enhance version handling by tolerating prerelease and build suffixes in version strings, and switches npm authentication from provenance-based configuration to token-based authentication using NODE_AUTH_TOKEN. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/prepare-release.yml(1 hunks).github/workflows/release.yml(1 hunks)
⏰ 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). (2)
- GitHub Check: Run Test Suite
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
.github/workflows/release.yml (1)
29-34: Version validation may not account for all suffix scenarios.The strict equality check at line 31 compares the release tag against
package.jsonversion. Given thatprepare-release.ymlnow handles prerelease and build suffixes, confirm that:
- Git tags always include the suffix (e.g.,
v1.2.3-alpha.1) when a suffix exists inpackage.json- The
prepare-releaseworkflow consistently maintains suffixes inpackage.jsonwhile stripping them from other filesIf there's any mismatch in how suffixes are handled between these workflows, the validation will fail.
.github/workflows/prepare-release.yml (2)
68-75: LGTM! Node.js JSON approach is more reliable than yarn version.Direct JSON manipulation is cleaner and avoids the yarn version command issues in shallow clones (as noted in PR objectives). The script correctly preserves package.json formatting with 2-space indentation and trailing newline.
81-82: I encountered a repository access issue. Let me analyze the review comment based on the pattern itself and the semver specification context.The review comment raises a legitimate concern about the VERSION_SUFFIX pattern
(-[A-Za-z0-9.]+)?(\+[A-Za-z0-9.]+)?. Let me verify the validity of this concern logically:Pattern Analysis:
- Pattern requires dots (
.) as separators:[A-Za-z0-9.]- This means formats like
1.2.3-alpha.1would match ✓- But formats like
1.2.3-alpha-1would NOT match (hyphen between "alpha" and "1") ✗- The semver spec (https://semver.org/) allows hyphens within prerelease identifiers separated by dots
Key finding: The review comment's concern is technically valid. The pattern is indeed more restrictive than the full semver spec because it prohibits consecutive hyphens like
alpha-1.However, without access to the actual codebase to verify:
- Whether the project actually uses such formats
- What versions exist in contracts/src/ and docs/
- Whether the sed replacements on lines 86 and 90 handle this correctly
I cannot definitively confirm if this is a critical issue for this specific project.
Actual version formats used by OpenZeppelin projects typically follow
1.2.3-alpha.1(dot-separated) rather than1.2.3-alpha-1(hyphen-separated), which would be matched by the pattern.The review comment is valid but cannot be fully verified without codebase access. The concern stands as written and remains actionable for the developer.
|
@andrew-fleming don't we need to remove |
@emnul we're only going to be publishing the |
| env: | ||
| NPM_CONFIG_PROVENANCE: 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.
| env: | |
| NPM_CONFIG_PROVENANCE: true |
I don't think this is required since npm automatically generates and publishes provenance attestations for packages that use trusted publishers eg GitHub Actions w/ GitHub-hosted runners. However, I can see how it improves code clarity by making it explicit. Up to you if we should keep it
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 mean either the flag or env, right? Personally, I prefer the flag but no strong opinions. I'm good with it as-is so long as it works haha
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.
I'm pretty sure neither are required if trusted publishing is configured in NPM, but I don't have access so I can't tell if it's been configured by IT or not sooooo if it works right now probably best to leave as is.
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.
Mmmm yes, you are correct. Agreed, we can improve this in another PR
emnul
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.
Good eye on the version change!
prepare-release
release
RemoveNPM_CONFIG_PROVENANCE: truebecause it's redundant. The--provenanceflag does the same thingAdd npm token for publishing. npm needs this to verify permissions to publish packages@0xisk @emnul can you confirm everything is set up on our end so we don't need the npm token? I'll remove the token if so. Also, AFAICT we only need one provenance: either the flag or the env. LMK if I'm missing something and we need both