chores: assorted fixes for release.yaml
#3134
Conversation
WalkthroughThis PR renames and normalizes release workflow inputs and env vars: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2024-09-26T21:14:56.813ZApplied to files:
📚 Learning: 2025-10-09T20:16:27.834ZApplied to files:
⏰ 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). (10)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@shichengripple001 would you mind taking a look at these changes to make sure nothing will break? Thanks! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release.yml (1)
25-25: Critical: Job output key mismatch will cause downstream failures.Line 25 defines the job output as
package_version: ${{ steps.get_version.outputs.version }}, but line 119 writespackage_version=${VERSION}to$GITHUB_OUTPUT. This creates a mismatch where the step outputs keypackage_versionbut the job output definition tries to read keyversion, causing all downstream jobs that referenceneeds.get_version.outputs.package_versionto receive empty/null values.Line 25 must be updated to reference the correct output key:
outputs: - package_version: ${{ steps.get_version.outputs.version }} + package_version: ${{ steps.get_version.outputs.package_version }}Also applies to: 119-119
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
74-77: Consolidate duplicated DIST_TAG derivation and validation logic.The logic to derive
DIST_TAGfromNPM_DIST_TAGand validate it appears in three separate locations:
- Lines 74–77 in the "Validate inputs" step
- Lines 111–114 in the "Get package version" step
- Lines 590–593 in the "Publish to npm" step
Each instance also includes separate validation (lines 81–90, 115–118, 594–597). This duplication introduces maintenance risk and cognitive load. Consider extracting this into a shared reusable workflow, a composite action, or a documented helper function to ensure consistent validation across all publish paths.
Also applies to: 111-114, 590-593
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml(16 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-09-26T21:14:56.813Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2788
File: .github/workflows/nodejs.yml:25-25
Timestamp: 2024-09-26T21:14:56.813Z
Learning: In `.github/workflows/nodejs.yml`, the `build-and-lint` job's actions `actions/setup-node` and `actions/cache` are updated to `v4`.
Applied to files:
.github/workflows/release.yml
📚 Learning: 2025-10-09T20:16:27.834Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3101
File: .github/workflows/nodejs.yml:220-221
Timestamp: 2025-10-09T20:16:27.834Z
Learning: In `.github/workflows/nodejs.yml`, the `generate-documentation` job intentionally uses `if: startsWith(github.ref, 'refs/tags/xrpl@')` instead of `env.GIT_REF` to ensure documentation is only generated and deployed on actual tag push events, not when the workflow is called via `workflow_call` with a tag reference.
Applied to files:
.github/workflows/release.yml
⏰ 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). (9)
- GitHub Check: integration (20.x)
- GitHub Check: integration (24.x)
- GitHub Check: browser (24.x)
- GitHub Check: integration (22.x)
- GitHub Check: unit (22.x)
- GitHub Check: unit (24.x)
- GitHub Check: unit (20.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
.github/workflows/release.yml (2)
148-149: Standardized variable naming applied consistently across jobs.The
PKG_NAMEandPKG_VERSIONenvironment variables are set correctly in all affected jobs (pre_release,ask_for_dev_team_review,release) and used consistently throughout downstream steps. This improves clarity and reduces confusion compared to the previous mixed naming convention.Also applies to: 314-315, 554-555
32-92: Input validation for PKG_NAME is robust and well-structured.The validation logic correctly guards against:
- Invalid characters in package name (line 50–53)
- Path traversal attacks via
..or/(line 55–58)- Invalid npm dist-tag format (lines 81–90)
Defaulting to
latestwhen npmjs_dist_tag is empty (lines 74–77) is sensible and documented. The validation flow is clear and provides specific error messages for each failure mode.
| PACKAGE_NAME_INPUT: ${{ github.event.inputs.package_name }} | ||
| NPMJS_DIST_TAG_INPUT: ${{ github.event.inputs.npmjs_dist_tag }} | ||
| PKG_NAME: ${{ github.event.inputs.package_name }} | ||
| NPM_DIST_TAG: ${{ github.event.inputs.npmjs_dist_tag }} |
There was a problem hiding this comment.
You can just name this DIST_TAG directly, since in the script below you call DIST_TAG="${NPM_DIST_TAG}" and don't use NPM_DIST_TAG for anything else.
There was a problem hiding this comment.
In line 76 we reassign the value to latest which I believe is not allowed for env variable, that's why we need an intermediate value. However, this is only used for printing out messages so we can use something like ${NPM_DIST_TAG:-latest}. Let me know what you think
There was a problem hiding this comment.
In line 76 we reassign the value to latest which I believe is not allowed for env variable, that's why we need an intermediate value.
Are you saying that if ${{ github.event.inputs.npmjs_dist_tag }} is "latest" then GitHub will not execute the workflow?
Assuming you'd rename NPM_DIST_TAG to DIST_TAG here in the env: section, then below you can just do:
# Empty → default to 'latest'
if [ -z "${DIST_TAG}" ]; then
DIST_TAG="latest"
echo "ℹ️ npmjs_dist_tag empty → defaulting to 'latest'."
fi
no? I've never heard of the value "latest" not being allowed as an env var, that would be very odd. (then again, Gmail doesn't allow you to use a label "Finance", so sometimes things one sees in the world are just strange).
There was a problem hiding this comment.
Actually, you are right, it's just a good practice not to reassign env variable since it is shared within the job, but you can certainly do so. Also, in our case it does not affect anything since these steps are at the end of the job. However, since this is pretty minor, I would prefer to keep it as is just to follow good practice
There was a problem hiding this comment.
That's fair. I don't know if next steps will use the original or the modified env var.
There was a problem hiding this comment.
btw you can also just do:
DIST_TAG: ${{ github.event.inputs.npmjs_dist_tag || "latest" }}
and then you don't need the check in the script.
Great catch! Fixed in a later commit |
| PACKAGE_NAME_INPUT: ${{ github.event.inputs.package_name }} | ||
| NPMJS_DIST_TAG_INPUT: ${{ github.event.inputs.npmjs_dist_tag }} | ||
| PKG_NAME: ${{ github.event.inputs.package_name }} | ||
| NPM_DIST_TAG: ${{ github.event.inputs.npmjs_dist_tag }} |
There was a problem hiding this comment.
btw you can also just do:
DIST_TAG: ${{ github.event.inputs.npmjs_dist_tag || "latest" }}
and then you don't need the check in the script.
High Level Overview of Change
Address some outstanding comments in #3133.
Changes include:
PKG_NAME,PKG_VERSION, andNPM_DIST_TAGconsistently within the script and remove unnecessary intermediate variables.$GITHUB_OUTPUTinstead of$GITHUB_ENVto be used in other jobs (instead of the same job).${{ needs.get_version.outputs.package_name }}directly since this should be available and && operator does not make much sense in this context.Type of Change
Did you update HISTORY.md?