sh1pt: install Node pnpm Test (node-pnpm-test@1.0.0) - #399
Conversation
vu1nz Security Review0 finding(s) in PR #? No security issues found. |
Greptile SummaryThis PR adds a new
Confidence Score: 4/5Safe to merge from a correctness standpoint — tests will still run and nothing will break — but the workflow duplicates work already done by ci.yml on every PR and master push. The new workflow introduces no broken steps and the CI environment is otherwise sound, but it unconditionally doubles test execution time on every trigger event by running the same steps that ci.yml already runs. The missing concurrency group means rapid pushes can also stack up multiple parallel runs of this workflow with no automatic cancellation. .github/workflows/test.yml warrants a second look given its overlap with the existing ci.yml. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Push to master or Pull Request] --> B[ci.yml triggered]
A --> C[test.yml triggered - NEW]
B --> B1[checkout]
B1 --> B2[pnpm setup v9]
B2 --> B3[setup-node 22]
B3 --> B4[pnpm install frozen]
B4 --> B5[pnpm type-check]
B5 --> B6[pnpm test]
C --> C1[checkout]
C1 --> C2[pnpm setup v9.12.0]
C2 --> C3[setup-node 22]
C3 --> C4[pnpm install frozen]
C4 --> C5[pnpm test - duplicate]
style C fill:#ffe0e0,stroke:#cc0000
style C5 fill:#ffe0e0,stroke:#cc0000
Reviews (2): Last reviewed commit: "Merge remote-tracking branch 'origin/mas..." | Re-trigger Greptile |
|
|
||
| on: | ||
| pull_request: | ||
| push: | ||
| branches: [master] |
There was a problem hiding this comment.
Overlapping triggers with existing
ci.yml
The existing .github/workflows/ci.yml already runs pnpm install --frozen-lockfile and pnpm test on every pull_request and on push to master. This new workflow duplicates those exact steps for the same trigger events, so every PR and every master push will now run pnpm test twice in parallel, consuming double the CI minutes with no additional coverage.
| jobs: | ||
| test: | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 15 |
There was a problem hiding this comment.
The existing ci.yml cancels in-progress runs with a concurrency group, so redundant queued runs are cleaned up automatically. Without this, rapid pushes or frequent PR updates will stack up multiple test workflow runs simultaneously.
| jobs: | |
| test: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 15 | |
| concurrency: | |
| group: test-${{ github.workflow }}-${{ github.ref }} | |
| cancel-in-progress: true | |
| jobs: | |
| test: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 15 |
| - uses: pnpm/action-setup@v4 | ||
| with: | ||
| version: 9.12.0 |
There was a problem hiding this comment.
Inconsistent pnpm version with
ci.yml
ci.yml uses version: 9 (tracks the latest 9.x patch automatically), while this workflow hard-pins to 9.12.0. When pnpm 9.x releases bug or security fixes, ci.yml picks them up but test.yml stays behind, making the two environments subtly differ.
| - uses: pnpm/action-setup@v4 | |
| with: | |
| version: 9.12.0 | |
| - uses: pnpm/action-setup@v4 | |
| with: | |
| version: 9 |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
sh1pt Actions Fleet: install Node pnpm Test
Installs Node pnpm Test (
node-pnpm-test@1.0.0) from the sh1pt Actions Store.Files
.github/workflows/test.yml— createRequired secrets
No new secrets required.
Notes
Generated by sh1pt Actions Fleet.