fix(ci): refactor workflow to securely support preview & cleanup for fork PRs - #272
Conversation
|
Warning Rate limit exceeded@SonyLeo has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughCI workflows updated: build now outputs docs and PR number artifacts with artifact name using PR head SHA when available; a new workflow publishes packages via pkg-pr-new; preview deployment converted to artifact-driven workflow_run trigger; cleanup workflow trigger and permissions adjusted. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer (PR)
participant GH as GitHub Actions
participant BuildWF as pr-ci-build
participant Artif as Artifact Storage
participant E2E as pr-ci-e2e-test
participant PublishWF as pr-ci-publish-packages
participant PkgPR as pkg-pr-new
participant DeployWF as pr-deploy-preview
participant Surge as Surge (preview)
Dev->>GH: Open PR (trigger)
GH->>BuildWF: Run build workflow
BuildWF->>BuildWF: Build components & docs\nWrite pr_number.txt
BuildWF->>Artif: Upload build artifacts,\ninclude docs/dist & pr_number.txt
GH->>E2E: Run E2E workflow
E2E->>Artif: Download build artifacts
E2E->>E2E: Execute tests
GH->>PublishWF: Invoke publish-workflow (with pr-number)
PublishWF->>Artif: Download build artifacts
PublishWF->>PkgPR: Run pkg-pr-new publish (components/kit/svgs)
PkgPR-->>PublishWF: Return output.json
PublishWF->>Artif: Upload pkg-pr-new output artifact
GH->>DeployWF: Trigger on CI workflow_run success
DeployWF->>Artif: Download build artifacts & pr_number.txt
DeployWF->>Artif: Download pkg-pr-new output (optional)
DeployWF->>Surge: Deploy docs/dist to PR preview domain
Surge-->>DeployWF: Return deploy URL/status
DeployWF->>GH: Post comment with preview URL and status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
SonyLeo
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pr-cleanup.yml (1)
23-34: Replace this unmaintained action with an actively maintained alternative.The
actions-cool/maintain-one-commentaction is no longer maintained and has been removed from the GitHub Actions marketplace. Using deprecated actions poses security and compatibility risks. Consider switching tomarocchino/sticky-pull-request-comment, which is actively maintained and provides similar functionality.
🧹 Nitpick comments (2)
.github/workflows/pr-ci-publish-packages.yml (1)
5-9: Unused input parameterpr-number.The
pr-numberinput is declared as required but is never referenced within this workflow. If commenting is handled bypr-deploy-preview.yml, consider removing this input to simplify the interface.🔎 Suggested fix
If the input is truly unused, remove it:
on: workflow_call: - inputs: - pr-number: - description: 'Pull Request number' - type: number - required: trueAnd update the caller in
pr-ci.yml:publish-packages: needs: [build] if: github.event_name == 'pull_request' uses: ./.github/workflows/pr-ci-publish-packages.yml - with: - pr-number: ${{ github.event.pull_request.number }} secrets: inherit.github/workflows/pr-deploy-preview.yml (1)
108-116: Verify Surge CLI is available without explicit installation.The workflow uses
npx surgebut doesn't explicitly install the Surge CLI. Whilenpxwill download and execute the package, this adds latency and relies on npm registry availability at runtime.Consider whether pre-installing surge would improve reliability:
+ - name: Install Surge + run: npm install -g surge + - name: Deploy Site to Surge id: deploy run: | DEPLOY_DOMAIN=preview-${{ steps.pr.outputs.number }}-tiny-robot.surge.sh echo "Deploying to: https://$DEPLOY_DOMAIN" - npx surge --project ./artifacts/docs/dist --domain $DEPLOY_DOMAIN --token $SURGE_TOKEN + surge --project ./artifacts/docs/dist --domain $DEPLOY_DOMAIN --token $SURGE_TOKEN echo "url=https://$DEPLOY_DOMAIN" >> $GITHUB_OUTPUT env: SURGE_TOKEN: ${{ secrets.SURGE_TOKEN }}
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/pr-ci-build.yml.github/workflows/pr-ci-e2e-test.yml.github/workflows/pr-ci-publish-packages.yml.github/workflows/pr-ci.yml.github/workflows/pr-cleanup.yml.github/workflows/pr-deploy-preview.yml
🔇 Additional comments (13)
.github/workflows/pr-ci-e2e-test.yml (1)
38-42: LGTM!The artifact naming now correctly references the PR head SHA with a fallback to
github.sha, which aligns with the updated naming convention inpr-ci-build.yml. Downloading to root path (.) is appropriate since the build artifacts now include multiple directories (packages/*/dist,docs/dist) that need to preserve their relative paths..github/workflows/pr-ci.yml (1)
31-38: LGTM!The job rename to
publish-packagesand workflow reference update topr-ci-publish-packages.ymlaccurately reflects the separation of concerns—package publishing viapkg.pr.newis now distinct from the preview deployment handled bypr-deploy-preview.ymlviaworkflow_run. This aligns well with the relay pattern for fork PR support..github/workflows/pr-ci-build.yml (1)
48-50: LGTM!The PR number is correctly extracted from
github.event.numberand saved only onpull_requestevents. This file will be consumed bypr-deploy-preview.ymlto identify which PR to comment on..github/workflows/pr-ci-publish-packages.yml (2)
43-47: LGTM!The artifact download correctly uses the same naming pattern as the build workflow and downloads to root path to preserve the package structure.
49-58: LGTM!The
pkg.pr.newpublish command correctly:
- Targets the three package directories
- Uses
--pnpmflag for pnpm compatibility- Outputs to JSON for downstream consumption
- Disables automatic commenting (
--comment=off) since the relay workflow handles PR comments.github/workflows/pr-deploy-preview.yml (4)
4-12: Well-implemented relay pattern for fork PR security.The
workflow_runtrigger with explicit minimal permissions correctly implements the recommended security pattern:
- Runs in base repository context (not fork context)
- Has access to secrets (
SURGE_TOKEN,GITHUB_TOKEN)- Declares only necessary permissions (
pull-requests: write,actions: read,contents: read)- Does not checkout fork source code, only pre-built static artifacts
This is the correct approach to securely handle fork PRs as described in GitHub's documentation.
15-19: LGTM!The conditional correctly ensures the deploy job only runs when:
- The triggering workflow was initiated by a
pull_requestevent- The CI workflow completed successfully
This prevents unnecessary deployments on push events or failed builds.
30-38: LGTM!The PR number extraction from artifact is correctly implemented with proper error handling. Exiting with code 1 on missing file will fail the workflow early rather than proceeding with invalid state.
118-132: Success condition may be too broad.
if: success()checks that all previous steps succeeded, which is the intended behavior here. However, if any prior step (like the pkg.pr.new comment step) fails, this success comment won't post even though the deploy itself succeeded.The current implementation should work correctly since
continue-on-error: trueis set on the optional pkg.pr.new download step (line 47), but it's worth noting this dependency..github/workflows/pr-cleanup.yml (4)
1-1: LGTM: Workflow name updated for consistency.The name change aligns with the CI workflow naming convention used across the repository.
4-5: LGTM: Correct use ofpull_request_targetfor fork PR support.The trigger change to
pull_request_targetis necessary to grant this workflow access to secrets and write permissions when cleaning up preview deployments from fork PRs. The implementation is secure because:
- No code from the PR is checked out
- Only trusted inputs (
github.event.pull_request.number) are usedThis aligns with GitHub's recommended security practices for workflows that need elevated permissions with fork PRs.
7-9: LGTM: Minimal permissions follow least-privilege principle.The permissions block correctly grants only
pull-requests: write, which is the minimum required for posting cleanup status comments. No additional permissions are needed since the workflow doesn't checkout code or perform other privileged operations.
14-21: LGTM: Cleanup logic is correct and resilient.The Surge teardown implementation is appropriate:
- Correctly constructs the deployment domain using the PR number
- Uses
continue-on-error: trueto handle cases where the deployment may not exist- Securely passes the token via environment variables
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pr-ci-build.yml (1)
3-4: Critical: Workflow trigger doesn't match PR objectives.The workflow is configured with
workflow_call(line 4), which requires manual invocation from another workflow. However, the PR objectives explicitly state: "modify pr-ci-build.yml to run on pull_request" and describe a build stage that "run[s] on pull_request" to support fork PRs in a low-privilege context.With the current
workflow_calltrigger, this workflow will not automatically run when PRs are opened or updated, breaking the relay pattern described in the PR objectives.🔎 Proposed fix
on: - workflow_call: - inputs: - skip-playground: - description: 'Skip playground build' - type: boolean - default: false + pull_request: + branches: + - develop + - mainIf
workflow_callmust be retained for backward compatibility, addpull_requestas an additional trigger:on: + pull_request: + branches: + - develop + - main workflow_call: inputs: skip-playground: description: 'Skip playground build' type: boolean default: false
♻️ Duplicate comments (1)
.github/workflows/pr-ci-build.yml (1)
51-61: Acknowledge past review: Artifact upload issue still applies.The past review comment correctly identified that
pr_number.txt(line 60) will not exist onpushevents, which may cause the artifact upload to fail. This issue persists in the current version.Combined with the critical issue on line 49 (incorrect property for PR number), the recommended fix is:
- Add a conditional to the "Save PR number" step (line 48-50) to run only on pull_request events
- Add
if-no-files-found: warnto the upload step to handle cases where pr_number.txt doesn't exist🔎 Combined fix addressing both issues
- name: Save PR number + if: github.event_name == 'pull_request' - run: echo "${{ github.event.number }}" > pr_number.txt + run: echo "${{ github.event.pull_request.number }}" > pr_number.txt - name: Upload build artifacts uses: actions/upload-artifact@v4 with: name: build-${{ github.event.pull_request.head.sha || github.sha }} path: | packages/components/dist packages/kit/dist packages/svgs/dist docs/dist pr_number.txt retention-days: 1 + if-no-files-found: warn
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pr-ci-build.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: build / build
🔇 Additional comments (1)
.github/workflows/pr-ci-build.yml (1)
45-46: LGTM: Documentation build step added.The new docs build step correctly uses the pnpm filter flag to build the docs package, and the output aligns with the
docs/distartifact path on line 59.
🧹 Preview Cleaned UpThe preview deployment has been removed. |
…fork PRs (opentiny#272) (cherry picked from commit 77aa8b5)
2 participants
修复 Fork 仓库 PR 无法触发预览和评论的问题 (CI Workflow)
描述：
1. 问题背景 (Why)
此前，当外部贡献者从 Fork 仓库提交 Pull Request 时，CI 工作流会报错或部分功能失效。主要原因如下：
权限限制：出于安全考虑，GitHub 默认将来自 Fork 的 PR 的 GITHUB_TOKEN 降级为 只读 (Read-only)，导致无法自动发表评论（如预览链接）。
Secrets 隔离：Fork 的 PR 无法访问原仓库的 Secrets（如 SURGE_TOKEN），导致部署预览站点失败。
2. 解决方案 (What)
本 PR 对 CI 架构进行了重构，采用了 GitHub 推荐的 “接力模式 (Relay Pattern)”，将构建与部署分离：
构建阶段 (低权限)：
修改 pr-ci-build.yml。
该阶段仍在 pull_request 事件下运行（安全沙箱环境）。
负责构建代码，并将构建产物 (dist) 和 PR 编号 (pr_number.txt) 打包为 Artifact 上传。
部署阶段 (高权限)：
新增 workflow_preview.yml。
使用 workflow_run 事件触发，监听主 CI 完成。
该阶段运行在原仓库上下文中，拥有 Write 权限 和 Secrets 访问权。
负责下载 Artifact，执行 Surge 部署，并回写评论到对应的 PR。
清理阶段：
修改 pr-cleanup.yml 的触发器为 pull_request_target，确保 PR 关闭时有权限清理 Surge 部署资源。
3. 安全性说明
构建过程不依赖任何 Secrets，且在低权限下运行，防止恶意代码通过 npm install/build 窃取凭证。
高权限的部署流程不 checkout 外部代码，仅处理预构建的静态资源，符合安全最佳实践。
4. 验证
已在 Fork 仓库模拟测试，外部 PR 提交后：
主 CI 构建成功 (Pass)。
Preview 工作流自动触发并成功发表预览链接评论。
可以参考： SonyLeo#18
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.