CI: Provide basic CI jobs for forked repositories, and disable our own jobs #32632
Conversation
|
View your CI Pipeline Execution ↗ for commit 380f9ac
☁️ Nx Cloud last updated this comment at |
Sidnioulz
force-pushed
the
sidnioulz/contrib-gh-actions-fork
branch
from
46016c7 to
f4d9dfd
Compare
📝 WalkthroughWalkthroughAdds repository-owner gating (github.repository_owner == 'storybookjs') to many GitHub Actions jobs, consolidates fork unit tests into a matrix-based Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Event as GitHub Event
participant WF as GitHub Actions Workflow
participant Jobs as Jobs
rect rgb(230,240,255)
note over WF: Owner-gated workflows
Event->>WF: trigger (schedule / push / pull_request)
WF->>WF: evaluate github.repository_owner == 'storybookjs'
alt owner == 'storybookjs'
WF->>Jobs: run gated jobs (publish, stale, triage, tests, generate-sandboxes, trigger-circle-ci, etc.)
else owner != 'storybookjs'
WF--xJobs: skip gated jobs
end
end
rect rgb(235,255,235)
note over WF: Fork-checks (matrix)
Event->>WF: push from fork or non-owner repo
WF->>WF: evaluate github.repository_owner != 'storybookjs'
alt owner != 'storybookjs'
WF->>Jobs: run fork-checks: type-check, formatting, unit-tests (matrix: ubuntu/windows)
else owner == 'storybookjs'
WF--xJobs: skip fork-checks
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![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)
CONTRIBUTING.md (1)
173-177: Fix broken shell command (too many arguments to cd)The example concatenates two commands into one line; it will fail. Split into two lines.
-```shell -cd code yarn build --watch react core-server api addon-docs -``` +```shell +cd code +yarn build --watch react core-server api addon-docs +```
🧹 Nitpick comments (7)
.github/workflows/prepare-patch-release.yml (1)
21-21: Owner-gate LGTM; consider explicit minimal permissionsThe repo-owner check is correct. Optionally harden the workflow by setting explicit, minimal GITHUB_TOKEN permissions.
Example (top-level):
on: push: branches: - next workflow_dispatch: +permissions: + contents: read + actions: read.github/workflows/cron-weekly.yml (1)
12-13: Pin third-party actions to a commit SHATo reduce supply-chain risk, pin external actions (checkout, markdown-link-check) to immutable SHAs instead of tags.
.github/workflows/stale.yml (1)
6-12: Set explicit permissions for the stale actionactions/stale needs write scopes to label/close. Define minimal permissions at workflow/job level to avoid relying on defaults.
Example (top-level):
on: schedule: - cron: "30 1 * * *" +permissions: + issues: write + pull-requests: write + contents: read.github/workflows/trigger-circle-ci-workflow.yml (2)
1-16: Harden pull_request_target workflow permissionsGiven this uses pull_request_target and triggers external systems, set minimal GITHUB_TOKEN permissions.
Example (top-level):
name: Trigger CircleCI workflow on: pull_request_target: types: [opened, synchronize, labeled, reopened] push: branches: - next - main +permissions: + contents: read + pull-requests: read + actions: read
62-68: Pin external action to a SHAPin fjogeleit/http-request-action to a commit SHA for supply-chain safety.
code/frameworks/angular/start-schema.json (1)
149-152: loglevel description vs validation mismatchDescription includes “error” but the pattern excludes it. Include error to avoid rejecting valid values.
- "pattern": "(silly|verbose|info|warn|silent)" + "pattern": "^(silly|verbose|info|warn|error|silent)$"code/frameworks/angular/build-schema.json (1)
33-36: loglevel description vs validation mismatchAllow “error” to match the documented options.
- "pattern": "(silly|verbose|info|warn|silent)" + "pattern": "^(silly|verbose|info|warn|error|silent)$"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.github/workflows/cron-weekly.yml(1 hunks).github/workflows/fork-checks.yml(1 hunks).github/workflows/generate-sandboxes.yml(2 hunks).github/workflows/handle-release-branches.yml(3 hunks).github/workflows/prepare-non-patch-release.yml(1 hunks).github/workflows/prepare-patch-release.yml(1 hunks).github/workflows/publish.yml(2 hunks).github/workflows/stale.yml(1 hunks).github/workflows/tests-unit.yml(1 hunks).github/workflows/triage.yml(1 hunks).github/workflows/trigger-circle-ci-workflow.yml(3 hunks)CONTRIBUTING.md(1 hunks)code/.prettierignore(1 hunks)code/frameworks/angular/build-schema.json(5 hunks)code/frameworks/angular/start-schema.json(6 hunks)code/frameworks/server-webpack5/template/cli/page.stories.yaml(1 hunks)code/frameworks/sveltekit/tsconfig.json(1 hunks)code/lib/eslint-plugin/docs/rules/no-stories-of.md(0 hunks)
💤 Files with no reviewable changes (1)
- code/lib/eslint-plugin/docs/rules/no-stories-of.md
🧰 Additional context used
📓 Path-based instructions (1)
code/**/tsconfig*.json
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep TypeScript strict mode enabled; do not relax strict compiler options in tsconfig files
Files:
code/frameworks/sveltekit/tsconfig.json
🪛 actionlint (1.7.7)
.github/workflows/fork-checks.yml
48-48: label "windows-11-arm" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
⏰ 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: Core Unit Tests, windows-latest
🔇 Additional comments (7)
.github/workflows/cron-weekly.yml (1)
9-9: Owner-gate LGTMThis prevents scheduled runs on forks. Looks good.
.github/workflows/stale.yml (1)
9-9: Owner-gate LGTMPrevents stale bot from operating on forks. Good.
.github/workflows/trigger-circle-ci-workflow.yml (1)
19-19: Owner-gates LGTMGating all CircleCI trigger jobs on the org owner is correct and avoids secret exposure on forks.
Also applies to: 41-41, 60-60
code/frameworks/angular/start-schema.json (1)
96-100: Formatting-only changes LGTMSingle-line arrays and trailing newline are fine; no behavior change.
Also applies to: 135-138, 140-143, 154-157, 196-197, 224-225, 233-233
code/frameworks/angular/build-schema.json (1)
70-74: Formatting-only changes LGTMArray compaction and newline tweak look good; behavior unchanged.
Also applies to: 76-79, 81-84, 119-122, 161-162, 189-190, 198-198
.github/workflows/handle-release-branches.yml (1)
8-8: Owner-gates LGTMKeeps release-branch handling limited to the org; safe given secrets and external hooks.
Also applies to: 72-72, 91-91
CONTRIBUTING.md (1)
132-140: Fork/upstream guidance LGTMClearer upstream setup; removing “disable Actions on forks” aligns with new fork CI.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 43 | 43 | 0 |
| Self size | 22.93 MB | 22.92 MB | 🎉 -11 KB 🎉 |
| Dependency size | 17.36 MB | 17.36 MB | 🎉 -6 B 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 533 | 532 | 🎉 -1 🎉 |
| Self size | 749 KB | 749 KB | 🎉 -230 B 🎉 |
| Dependency size | 58.92 MB | 58.50 MB | 🎉 -421 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 124 | 124 | 0 |
| Self size | 3.83 MB | 3.83 MB | 🎉 -51 B 🎉 |
| Dependency size | 21.75 MB | 21.57 MB | 🎉 -184 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 157 | 157 | 0 |
| Self size | 31 KB | 31 KB | 🎉 -15 B 🎉 |
| Dependency size | 23.11 MB | 22.93 MB | 🎉 -184 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 114 | 114 | 0 |
| Self size | 37 KB | 37 KB | 🚨 +127 B 🚨 |
| Dependency size | 19.66 MB | 19.48 MB | 🎉 -184 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 273 | 272 | 🎉 -1 🎉 |
| Self size | 25 KB | 25 KB | 🎉 -15 B 🎉 |
| Dependency size | 43.88 MB | 43.46 MB | 🎉 -421 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 187 | 0 |
| Self size | 928 KB | 928 KB | 🚨 +168 B 🚨 |
| Dependency size | 72.89 MB | 72.88 MB | 🎉 -11 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 169 | 169 | 0 |
| Self size | 35 KB | 35 KB | 🚨 +78 B 🚨 |
| Dependency size | 69.31 MB | 69.30 MB | 🎉 -11 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 44 | 44 | 0 |
| Self size | 1.55 MB | 1.55 MB | 🎉 -366 B 🎉 |
| Dependency size | 40.29 MB | 40.28 MB | 🎉 -11 KB 🎉 |
| Bundle Size Analyzer | node | node |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 57 | 2 | 🎉 -55 🎉 |
| Self size | 812 KB | 627 KB | 🎉 -184 KB 🎉 |
| Dependency size | 12.94 MB | 28 KB | 🎉 -12.91 MB 🎉 |
| Bundle Size Analyzer | Link | Link |
|
|
||
| import { addons } from 'storybook/manager-api'; | ||
|
|
||
| /* eslint-disable prettier/prettier */ |
There was a problem hiding this comment.
This is questionable. To make sure prettier doesn't mess with our sensitive files in forked repos, we need to actually add them to prettierignore.
But this is a less good setup for us where we know internally that specific bits of the files should be ignored, but still want automatic formatting on the rest of the files through ESLint.
There was a problem hiding this comment.
Why is prettierignore better for forks here?
Is it because forks run prettier directly instead of via eslint? If so, can we just add a prettierignore comment as well as the existing eslint comment?
There was a problem hiding this comment.
Exactly. See how our command uses eslint-disable to prevent prettifying? We don't appear to be running prettier directly in the monorepo. If we don't ignore the file in prettier, the file's content changes when running prettier. So actually ignoring the file results in stronger protection (at the expense of no longer having ESLint format the rest).
There was a problem hiding this comment.
(btw I meant that my code change was questionable, not the original code ;-) so thanks for questioning it!)
There was a problem hiding this comment.
Even going as far as this doesn't stop prettier. Both VSCode and yarn lint:prettier change import order.
|
|
||
| import { addons } from 'storybook/manager-api'; | ||
|
|
||
| /* eslint-disable prettier/prettier */ |
There was a problem hiding this comment.
Why is prettierignore better for forks here?
Is it because forks run prettier directly instead of via eslint? If so, can we just add a prettierignore comment as well as the existing eslint comment?
Sidnioulz
force-pushed
the
sidnioulz/contrib-gh-actions-fork
branch
from
5c17c12 to
79cac23
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
CONTRIBUTING.md (1)
132-140: Minor wording refinement for clarity.The documentation update correctly aligns with the PR's shift from discouraging CI on forks to providing streamlined upstream setup. However, the static analysis tool flags two minor style opportunities to strengthen the wording:
- Line 132: Consider tightening "This will allow you to make changes to the codebase and submit a pull request to the main repository when you're ready" → something more direct like "allowing you to propose changes via pull request."
- Line 134: "To do this, run the following commands" could be more direct, e.g., "Set up the upstream remote with these commands:"
-If you plan to contribute to Storybook's codebase, you should fork the repository to your GitHub account. This will allow you to make changes to the codebase and submit a pull request to the main repository when you're ready to contribute your changes. +If you plan to contribute to Storybook's codebase, you should fork the repository to your GitHub account, allowing you to propose changes via pull request. -Additionally, adding our codebase as upstream ensures you can rebase against the latest changes in the main repository. To do this, run the following commands: +Additionally, add the upstream repository to track the latest changes and facilitate rebasing:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.github/workflows/cron-weekly.yml(1 hunks).github/workflows/fork-checks.yml(1 hunks).github/workflows/generate-sandboxes.yml(2 hunks).github/workflows/handle-release-branches.yml(3 hunks).github/workflows/prepare-non-patch-release.yml(1 hunks).github/workflows/prepare-patch-release.yml(1 hunks).github/workflows/publish.yml(2 hunks).github/workflows/stale.yml(1 hunks).github/workflows/tests-unit.yml(1 hunks).github/workflows/triage.yml(1 hunks).github/workflows/trigger-circle-ci-workflow.yml(3 hunks)CONTRIBUTING.md(1 hunks)code/.prettierignore(1 hunks)code/core/src/core-server/presets/common-manager.ts(0 hunks)code/frameworks/angular/build-schema.json(5 hunks)code/frameworks/angular/start-schema.json(6 hunks)code/frameworks/server-webpack5/template/cli/page.stories.yaml(1 hunks)code/frameworks/sveltekit/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- code/core/src/core-server/presets/common-manager.ts
✅ Files skipped from review due to trivial changes (1)
- code/frameworks/sveltekit/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (10)
- .github/workflows/publish.yml
- .github/workflows/cron-weekly.yml
- .github/workflows/handle-release-branches.yml
- .github/workflows/prepare-patch-release.yml
- .github/workflows/triage.yml
- .github/workflows/generate-sandboxes.yml
- .github/workflows/tests-unit.yml
- code/frameworks/angular/start-schema.json
- code/.prettierignore
- .github/workflows/prepare-non-patch-release.yml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/frameworks/angular/build-schema.json
🪛 LanguageTool
CONTRIBUTING.md
[style] ~132-~132: Consider shortening or rephrasing this to strengthen your wording.
Context: ... GitHub account. This will allow you to make changes to the codebase and submit a pull request ...
(MAKE_CHANGES)
[style] ~134-~134: Consider a more expressive alternative.
Context: ...test changes in the main repository. To do this, run the following commands: ```s...
(DO_ACHIEVE)
⏰ 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: Core Unit Tests, windows-latest
🔇 Additional comments (8)
code/frameworks/angular/build-schema.json (1)
70-70: Prettier formatting looks good.All changes are formatting adjustments that condense arrays to single-line representation where appropriate. The JSON structure, schema definitions, and validation logic remain unchanged. This aligns with the PR's goal of running Prettier on the codebase and meets the coding guidelines requirement for Prettier formatting.
Also applies to: 76-76, 81-81, 119-119, 161-161, 189-189
code/frameworks/server-webpack5/template/cli/page.stories.yaml (1)
1-10: LGTM – quote style formatting consistent with Prettier pass.These quote-style changes (double to single quotes on YAML string values) are purely cosmetic and align with the PR's stated Prettier formatting objectives. The YAML remains valid, and there are no functional or semantic impacts.
CONTRIBUTING.md (1)
132-140: Documentation update appropriately simplifies fork setup guidance.The removal of explicit "disable CI" instructions and replacement with streamlined upstream setup is consistent with the PR's goal of supporting CI on forked repositories. The git commands provided are accurate and follow standard practice. The updated guidance is helpful for new contributors setting up their fork for development.
.github/workflows/stale.yml (1)
9-9: LGTM!The repository owner gate is correctly placed and prevents the stale job from running on forked repositories, aligning with the PR's access-control strategy.
.github/workflows/trigger-circle-ci-workflow.yml (3)
19-19: LGTM!The repository owner gate correctly restricts branch detection to the storybookjs repository, preventing unnecessary CircleCI API calls from forks.
41-41: LGTM!The repository owner gate correctly restricts parameter detection to the storybookjs repository. The dependent
trigger-circle-ci-workflowjob is also gated on the same condition, so outputs are safely accessed only when available.
60-60: LGTM!The condition correctly gates CircleCI workflow triggering to the storybookjs repository and validates that a workflow parameter is defined, preventing unnecessary API calls. The syntax is clean and doesn't include unnecessary
${{}}wrapping..github/workflows/fork-checks.yml (1)
11-27: Good: Proper fork gating and matrix consolidation.The workflow correctly gates all three jobs to run only on forks (
github.repository_owner != 'storybookjs'), and thetestjob consolidates Windows and Ubuntu runs into a single matrix-driven job rather than separate job definitions. This aligns with the goal of providing basic CI for forks without duplicating configuration.Also applies to: 28-43, 45-69
| on: | ||
| push: |
There was a problem hiding this comment.
Add pull_request trigger to enable checks on fork PRs.
The workflow currently only triggers on push events, which means code reviews and PR checks won't automatically run for contributors. Add the pull_request trigger so checks execute when PRs are opened or updated.
on:
push:
+ pull_request:📝 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.
| on: | |
| push: | |
| on: | |
| push: | |
| pull_request: |
🤖 Prompt for AI Agents
In .github/workflows/fork-checks.yml around lines 4 to 5 the workflow only
triggers on push, so forked PRs won’t run checks automatically; update the
top-level events to include pull_request (e.g., add pull_request with useful
activity types like opened, reopened, synchronize) alongside push so the
workflow runs when PRs are created or updated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CONTRIBUTING.md (1)
134-142: ✓ Good removal of outdated fork CI guidance and clean upstream setup.The removal of the "disable GitHub Actions for forks" instruction aligns well with the PR's new fork-specific CI jobs (unit tests, prettier, TypeScript). The updated section now correctly directs contributors to add the upstream remote, which is the right focus.
Minor wording refinement opportunities per static analysis:
- Line 134: "This will allow you to" → consider "enabling you to" or "letting you" for stronger voice
- Line 136: "To do this, run" → consider "run the following" or "execute these commands"
These are optional improvements for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CONTRIBUTING.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md
[style] ~134-~134: Consider shortening or rephrasing this to strengthen your wording.
Context: ... GitHub account. This will allow you to make changes to the codebase and submit a pull request ...
(MAKE_CHANGES)
[style] ~136-~136: Consider a more expressive alternative.
Context: ...test changes in the main repository. To do this, run the following commands: ```s...
(DO_ACHIEVE)
⏰ 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: Core Unit Tests, windows-latest
|
@JReinhold FYI I can't merge myself, so I'll leave it up to the core team to merge when relevant :) |
JReinhold
added
build
3 participants
TO DO
What I did
code/to ensure the prettier job provided to fork repo owners provides actual feedback on their code changesThis should help ensure that forked repositories have a coherent CI status, that actually reflects code quality issues rather than CI setup shenanigans. If forked repos have code quality reporting, there's a wee chance this increases the quality of received PRs.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
ø
Manual testing
Documentation
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Summary by CodeRabbit