Merged
Conversation
refs: - ci: switch to pnpm - ruyisdk/ruyisdk-website@22f4137 Signed-off-by: Mozi <29089388+pzhlkj6612@users.noreply.github.com>
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideCI workflows for GitHub Pages deployment and PR builds are updated to use pnpm (with pnpm/action-setup and pnpm caching) instead of npm for installing dependencies and building the website. Flow diagram for updated gh-page and PR workflows with pnpmflowchart TD
Start[workflow_start]
subgraph CommonSteps[common_job_steps]
Checkout[checkout_repository]
PNPMSetup[pnpm_action_setup_v4_version_10]
NodeSetup[setup_node_22_4_1_cache_pnpm]
Install[run_pnpm_install]
Build[run_pnpm_run_build]
end
Start --> Checkout --> PNPMSetup --> NodeSetup --> Install --> Build
subgraph gh_page_workflow
Deploy[peaceiris_actions_gh_pages_deploy]
end
subgraph pr_workflow
PRChecks[pr_build_validation]
end
Build --> Deploy
Build --> PRChecks
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In CI it’s generally safer to use
pnpm install --frozen-lockfile(or equivalent) instead of plainpnpm installso that builds fail if the lockfile is out of sync with package.json. - Given the description mentions reusing actions from
ruyisdk/ruyisdk-website, consider extracting a shared reusable workflow for the pnpm setup/build so that the configuration isn’t duplicated across repos.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In CI it’s generally safer to use `pnpm install --frozen-lockfile` (or equivalent) instead of plain `pnpm install` so that builds fail if the lockfile is out of sync with package.json.
- Given the description mentions reusing actions from `ruyisdk/ruyisdk-website`, consider extracting a shared reusable workflow for the pnpm setup/build so that the configuration isn’t duplicated across repos.
## Individual Comments
### Comment 1
<location> `.github/workflows/gh-page.yaml:33-36` </location>
<code_context>
with:
node-version: 22.4.1
- cache: npm
+ cache: pnpm
- name: Install dependencies
</code_context>
<issue_to_address>
**suggestion (performance):** Consider specifying `cache-dependency-path` for more reliable pnpm cache hits.
With `cache: pnpm`, `actions/setup-node` derives cache keys from lockfiles. If your lockfile isn’t at the repo root (or you want to be explicit), set `cache-dependency-path` (e.g. `docs/pnpm-lock.yaml`) so the cache uses the correct file and remains stable if the repo layout changes.
```suggestion
- uses: actions/setup-node@v4
with:
node-version: 22.4.1
cache: pnpm
cache-dependency-path: docs/pnpm-lock.yaml
```
</issue_to_address>
### Comment 2
<location> `.github/workflows/gh-page.yaml:38-39` </location>
<code_context>
- name: Install dependencies
- run: npm ci
+ run: pnpm install
- name: Build website
- run: npm run build
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Lockfile integrity enforcement is looser with `pnpm install` than `npm ci`.
`npm ci` failed when the lockfile and `package.json` were out of sync. To preserve that safety with pnpm, consider `pnpm install --frozen-lockfile` (or `--lockfile-only` elsewhere) so CI fails on lockfile drift instead of updating it during the run.
```suggestion
- name: Install dependencies
run: pnpm install --frozen-lockfile
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
38
to
+39
| - name: Install dependencies | ||
| run: npm ci | ||
| run: pnpm install |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Lockfile integrity enforcement is looser with pnpm install than npm ci.
npm ci failed when the lockfile and package.json were out of sync. To preserve that safety with pnpm, consider pnpm install --frozen-lockfile (or --lockfile-only elsewhere) so CI fails on lockfile drift instead of updating it during the run.
Suggested change
| - name: Install dependencies | |
| run: npm ci | |
| run: pnpm install | |
| - name: Install dependencies | |
| run: pnpm install --frozen-lockfile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
refs:
Better approach: re-use actions in the "ruyisdk/ruyisdk-website" repo.
Summary by Sourcery
Switch GitHub workflows to use pnpm for installing dependencies and building the website.
Build:
CI: