Make Knip and Jest workflows reusable#67
Conversation
- Create reusable/knip.yml with configurable package manager support - Create reusable/jest.yml with configurable package manager and coverage support - Update knip.yml to use reusable workflow - Update jest-testing.yml to use reusable workflow - Remove knip-reporter.yml (integrated into reusable knip workflow) - Add documentation for reusable workflows Benefits: - Centralized maintenance across all UbiquityOS repositories - Support for bun, yarn, npm, and pnpm package managers - Configurable Node.js and Yarn versions - Reduced duplication and improved consistency
📝 WalkthroughWalkthroughThis change extracts Jest and Knip workflow logic into reusable workflows stored in 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
.github/workflows/reusable/knip.yml (2)
64-70: Redundantactions/setup-nodein Setup Yarn step.Node is already set up at lines 53-56; re-invoking
setup-nodewith the same version here is dead weight. Dropping it and keeping only the Corepack activation is cleaner.♻️ Suggested fix
- - name: Setup Yarn - if: inputs.package_manager == 'yarn' - uses: actions/setup-node@v4 - with: - node-version: ${{ inputs.node_version }} - - run: corepack enable && corepack prepare yarn@${{ inputs.yarn_version }} --activate + - name: Setup Yarn (Corepack) if: inputs.package_manager == 'yarn' + run: corepack enable && corepack prepare yarn@${{ inputs.yarn_version }} --activate
72-83: Use--immutableinstead of--frozen-lockfilefor Yarn 2+ to future-proof against deprecation.
--frozen-lockfileis a backward-compatible alias in Yarn Berry (2+/3.x) and works without errors, but it's deprecated and will be removed in a future release. Use--immutableto avoid breakage when upgrading Yarn.♻️ Suggested fix
elif [ "${{ inputs.package_manager }}" = "yarn" ]; then - yarn install --frozen-lockfile + yarn install --immutable
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9c04339f-7ed8-4940-962f-0dcda9fa2f3a
📒 Files selected for processing (6)
.github/workflows/jest-testing.yml.github/workflows/knip-reporter.yml.github/workflows/knip.yml.github/workflows/reusable/README.md.github/workflows/reusable/jest.yml.github/workflows/reusable/knip.yml
💤 Files with no reviewable changes (1)
- .github/workflows/knip-reporter.yml
| - name: Install dependencies | ||
| working-directory: ${{ inputs.working_directory }} | ||
| run: | | ||
| if [ "${{ inputs.package_manager }}" = "bun" ]; then | ||
| bun install --frozen-lockfile | ||
| elif [ "${{ inputs.package_manager }}" = "yarn" ]; then | ||
| yarn install --frozen-lockfile | ||
| elif [ "${{ inputs.package_manager }}" = "npm" ]; then | ||
| npm ci | ||
| elif [ "${{ inputs.package_manager }}" = "pnpm" ]; then | ||
| pnpm install --frozen-lockfile | ||
| fi |
There was a problem hiding this comment.
Yarn Berry breaks with --frozen-lockfile (same as knip.yml).
Yarn 2+ uses --immutable. See the Knip review for the suggested gate on yarn --version. Worth extracting a shared composite action or script to avoid drift between the two reusable workflows.
| - name: Run Jest Tests | ||
| id: test-run | ||
| working-directory: ${{ inputs.working_directory }} | ||
| run: | | ||
| if [ "${{ inputs.package_manager }}" = "bun" ]; then | ||
| bun run ${{ inputs.test_script }} | ||
| elif [ "${{ inputs.package_manager }}" = "yarn" ]; then | ||
| yarn ${{ inputs.test_script }} | ||
| elif [ "${{ inputs.package_manager }}" = "npm" ]; then | ||
| npm run ${{ inputs.test_script }} | ||
| elif [ "${{ inputs.package_manager }}" = "pnpm" ]; then | ||
| pnpm run ${{ inputs.test_script }} | ||
| fi | ||
|
|
||
| - name: Run Coverage (if enabled) | ||
| if: inputs.coverage | ||
| working-directory: ${{ inputs.working_directory }} | ||
| run: | | ||
| if [ "${{ inputs.package_manager }}" = "bun" ]; then | ||
| bun run ${{ inputs.coverage_script }} | ||
| elif [ "${{ inputs.package_manager }}" = "yarn" ]; then | ||
| yarn ${{ inputs.coverage_script }} | ||
| elif [ "${{ inputs.package_manager }}" = "npm" ]; then | ||
| npm run ${{ inputs.coverage_script }} | ||
| elif [ "${{ inputs.package_manager }}" = "pnpm" ]; then | ||
| pnpm run ${{ inputs.coverage_script }} | ||
| fi |
There was a problem hiding this comment.
Coverage runs in addition to tests, not instead.
When coverage: true, the suite executes twice (test_script, then coverage_script), roughly doubling CI time. Typically coverage supersedes the plain test run.
♻️ Suggested fix
- - name: Run Jest Tests
- id: test-run
+ - name: Run Jest Tests
+ id: test-run
+ if: ${{ !inputs.coverage }}
working-directory: ${{ inputs.working_directory }}
run: |
...
- - name: Run Coverage (if enabled)
+ - name: Run Jest Tests with Coverage
if: inputs.coverage| - name: Run Knip | ||
| id: knip-run | ||
| working-directory: ${{ inputs.working_directory }} | ||
| run: | | ||
| if [ "${{ inputs.package_manager }}" = "bun" ]; then | ||
| bun run ${{ inputs.knip_script }} || bun run ${{ inputs.knip_script }} --reporter json > knip-results.json | ||
| elif [ "${{ inputs.package_manager }}" = "yarn" ]; then | ||
| yarn ${{ inputs.knip_script }} || yarn ${{ inputs.knip_script }} --reporter json > knip-results.json | ||
| elif [ "${{ inputs.package_manager }}" = "npm" ]; then | ||
| npm run ${{ inputs.knip_script }} || npm run ${{ inputs.knip_script }} -- --reporter json > knip-results.json | ||
| elif [ "${{ inputs.package_manager }}" = "pnpm" ]; then | ||
| pnpm run ${{ inputs.knip_script }} || pnpm run ${{ inputs.knip_script }} -- --reporter json > knip-results.json | ||
| fi | ||
|
|
||
| - name: Upload knip result | ||
| if: failure() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: knip-results | ||
| path: | | ||
| knip-results.json | ||
| pr-number.txt | ||
| retention-days: 1 |
There was a problem hiding this comment.
Artifact path breaks when working_directory != .
Run Knip has working-directory: ${{ inputs.working_directory }}, so knip-results.json is written inside that directory. But upload-artifact resolves knip-results.json relative to the workspace root, so the upload will miss the file whenever working_directory is overridden. Same concern applies to the reporter job's json_input_file_name: knip-artifact/knip-results.json.
Consider writing the JSON to a stable workspace-root path (e.g. $GITHUB_WORKSPACE/knip-results.json) or prefixing the upload path with ${{ inputs.working_directory }}.
| ## Permissions | ||
|
|
||
| Both workflows require the following permissions: | ||
|
|
||
| ```yaml | ||
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| actions: read | ||
| ``` | ||
|
|
||
| The Knip workflow additionally needs `actions: read` permission to download artifacts from the workflow run. |
There was a problem hiding this comment.
Permissions section contradicts itself and the Jest workflow.
The code block lists actions: read as required for both, but the follow-up sentence says it's Knip-only — and indeed reusable/jest.yml only declares contents: read + pull-requests: write. Split the blocks per workflow to avoid confusion.
📝 Suggested fix
-Both workflows require the following permissions:
+**Jest workflow:**
+
+```yaml
+permissions:
+ contents: read
+ pull-requests: write
+```
+
+**Knip workflow** (needs `actions: read` to download artifacts between jobs):
```yaml
permissions:
contents: read
pull-requests: write
actions: read-The Knip workflow additionally needs actions: read permission to download artifacts from the workflow run.
</details>
<!-- fingerprinting:phantom:poseidon:nectarine:fc6fe64d-33c1-4664-943f-04e0bf26173a -->
<!-- This is an auto-generated comment by CodeRabbit -->
|
Update: PR #67 is ready for review! Implementation:
All tests passed. Ready for merge! 🚀 |
1 participant
Description
Related Issue
Fixes #13
Testing