Beta release workflow #3131
Beta release workflow #3131
Conversation
![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
♻️ Duplicate comments (2)
.github/workflows/release.yml (2)
362-362: Use validatedrelease_branchoutput instead of input.Line 362 references
github.event.inputs.release_branch_namedirectly, butneeds.get_version.outputs.release_branchis the validated and canonical value. For consistency with lines 173, 181, and 199 (which all useneeds.get_version.outputs.release_branch), update this reference.Past review comments flagged this, but it appears the fix was not applied.
Apply this diff:
env: PKG_VERSION: "${{ needs.get_version.outputs.package_version }}" PKG_NAME: "${{ github.event.inputs.package_name }}" - RELEASE_BRANCH: "${{ github.event.inputs.release_branch_name }}" + RELEASE_BRANCH: "${{ needs.get_version.outputs.release_branch }}"
684-684:generate-documentationuses incorrectgit_ref.Line 683 uses
git_ref: ${{ github.ref_name }}, which for aworkflow_dispatchevent resolves to the current branch (typicallymain), not the release branch. Documentation must be generated from the released code.The
releasejob does not exporttag_nameas a job output, so the first suggested fix in the original comment will not work. Use the validated release branch instead:generate-documentation: name: Generate and Publish documentation for ${{ needs.get_version.outputs.package_version }} if: ${{ needs.get_version.outputs.is_beta != 'true' }} uses: ./.github/workflows/generate-documentation.yml needs: [get_version, release] permissions: contents: read pages: write id-token: write with: - git_ref: ${{ github.ref_name }} + git_ref: ${{ needs.get_version.outputs.release_branch }}
🧹 Nitpick comments (3)
.github/workflows/release.yml (3)
719-719: Consider pinning shell for consistency with other jobs.While
set -euo pipefailworks on most Ubuntu runners (which default tobash), some jobs in this workflow explicitly pin the shell (e.g.,shell: bash). For consistency and robustness, consider pinning the shell here as well.A past reviewer flagged that
pipefailmay not work on all shells (dash/sh), so explicit pinning reduces ambiguity.Apply this diff:
steps: - name: Notify Slack about workflow failure + shell: bash env: SLACK_TOKEN: ${{ secrets.SLACK_TOKEN }}
186-186: Remove redundantalways()from job conditional.The
always()function is negated by the subsequent conditions. The logic already checksneeds.get_version.result == 'success', which ensures this job only runs ifget_versionsucceeded. Removingalways()clarifies intent.A past reviewer (ckeshava) flagged this same redundancy.
Apply this diff:
- if: ${{ always() && needs.get_version.result == 'success' && (needs.run_faucet_test.result == 'success' || needs.get_version.outputs.is_beta == 'true') && (needs.run_tests.result == 'success' || needs.get_version.outputs.is_beta == 'true') }} + if: ${{ needs.get_version.result == 'success' && (needs.run_faucet_test.result == 'success' || needs.get_version.outputs.is_beta == 'true') && (needs.run_tests.result == 'success' || needs.get_version.outputs.is_beta == 'true') }}Alternatively, apply the same fix to lines 354, 489, 508, and 548 for consistency.
623-638: Tag creation uses-f(force) despite pre-existence check.Lines 631–634 check if the tag already exists and fail if it does. However, lines 637–638 use the
-fflag to force-overwrite the tag and push. The force flag is redundant given the earlier check and could mask issues if the tag somehow exists despite the check.Consider removing the
-fflags for stricter tag management:echo "🔖 Tagging ${TAG}" - git tag -f "${TAG}" - git push origin -f "${TAG}" + git tag "${TAG}" + git push origin "${TAG}"This ensures the tag is created cleanly without force-overwriting, aligning with the pre-existence check.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml(18 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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
📚 Learning: 2025-04-16T15:28:21.204Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2801
File: packages/xrpl/test/wallet/batchSigner.test.ts:0-0
Timestamp: 2025-04-16T15:28:21.204Z
Learning: In the XRPL.js library, hardcoded seeds in test files are acceptable as they don't represent protected data or real funds - they're only used for consistent test behavior.
Applied to files:
.github/workflows/release.yml
📚 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
⏰ 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 (9)
.github/workflows/release.yml (9)
12-33: New input and outputs wired correctly.The new
release_branch_nameinput and the expanded outputs (dist_tag,release_branch,release_pr_number,release_pr_url,is_beta) provide a clean contract for downstream jobs to access validated, canonical values.
41-137: Comprehensive input validation and beta/stable branching logic.The validation step correctly implements the PR objectives: validates inputs, derives
is_betafrom dist-tag semantics, appends "-experimental" suffix for beta releases, enforces release-branch naming for stable releases, and requires PR existence only for stable releases.
159-166: Verify redundantNPM_DIST_TAGassignment.Line 159 reassigns
NPM_DIST_TAGfrom the input with a default fallback, butNPM_DIST_TAGwas already normalized and written toGITHUB_ENVin thevalidate_inputsstep (lines 103–106). A past reviewer flagged similar redundancy.Confirm whether line 159 is necessary or if it should be removed/refactored to avoid duplication.
If this assignment is redundant, remove it:
# Validate package_name if ! [[ "${PKG_NAME}" =~ ^[a-z0-9][a-z0-9-]*$ ]]; then echo "❌ Invalid package_name '${PKG_NAME}' (allowed: [a-z0-9-], must start with alnum)." >&2 exit 1 fi - - NPM_DIST_TAG="${NPM_DIST_TAG:-latest}" if [[ "${IS_BETA:-false}" != "true" ]] && ! [[ "$VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; thenAlternatively, if
NPM_DIST_TAGneeds to be re-evaluated in this step, document why.
160-163: Correct SemVer validation for stable vs. beta releases.The validation correctly enforces x.y.z SemVer for stable releases (
latestdist-tag) while allowing other formats for beta releases. The duplicate check at lines 615–618 in the publish step serves as a defensive safety gate.
91-96: Correct "-experimental" suffix logic for beta releases.The logic correctly appends "-experimental" to non-
latestdist-tags, aligning with the PR objective. ModifyingNPM_DIST_TAGin thevalidate_inputsstep ensures consistency across all downstream uses.
649-650: Correct GitHub release flags for beta vs. stable.The
prereleaseandmake_latestflags are correctly driven by theis_betaoutput: beta releases are marked as prerelease (not latest), and stable releases are marked as latest.
592-621: Correct npm publish validation and flags.The publish step correctly validates that
NPM_DIST_TAGis set and enforces SemVer only for stable releases. The--provenanceflag provides good security posture. The logic allows beta releases with non-SemVer versions while rejecting stable releases with invalid SemVer.
458-485: Slack notifications appropriately contextualized for dev and release workflows.The dev team review message includes reviewer lists, run URLs, and conditionally includes the PR update URL when available. The structure is clear and provides all necessary context for approvers.
112-130: Correct PR validation logic for stable releases.The PR existence check for stable releases correctly uses the GitHub API to validate that an open PR exists from the release branch to
main. The query filters (state=open,base=main,head=owner:branch) are correctly formatted. Beta releases appropriately bypass this check. The error message provides clear guidance.
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)
140-166: Critical: NPM_DIST_TAG not properly propagated from validate_inputs to get_version stepThe
validate_inputsstep modifiesNPM_DIST_TAGby appending "-experimental" for beta releases (line 95) and writes it toGITHUB_ENV(line 104). However, theget_versionstep re-reads from the original input (line 145) instead of the validated environment variable. This causes thedist_tagoutput (line 166) to be incorrect for beta releases—it will lack the "-experimental" suffix.Impact: The
releasejob usesneeds.get_version.outputs.dist_tag(line 594), which publishes to npm with the wrong dist tag.Fix: Use the environment variable instead:
- name: Get package version from package.json id: get_version env: IS_BETA: ${{ steps.validate_inputs.outputs.is_beta }} PKG_NAME: ${{ github.event.inputs.package_name }} - NPM_DIST_TAG: ${{ github.event.inputs.npmjs_dist_tag }} + NPM_DIST_TAG: ${{ env.NPM_DIST_TAG }} run: |Also fix line 362:
RELEASE_BRANCH: "${{ needs.get_version.outputs.release_branch }}"(use output instead of input).Minor: Typo at line 666: "succesfully" → "successfully".
♻️ Duplicate comments (1)
.github/workflows/release.yml (1)
362-362: Use validated output instead of input for consistency.Line 362 still references
github.event.inputs.release_branch_namedirectly, but therelease_branchoutput fromget_versionis the validated canonical value. This comment was marked as addressed in past reviews but the code still contains the issue.env: PKG_VERSION: "${{ needs.get_version.outputs.package_version }}" PKG_NAME: "${{ github.event.inputs.package_name }}" - RELEASE_BRANCH: "${{ github.event.inputs.release_branch_name }}" + RELEASE_BRANCH: "${{ needs.get_version.outputs.release_branch }}"
🧹 Nitpick comments (4)
.github/workflows/release.yml (4)
683-683: Verify:generate-documentationgit_ref should reference the release tag, not branch.Per the learning from PR 3101, the
generate-documentationjob should checkout the actual released tag (not just the release branch) to ensure documentation is generated from the correct commit. Currently, it usesneeds.get_version.outputs.release_branch.The
releasejob creates a tag (line 637) and outputstag_name(line 640), but this output is not exported by the job itself—only by the internal step. Downstream jobs cannot access it.Suggested actions:
- Add
tag_nameas a job-level output in thereleasejob:outputs: tag_name: ${{ steps.create_tag.outputs.tag_name }}
- Update
generate-documentationto use the tag:- git_ref: ${{ needs.get_version.outputs.release_branch }} + git_ref: ${{ needs.release.outputs.tag_name }}Alternatively, if the branch and released tag always point to the same commit, document this assumption.
98-101: Validate release branch for beta releases too.Per the PR description, "Workflow will be triggered from the main branch for both beta and stable releases." Currently, line 98-101 only validates the branch name for stable releases (requires
release-orrelease/prefix). Beta releases skip this check entirely.Consider adding validation to ensure beta releases originate from the
mainbranch:- if [ "$IS_BETA" != "true" ] && [[ ! "${RELEASE_BRANCH,,}" =~ ^release[-/] ]]; then - echo "❌ Release branch '$RELEASE_BRANCH' must start with 'release-' or 'release/' for stable releases." >&2 - exit 1 + if [ "$IS_BETA" = "true" ]; then + if [[ "${RELEASE_BRANCH}" != "main" ]]; then + echo "❌ Beta releases must be triggered from 'main' branch, not '${RELEASE_BRANCH}'." >&2 + exit 1 + fi + else + if [[ ! "${RELEASE_BRANCH,,}" =~ ^release[-/] ]]; then + echo "❌ Release branch '$RELEASE_BRANCH' must start with 'release-' or 'release/' for stable releases." >&2 + exit 1 + fi fi
719-719: Pin shell tobashin notify_failures step.The step uses
set -euo pipefail(line 719), which is bash-specific and will not work withdashorsh. Explicitly specifyshell: bashto ensure the pipefail option is honored:steps: - name: Notify Slack about workflow failure + shell: bash env: SLACK_TOKEN: ${{ secrets.SLACK_TOKEN }}
666-666: Minor: Fix typo in success message.- text="${PKG_NAME} ${PKG_VERSION} has been succesfully released and published to npm.js. Release URL: ${RELEASE_URL}" + text="${PKG_NAME} ${PKG_VERSION} has been successfully released and published to npm.js. Release URL: ${RELEASE_URL}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml(18 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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
📚 Learning: 2025-04-16T15:28:21.204Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2801
File: packages/xrpl/test/wallet/batchSigner.test.ts:0-0
Timestamp: 2025-04-16T15:28:21.204Z
Learning: In the XRPL.js library, hardcoded seeds in test files are acceptable as they don't represent protected data or real funds - they're only used for consistent test behavior.
Applied to files:
.github/workflows/release.yml
📚 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
⏰ 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 (24.x)
- GitHub Check: integration (20.x)
- GitHub Check: browser (24.x)
- GitHub Check: integration (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: unit (24.x)
- GitHub Check: unit (22.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
.github/workflows/release.yml (1)
186-186: Clarify intent ofalways()in conditional gates.Multiple jobs use
if: ${{ always() && condition }}(lines 186, 354, 489, 508, 548). Past review comments suggest removingalways(), but this conflicts with the requirement that "tests are allowed to fail for beta releases."Question: Do you want these jobs to:
- Option A: Proceed even if upstream jobs fail (requires
always()), but gate on conditions like(tests_passed || is_beta)?- Option B: Skip if upstream jobs fail (no
always()), and only run if conditions are met?The current pattern allows beta releases to proceed through gates even when tests fail, which aligns with the PR objectives. However, past reviewers flagged this as potentially incorrect.
Please clarify the intended behavior so the conditionals can be finalized.
Also applies to: 354-354, 489-489, 508-508, 548-548
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
RELEASE.md (3)
62-64: Explicitly referencerelease_branch_nameas the Git ref for tests.Line 62 mentions the workflow "run[s] unit, integration, and faucet tests against the specified Git ref" but doesn't clarify that this ref is the
release_branch_nameinput provided in the previous step. Explicitly naming it would improve clarity.-Triggers the `faucet_test.yml` and `nodejs.yml` workflows to run unit, integration, and faucet tests against the specified Git ref. +Triggers the `faucet_test.yml` and `nodejs.yml` workflows to run unit, integration, and faucet tests against the release branch (`release_branch_name`).
48-48: Make "pause at specific steps" more explicit.Line 48 states the pipeline will "pause at specific steps" but doesn't enumerate them. Based on the PR context, this should explicitly mention the steps where manual review is required (e.g., before final release, after security scan).
-The pipeline will pause at the "Print Test/Security scan result and invite Dev team to review" step and also before the final release step, relevant team should review the release details and scan result. +The pipeline pauses at two points for manual review: (1) after the security scan results are published, and (2) before the final npm publish and GitHub release. Teams must approve before proceeding to each next stage.
1-111: Document the newgenerate-documentationandnotify_failuresjobs.The PR introduces two new top-level jobs (
generate-documentationandnotify_failures) as part of the beta release workflow restructuring, but they are not mentioned in this guide. Users should understand when and how these jobs run.Add a new section (after the "Release Stage" section or within "How the Pipeline Works") briefly describing these jobs:
+### 6. **Post-Release Steps** +- **generate-documentation**: Generates and deploys package documentation (runs on tag push events). +- **notify_failures**: Notifies relevant teams on Slack if any prior stage fails.Confirm the exact triggering conditions for
generate-documentation(e.g., does it run for both stable and beta, or only on certain dist tags?). Based on retrieved learnings, documentation generation should only occur on actual tag push events, not when called viaworkflow_call. Ensure this behavior is documented or clarified if the new job changes it.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
RELEASE.md(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
⏰ 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). (8)
- GitHub Check: integration (24.x)
- GitHub Check: integration (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: unit (20.x)
- GitHub Check: browser (24.x)
- GitHub Check: unit (22.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
RELEASE.md (1)
27-29: Clarify the relationship between triggering location andrelease_branch_nameinput.The documentation states the workflow must be triggered from
main, then asks users to providerelease_branch_name. This could be clearer—specifically:
- Does "triggered from main" mean the workflow_dispatch UI is accessed on the main branch, or does the workflow checkout
release_branch_nameregardless of the current branch?- For stable releases with a merged PR, should
release_branch_nameequal the now-merged release branch name ormainitself?Please clarify the operational flow and update lines 27-29 to explain whether users need to switch branches before triggering, or if the workflow independently checks out the specified
release_branch_name.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
RELEASE.md(3 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
RELEASE.md
13-13: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
26-26: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
⏰ 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). (6)
- GitHub Check: integration (24.x)
- GitHub Check: browser (24.x)
- GitHub Check: integration (20.x)
- GitHub Check: unit (20.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (4)
RELEASE.md (4)
13-36: Documentation properly reflects beta vs stable release workflows.The documentation clearly separates and documents the distinct requirements for stable and beta releases, including branch naming conventions, versioning, and distribution tag handling. The beta release guidance around the "-experimental" suffix and flexible branch naming aligns with the PR objectives.
40-56: Release trigger section correctly documents newrelease_branch_nameinput.The triggering instructions now document the
release_branch_nameparameter alongside existing inputs, and the example table is updated to reflect this change. The note that the workflow must be triggered frommainaligns with the PR objectives.
61-61: Review ownership documentation properly differentiates stable vs beta paths.Line 61 correctly documents that stable releases are reviewed by the infosec team, while beta releases are reviewed by security champions from the Dev team. This aligns with the PR objectives and stakeholder changes in the release process.
77-77: Test failure guidance properly reflects beta release allowances.The note that tests are allowed to fail for beta releases aligns with the PR objectives and provides necessary guidance for release managers conducting beta releases.
| -H "X-Api-Key: ${OWASP_TOKEN}" \ | ||
| -F "autoCreate=true" \ | ||
| -F "projectName=xrpl-js" \ | ||
| -F "projectName=test-alert" \ |
There was a problem hiding this comment.
Forget to revert the change?
ahoffman-ripple
left a comment
ahoffman-ripple
left a comment
There was a problem hiding this comment.
update projectName in release.yml, beyond that looks good to go
|
Re: #3131 (comment) The reviewers are fetched from a GitHub protected environment. However, npmjs supports only a single protected environment for trusted publishing, which must be the designated “official-release” environment. As a result, reviewers configured in other protected environments cannot grant approval; only reviewers defined within the “official-release” environment are permitted to authorize the release. |
|
Overall, LGTM. We can merge once:
|
RELEASE.md
Outdated
| ### **Before triggering a release** | ||
|
|
||
| **Stable release ** | ||
| 1. Create a release branch. A qualified branch name should start with "release-" or "release/", case-insensitive. e.g: `release/[email protected]`, `release-xrpl-4.3.8`, `Release/[email protected]`. |
There was a problem hiding this comment.
As per changes to release.yml, upper case release branch names are not allowed. RELEASE.md should be updated.
There was a problem hiding this comment.
Environment protection rules for offical-review, github-pages and first-review etc should be updated accordingly to reflect these changes.
There was a problem hiding this comment.
thanks for spotting it. I have made change to release.yml to match the environment protection rule.
| PR_NUMBER="" | ||
| PR_URL="" | ||
| # For stable releases, check that a PR exists from the release branch to main |
There was a problem hiding this comment.
I was under the impression that for stable releases, we would first merge the PR into main branch and then release it from main branch itself so that provenance commit details match. There won't be any need for RELEASE_BRANCH for stable releases. So, checking release -> main PR would be unnecessary for stable releases.
For beta releases, we would release from RELEASE_BRANCH since, we can't merge beta branch into main.
Do you see any issue with this approach?
There was a problem hiding this comment.
Another benefit of releasing stable releases from main branch is that the Git tags are tagged on the commit of main branch as opposed to the commit of release branch.
There was a problem hiding this comment.
One concern with releasing directly from the main branch is that the workflow becomes unreliable if any unrelated commits are pushed to main after the intended release commit. In that scenario, the workflow might release a wrong version.
I suggest we defer this change to a later PR if we truly want to support releases from main. And I'd rather keep the release flow tied to the merge event, so the merge automatically triggers the release and guarantees that the workflow runs against the exact commit we intend to publish.
RELEASE.md
Outdated
| } | ||
| ``` | ||
| 3. Run npm i to update the package-lock with the updated versions and commit the lock file to the release branch | ||
| 4. Set `npm distribution tag` to `latest`. |
There was a problem hiding this comment.
This is a Github Action step and should be mentioned somewhere else (or at the end of the list), since other steps are the one developer performs on their local machine.
There was a problem hiding this comment.
rewrote this section.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
.github/workflows/release.yml (1)
373-373: Use validated output instead of raw input for RELEASE_BRANCH.Line 373 uses
github.event.inputs.release_branch_namedirectly; use the validated output from theget_versionjob instead for consistency with other jobs and to ensure downstream logic uses the canonical branch name.env: PACKAGE_VERSION: "${{ needs.get_version.outputs.package_version }}" PACKAGE_NAME: "${{ github.event.inputs.package_name }}" - RELEASE_BRANCH: "${{ github.event.inputs.release_branch_name }}" + RELEASE_BRANCH: "${{ needs.get_version.outputs.release_branch }}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/faucet_test.yml(1 hunks).github/workflows/nodejs.yml(4 hunks).github/workflows/release.yml(18 hunks)RELEASE.md(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
📚 Learning: 2024-10-11T18:48:28.561Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2779
File: packages/xrpl/HISTORY.md:10-14
Timestamp: 2024-10-11T18:48:28.561Z
Learning: Ensure to reference the correct version numbers when commenting on changes to `HISTORY.md` to avoid confusion.
Applied to files:
RELEASE.md
📚 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/nodejs.yml.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.github/workflows/faucet_test.yml
📚 Learning: 2025-04-16T15:28:21.204Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2801
File: packages/xrpl/test/wallet/batchSigner.test.ts:0-0
Timestamp: 2025-04-16T15:28:21.204Z
Learning: In the XRPL.js library, hardcoded seeds in test files are acceptable as they don't represent protected data or real funds - they're only used for consistent test behavior.
Applied to files:
.github/workflows/release.yml
🪛 LanguageTool
RELEASE.md
[grammar] ~40-~40: Ensure spelling is correct
Context: ... Triggering the workflow with following requied inputs: - Stable release -...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
RELEASE.md
13-13: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
25-25: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
49-49: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
⏰ 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)
- GitHub Check: integration (24.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: build-and-lint (24.x)
- GitHub Check: browser (24.x)
- GitHub Check: unit (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: unit (24.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (9)
.github/workflows/faucet_test.yml (1)
13-22: LGTM!The new
run_faucet_testsinput and conditional job execution follow the gating pattern used across the release workflow. The conditional logic correctly skips the job only when invoked viaworkflow_callwithinputs.run_faucet_tests == false, allowing faucet tests to be selectively skipped during beta releases.RELEASE.md (1)
59-59: LGTM — content and structure are sound.The documentation clearly separates stable and beta release flows, properly explains the PR requirement for stable releases, and accurately documents test behavior (integration and faucet tests skipped for beta). The input/validation sections are well-structured.
Also applies to: 68-68, 75-75
.github/workflows/nodejs.yml (1)
23-37: LGTM!The three new inputs (
run_unit_tests,run_integration_tests,run_browser_tests) are well-defined with consistent descriptions and defaults. The conditional guards on the corresponding jobs (lines 83, 127, 182) correctly implement the gating pattern: jobs skip only when invoked viaworkflow_callwith the input set tofalse..github/workflows/release.yml (6)
12-14: LGTM!The
release_branch_nameinput is properly defined as required, enabling the workflow to be triggered frommainwith explicit branch selection for both stable and beta releases.
45-148: LGTM — input validation logic is sound.The
validate_inputsstep correctly:
- Checks that the workflow is triggered from
main(line 108).- Validates the release branch name for stable releases (lines 103–106).
- Determines
is_betabased onnpmjs_dist_tagand appends "-experimental" suffix for beta releases (lines 96–101).- Validates the npm dist-tag format (lines 87–94).
- For stable releases, verifies that an open PR exists from the release branch to
main(lines 121–140).- Outputs all derived values (
dist_tag,release_branch,release_pr_number,release_pr_url,is_beta) for downstream jobs.
175-193: LGTM — test gating correctly implemented.The
run_faucet_testandrun_testsjobs correctly:
- Pass
git_reffrom the validated release branch output.- Skip faucet and integration/browser tests for beta releases using the
is_betaoutput.- Allow unit tests to always run, ensuring baseline functionality validation.
This aligns with the PR objectives and RELEASE.md documentation.
684-694: LGTM!The
generate-documentationjob correctly:
- Runs only for stable releases (
is_beta != 'true').- Uses the validated release branch output for
git_ref(addressing the prior feedback to use outputs instead of inputs).- Depends on the
releasejob to ensure documentation is generated after the tag is created.
696-748: LGTM — failure notification logic is comprehensive.The
notify_failuresjob correctly:
- Uses
always()to run regardless of prior job outcomes.- Filters failed jobs using a granular conditional that respects beta release test-skip logic (lines 713–714: integration/faucet test failures don't block beta releases).
- Extracts failed job names dynamically from the
needscontext and posts a consolidated Slack notification.
1-30: LGTM — workflow structure and concurrency controls are sound.The inputs, outputs, permissions, and concurrency settings are well-configured. The
defaults.run.shell: bashensures pipefail and other bash options work consistently across all steps.
| -H "X-Api-Key: ${OWASP_TOKEN}" \ | ||
| -F "autoCreate=true" \ | ||
| -F "projectName=xrpl-js" \ | ||
| -F "projectName=test-alert" \ |
There was a problem hiding this comment.
Replace hardcoded project name with dynamic value.
Line 270 uses a hardcoded projectName=test-alert for the OWASP Dependency-Track upload. This should be dynamic to reflect the actual package being released.
-F "autoCreate=true" \
- -F "projectName=test-alert" \
+ -F "projectName=${PKG_NAME}" \
-F "projectVersion=${PKG_VERSION}" \Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
.github/workflows/release.yml around line 270: the workflow currently hardcodes
-F "projectName=test-alert" when uploading to OWASP Dependency-Track; replace
this with a dynamic value (e.g., use the package name or a workflow/repo
variable). Update the step to inject a variable such as ${ { env.PROJECT_NAME }
} or ${ { github.event.repository.name } } (set PROJECT_NAME earlier from
package.json or repository context) so the form field uses that variable instead
of the literal "test-alert".
There was a problem hiding this comment.
@shichengripple001 this appears to be a relevant concern ^
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
|
||
| 1. Create a release branch. A qualified branch name should start with "release-" or "release/", case-insensitive. e.g: `release/[email protected]`, `release-xrpl-4.3.8`, `Release/[email protected]`. | ||
| 2. Update the **`version`** field in `packages/<package_name>/package.json` to the intended release version. | ||
| **Stable release ** |
There was a problem hiding this comment.
Fix markdown formatting: remove spaces inside emphasis markers.
Lines 13 and 26 have trailing spaces before the closing ** markers (**Stable release ** and **Beta release **), which violates markdownlint MD037. Remove the interior space before the closing asterisks.
-**Stable release **
+**Stable release**-**Beta release **
+**Beta release**Also applies to: 26-26
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
13-13: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
🤖 Prompt for AI Agents
In RELEASE.md around lines 13 and 26, the bold emphasis markers contain an extra
space before the closing ** (e.g., "**Stable release **" and "**Beta release
**"); remove the interior trailing space so the markers read "**Stable
release**" and "**Beta release**" to satisfy markdownlint MD037.
| - `rc` → Release candidate | ||
| 1. Go to **GitHub → Actions → Release Pipeline → Run workflow** (must be triggered from `main`). | ||
|
|
||
| 2. Triggering the workflow with following requied inputs: |
There was a problem hiding this comment.
Fix typo: "requied" → "required".
Line 40 contains a spelling error: "requied" should be "required".
-2. Triggering the workflow with following requied inputs:
+2. Triggering the workflow with following required inputs:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 2. Triggering the workflow with following requied inputs: | |
| 2. Triggering the workflow with following required inputs: |
🧰 Tools
🪛 LanguageTool
[grammar] ~40-~40: Ensure spelling is correct
Context: ... Triggering the workflow with following requied inputs: - Stable release -...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In RELEASE.md around line 40, there's a spelling mistake: change "requied" to
"required" in the sentence "Triggering the workflow with following requied
inputs:" so the line reads "Triggering the workflow with following required
inputs:"; update only the typo and keep surrounding wording intact.
| - **Beta release** (publishes as `<tag>-experimental`) | ||
| - `release_branch_name`: e.g., `feature/xrpl-beta` (no naming restriction). | ||
| - `package_name`: e.g., `xrpl`. | ||
| - `npmjs_dist_tag`: a non-`latest` tag like `beta` or `rc` (must match `[a-z][a-z0-9._-]{0,127}` and not start with `v` + digit or a digit). |
There was a problem hiding this comment.
Fix list indentation.
Lines 49–52 have incorrect indentation for the nested list items under "Beta release". Markdownlint expects consistent 2-space indentation. Realign the - markers for release_branch_name, package_name, and npmjs_dist_tag under the Beta release block.
- **Beta release** (publishes as `<tag>-experimental`)
- - `release_branch_name`: e.g., `feature/xrpl-beta` (no naming restriction).
- - `package_name`: e.g., `xrpl`.
- - `npmjs_dist_tag`: a non-`latest` tag like `beta` or `rc` (must match `[a-z][a-z0-9._-]{0,127}` and not start with `v` + digit or a digit).
+ - `release_branch_name`: e.g., `feature/xrpl-beta` (no naming restriction).
+ - `package_name`: e.g., `xrpl`.
+ - `npmjs_dist_tag`: a non-`latest` tag like `beta` or `rc` (must match `[a-z][a-z0-9._-]{0,127}` and not start with `v` + digit or a digit).Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
49-49: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 2; Actual: 5
(MD007, ul-indent)
🤖 Prompt for AI Agents
In RELEASE.md around lines 49 to 52, the nested list items under "Beta release"
are mis-indented; adjust each nested bullet (`release_branch_name`,
`package_name`, `npmjs_dist_tag`) to be indented with two spaces under the
parent bullet so the `-` markers align at the same column and conform to
Markdownlint’s 2-space nested list rule; keep the content unchanged, only update
leading spaces to two for each nested line.
6 participants
High Level Overview of Change
1, Workflow needs to be triggered from main branch for both beta and stable release
2, For stable release, A PR from release branch to main branch is required before triggering the release pipeline
3, Security champion(Chenna and Cheng) are added to Security reviewers. Beta sec review will be done by Security champion.
4, "-experimental" will automatically appended to dist tag for beta release
5, Tests are allowed to fail for beta release
Context of Change
Type of Change
Did you update HISTORY.md?
Test Plan