Beta release workflow #3131
Beta release workflow #3131
Conversation
WalkthroughThe pull request modifies Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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
requested review from
Patel-Raj11,
ahoffman-ripple,
kuan121 and
mvadari
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
513-513: Unclear logical AND inPACKAGE_NAMEassignment.Line 513 uses
needs.get_version.outputs.package_version && github.event.inputs.package_name, which employs a logical AND to conditionally select the package name. This is confusing and suggests unintended logic—the expression will return the package name only if package_version is truthy. Simplify to directly reference the input or output:- PACKAGE_NAME: ${{ needs.get_version.outputs.package_version && github.event.inputs.package_name }} + PACKAGE_NAME: ${{ github.event.inputs.package_name }}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml(17 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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`.
📚 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: 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
🔇 Additional comments (9)
.github/workflows/release.yml (9)
12-14: New input and outputs are well-structured.The addition of
release_branch_nameas a required input and the new outputs (dist_tag, npm_is_beta, release_branch, release_pr_number, release_pr_url, is_beta) establish a clear contract for downstream jobs.Also applies to: 29-34
42-143: Comprehensive input validation with proper guards.The
validate_inputsstep correctly handles package name validation, security checks (Artifactory URL), dist tag normalization & validation, branch requirements for stable releases, and PR existence checks for stable releases. The use ofgh apifor PR validation is appropriate.
176-176: Consistent use ofrelease_branchoutput across jobs.All downstream jobs correctly reference
needs.get_version.outputs.release_branchfor checkout and git refs.Also applies to: 184-184, 202-202
189-189: Test failure bypass for beta releases is correctly implemented.The conditional logic across
pre_release,ask_for_dev_team_review,first_review,ask_for_sec_team_review, andreleasejobs properly allows test failures for beta releases (whenis_beta == 'true') while requiring success for stable releases. This aligns with the PR objective.Also applies to: 351-351, 491-491, 503-503, 533-533
382-382: PR_URL outputs are correctly referenced.The new
release_pr_urloutput is consistently used across jobs and properly incorporated into Slack notifications.Also applies to: 435-435, 472-472
550-599: Release job correctly validates and publishes with appropriate flags.The publish step properly validates the package.json, enforces semantic versioning for stable releases, and uses the correct npm dist-tag. The GitHub release creation correctly sets
prereleaseandmake_latestflags based on theis_betaoutput.Also applies to: 626-627
664-716: Failure aggregation logic correctly distinguishes beta and stable failures.The
notify_failuresjob properly evaluates which failures should trigger notifications, allowing test failures for beta releases while requiring success for stable releases. The conditional logic is well-structured and the Slack notification lists failed job names.
212-212: Typo fix confirmed.The comment on line 212 correctly uses "debugging" (previously "dubugging").
145-170: ****The review comment misunderstands how GitHub Actions environment variables work. Environment variables set via
GITHUB_ENVare available to all processes in following steps of the same job. Since bothvalidate_inputsandget_versionare steps within the sameget_versionjob, theNPM_DIST_TAGvariable set invalidate_inputsis automatically available toget_version. The code is correct as-is and requires no changes.Likely an incorrect or invalid review comment.
| PACKAGE_VERSION: "${{ needs.get_version.outputs.package_version }}" | ||
| PACKAGE_NAME: "${{ github.event.inputs.package_name }}" | ||
| RELEASE_BRANCH: "${{ github.ref_name }}" | ||
| RELEASE_BRANCH: "${{ github.event.inputs.release_branch_name }}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use release_branch output instead of input for consistency.
Line 359 references github.event.inputs.release_branch_name directly, but the release_branch output from get_version is the validated and canonical value. For consistency with other jobs, use needs.get_version.outputs.release_branch.
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 }}"📝 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.
| RELEASE_BRANCH: "${{ github.event.inputs.release_branch_name }}" | |
| env: | |
| PACKAGE_VERSION: "${{ needs.get_version.outputs.package_version }}" | |
| PACKAGE_NAME: "${{ github.event.inputs.package_name }}" | |
| RELEASE_BRANCH: "${{ needs.get_version.outputs.release_branch }}" |
🤖 Prompt for AI Agents
.github/workflows/release.yml around line 359: the workflow is using
github.event.inputs.release_branch_name directly; replace that reference with
the validated output from the get_version job by setting RELEASE_BRANCH to
needs.get_version.outputs.release_branch so the job consumes the canonical,
validated branch value consistent with other jobs.
| 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 }} |
There was a problem hiding this comment.
generate-documentation job uses incorrect git_ref.
Line 662 uses git_ref: ${{ github.ref_name }}, which for a workflow_dispatch event refers to the current branch (typically main), not the release branch. Documentation should be generated from the released code. Consider using the release tag or 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 }}Alternatively, if documentation should be generated from the tag, use the tag name from the release job output (e.g., ${{ needs.release.outputs.tag_name }}).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
.github/workflows/release.yml lines 652-662: the generate-documentation job is
using git_ref: ${{ github.ref_name }}, which for workflow_dispatch resolves to
the current branch (usually main) instead of the released commit/tag; replace
the git_ref with the release job's tag or branch (for example use ${{
needs.release.outputs.tag_name }} or the release commit/branch output you have)
so the documentation job checks out the actual released code; update the
with.git_ref value accordingly and ensure the release job is listed in needs so
that its output is available.
There was a problem hiding this comment.
^^ @shichengripple001 can you please update the git_ref variable? I wrote the generate-documentation workflow based on the current version of the release workflow.
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!
| TAG="latest" | ||
| echo "ℹ️ npmjs_dist_tag empty → defaulting to 'latest'." | ||
| else | ||
| TAG="$(printf '%s' "$RAW_DIST_TAG" | tr -d '[:space:]')" |
There was a problem hiding this comment.
| TAG="$(printf '%s' "$RAW_DIST_TAG" | tr -d '[:space:]')" | |
| TAG="$(printf '%s' "$RAW_DIST_TAG" | tr -d '[:blank:]')" |
nit: This removes both spaces and tabs from the inputs, whereas :space: does not recognize tab characters.
| NPM_DIST_TAG="latest" | ||
| fi | ||
| if [[ "$NPM_DIST_TAG" == "latest" ]] && ! [[ "$VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then | ||
| NPM_DIST_TAG="${NPM_DIST_TAG:-latest}" |
There was a problem hiding this comment.
Is this step necessary? NPM_DIST_TAG is already written into GITHUB_ENV in lines 109-112 (get_version.steps.validate_inputs section) of this updated file.
|
|
||
| pre_release: | ||
| runs-on: ubuntu-latest | ||
| 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') }} |
There was a problem hiding this comment.
| 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') }} |
Since there are other conditional expressions, always() is not useful in this context.
| - name: Generate CycloneDX SBOM | ||
| run: cyclonedx-npm --output-format json --output-file sbom.json | ||
|
|
||
| - name: Scan SBOM for vulnerabilities using Trivy |
There was a problem hiding this comment.
Hypothetically, suppose that there are SBOM vulnerabilities reported for a beta release. Do we have a policy for accepting/rejecting the beta-release, for the role of the security champion? What are the tolerable types of SBOM vulnerabilities?
| - name: Print scan report | ||
| run: cat vuln-report.txt | ||
|
|
||
| - name: Upload vulnerability report artifact |
There was a problem hiding this comment.
Can we increase the retention policy for these uploaded vulnerability-report artifacts? I believe the default is 10 days, however a 30-day period is useful to perform retroactive analysis.
|
|
||
| ask_for_dev_team_review: | ||
| runs-on: ubuntu-latest | ||
| if: ${{ always() && needs.pre_release.result == 'success' && (needs.run_tests.result == 'success' || needs.get_version.outputs.is_beta == 'true') }} |
There was a problem hiding this comment.
| if: ${{ always() && needs.pre_release.result == 'success' && (needs.run_tests.result == 'success' || needs.get_version.outputs.is_beta == 'true') }} | |
| if: ${{ needs.pre_release.result == 'success' && (needs.run_tests.result == 'success' || needs.get_version.outputs.is_beta == 'true') }} |
| 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 }} |
There was a problem hiding this comment.
^^ @shichengripple001 can you please update the git_ref variable? I wrote the generate-documentation workflow based on the current version of the release workflow.
| run: | | ||
| MESSAGE="❌ Release failed for ${{ env.PACKAGE_NAME }}@${{ env.PACKAGE_VERSION }}. Check the logs: https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}" | ||
| curl -X POST https://slack.com/api/chat.postMessage \ | ||
| set -euo pipefail |
There was a problem hiding this comment.
The notify_failures job is not pinned to run on any specific shell. However, the pipefail option is configured to work only on bash and ksh shells. It will not work on dash (or) sh shells.
Discussion: https://askubuntu.com/questions/886537/set-e-o-pipefail-not-working-on-bash-script-on-ubuntu-16
Some of the jobs in this workflow use bash as the pinned shell, is that sufficient for all the jobs?
| IS_BETA="false" | ||
| else | ||
| IS_BETA="true" | ||
| TAG="${TAG}-experimental" |
There was a problem hiding this comment.
Can we do TAG="${TAG}-beta" instead?
| TAG="${TAG}-experimental" | ||
| fi | ||
| if [ "$IS_BETA" != "true" ] && [[ ! "${RELEASE_BRANCH,,}" =~ ^release[-/] ]]; then |
There was a problem hiding this comment.
Can we also validate branch name for beta release to ensure that the branch is main? I don't think we want people to release beta using any branch.
| on: | ||
| workflow_dispatch: | ||
| inputs: | ||
| package_name: |
There was a problem hiding this comment.
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
Why do we want to check that a PR to merge release-xxx into main exists? Can we still automatically create a PR to merge release-xxx into main for stable release?
5, Tests are allowed to fail for beta release
If test failures are acceptable, can we skip the test steps entirely for beta releases? What value do the tests provide in this case?
| package_name: | ||
| description: 'Package folder (Name of the package directory under packages/ folder. e.g., xrpl, ripple-address-codec)' | ||
| required: true | ||
| release_branch_name: |
There was a problem hiding this comment.
Can you update https://github.com/XRPLF/xrpl.js/blob/main/RELEASE.md to reflect the final release process before you merge this PR?
| if [ -z "$NPM_DIST_TAG" ]; then | ||
| NPM_DIST_TAG="latest" | ||
| # validate dist tag(s) | ||
| RAW_DIST_TAG="${{ github.event.inputs.npmjs_dist_tag }}" |
| set -euo pipefail | ||
| MSG="${EXECUTOR} is releasing ${PACKAGE_NAME}@${PACKAGE_VERSION}. A member from the infosec team (${SEC_REVIEWERS}) needs to take the following action:\n Review the release artifacts and approve/reject the release. (${RUN_URL})" | ||
| MSG="${EXECUTOR} is releasing ${PACKAGE_NAME}@${PACKAGE_VERSION}. A sec reviewer from (${SEC_REVIEWERS}) needs to take the following action:\nReview the release artifacts and approve/reject the release. (${RUN_URL})" |
There was a problem hiding this comment.
3, Security champion(Chenna and Cheng) are added to Security reviewers. Beta sec review will be done by Security champion.
For the beta release, will ${SEC_REVIEWERS} include Infosec members? They may find it confusing or inconvenient to be mentioned in the Slack notification when they don’t actually need to review beta releases. Would it make sense to use a separate notification mechanism, or maintain two security reviewer lists—one for stable releases and another for beta?
3 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