Skip to content

Conversation

@hanabi1224
Copy link
Contributor

@hanabi1224 hanabi1224 commented Jan 26, 2026

Summary of changes

Disable docs-deploy CI job for fork PRs as cloudflare secrect is inaccessible, to address failure https://github.com/ChainSafe/forest/actions/runs/21317189360/job/61376868088?pr=6455#step:7:7 in #6455

Changes introduced in this pull request:

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Summary by CodeRabbit

  • Chores
    • Deployment workflow updated to run for non-PR builds and to skip forked PRs unless a secure repository token is available, improving consistency and safety of documentation deployments.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 26, 2026

Walkthrough

Adds an if condition to the docs-deploy GitHub Actions job to run for non-PR events and to guard against fork PRs by requiring a non-null github.token, matching the workflow's inline comment.

Changes

Cohort / File(s) Summary
GitHub Actions workflow
/.github/workflows/docs-deploy.yml
Adds an if condition on the docs-deploy job that enables execution for non-PR events and prevents running for fork PRs by requiring a non-null github.token (aligns with inline comment)

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Suggested reviewers

  • akaladarshi
  • sudo-shashank
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a condition to disable the docs-deploy job for fork PRs by requiring a non-null github.token.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@hanabi1224 hanabi1224 marked this pull request as ready for review January 26, 2026 11:23
@hanabi1224 hanabi1224 requested a review from a team as a code owner January 26, 2026 11:23
@hanabi1224 hanabi1224 requested review from LesnyRumcajs and akaladarshi and removed request for a team January 26, 2026 11:23
LesnyRumcajs
LesnyRumcajs previously approved these changes Jan 26, 2026
@hanabi1224 hanabi1224 added this pull request to the merge queue Jan 26, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🤖 Fix all issues with AI agents
In @.github/workflows/docs-deploy.yml:
- Around line 27-28: The condition `if:
github.event.pull_request.head.repo.full_name == github.repository` fails for
non-pull_request events (push, merge_group) because github.event.pull_request is
null; change the conditional to only perform the fork check for pull_request
events, e.g. replace it with `if: github.event_name != 'pull_request' ||
github.event.pull_request.head.repo.full_name == github.repository` so
push/merge_group always pass and PRs are gated by the fork check, and also fix
the comment typo "secrect" → "secret".

Comment on lines 27 to 28
# Cloudflare secrect does not work for fork PRs
if: github.event.pull_request.head.repo.full_name == github.repository
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Condition blocks push and merge_group events, breaking main deployment.

The github.event.pull_request context is null for push and merge_group events, so this condition evaluates to false for those triggers. This will prevent docs from being deployed when changes are pushed to main.

Also, minor typo: "secrect" → "secret".

🔧 Proposed fix
-    # Cloudflare secrect does not work for fork PRs
-    if: github.event.pull_request.head.repo.full_name == github.repository
+    # Cloudflare secret does not work for fork PRs
+    if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository
📝 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.

Suggested change
# Cloudflare secrect does not work for fork PRs
if: github.event.pull_request.head.repo.full_name == github.repository
# Cloudflare secret does not work for fork PRs
if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository
🤖 Prompt for AI Agents
In @.github/workflows/docs-deploy.yml around lines 27 - 28, The condition `if:
github.event.pull_request.head.repo.full_name == github.repository` fails for
non-pull_request events (push, merge_group) because github.event.pull_request is
null; change the conditional to only perform the fork check for pull_request
events, e.g. replace it with `if: github.event_name != 'pull_request' ||
github.event.pull_request.head.repo.full_name == github.repository` so
push/merge_group always pass and PRs are gated by the fork check, and also fix
the comment typo "secrect" → "secret".

✅ Confirmed as addressed by @hanabi1224

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hanabi1224, thank you for fixing the condition and the typo! 🎉


🧠 Learnings used
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai is the if condition correct now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hanabi1224, yes, the if condition is now correct! ✅

The current condition:

if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository }}

Works as intended:

  • Push to main: github.event_name != 'pull_request' is true → job runs ✅
  • Merge group: github.event_name != 'pull_request' is true → job runs ✅
  • Same-repo PR: First condition is false, but github.event.pull_request.head.repo.full_name == github.repository is true → job runs ✅
  • Fork PR: Both conditions are false → job is skipped ✅

The comment is also accurate and the typo is fixed. This will properly deploy docs for all non-fork events while preventing fork PRs from attempting to use the Cloudflare secret.


🧠 Learnings used
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.

@hanabi1224 hanabi1224 removed this pull request from the merge queue due to a manual request Jan 26, 2026
@hanabi1224 hanabi1224 enabled auto-merge January 26, 2026 11:28
@hanabi1224 hanabi1224 marked this pull request as draft January 26, 2026 11:47
auto-merge was automatically disabled January 26, 2026 11:47

Pull request was converted to draft

@hanabi1224 hanabi1224 force-pushed the hm/no-doc-deploy-for-fork-prs branch from 4fc1e41 to 9698be2 Compare January 26, 2026 11:49
@hanabi1224 hanabi1224 force-pushed the hm/no-doc-deploy-for-fork-prs branch from 9698be2 to 8fa09b2 Compare January 26, 2026 11:54
@hanabi1224 hanabi1224 marked this pull request as ready for review January 26, 2026 11:55
name: Deploy to Cloudflare Pages
runs-on: ubuntu-24.04-arm
# Enable workflow for non-PRs but disable workflow for fork PRs as Cloudflare secret does not work for fork PRs
if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this run on push?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@hanabi1224 hanabi1224 added this pull request to the merge queue Jan 26, 2026
Merged via the queue into main with commit cff1d86 Jan 26, 2026
10 checks passed
@hanabi1224 hanabi1224 deleted the hm/no-doc-deploy-for-fork-prs branch January 26, 2026 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants