chore: migrate to pnpm#229
Conversation
|
Thanks Denis. For app-4 in your screenshot, which test case (repo) did you use and how did you create/deploy it? |
I created a simple file 
|
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR migrates build, test, and development workflows from Yarn to pnpm. It updates package metadata, dependency overrides, hoisting configuration, container and CI commands, README instructions, and webpack stylesheet exclusion logic. ChangesYarn to pnpm migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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)
.npmrc (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBroaden-then-narrow:
*types*hoist pattern is too permissive.
*types*matches any package name containing that substring anywhere (e.g.mime-types), not just@types/*scoped packages. This can unintentionally hoist unrelated transitive deps, reintroducing phantom-dependency issues.Proposed tightening
-public-hoist-pattern[]=*types* +public-hoist-pattern[]=`@types/`*🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.npmrc at line 1, The public-hoist-pattern entry is too broad because the *types* glob can match unrelated packages like mime-types, so tighten the hoist rule in .npmrc to target only `@types` scoped packages. Update the existing hoist pattern to a more specific matcher using the same npm config entry so only intended TypeScript type packages are hoisted and phantom dependencies are avoided.package.json (1)
86-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale
jest/@types/jestoverrides after removing Jest scripts.
pnpm.overridesstill pinsjest/@types/jestto the very old21.xline even though the Jest test scripts were removed fromscripts(lines 8-17). If Jest is no longer used, these overrides are dead weight pinning an outdated (and likely vulnerable) transitive version; if it's still needed transitively, the override should track a current major version instead of21.x.#!/bin/bash # Check if jest is referenced anywhere else in the repo (config, CI, other scripts) rg -n "jest" --iglob '!**/node_modules/**'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 86 - 100, The pnpm overrides still pin jest and `@types/jest` to the obsolete 21.x line even though the Jest scripts were removed from the package scripts. Update the package.json overrides by removing these entries if Jest is no longer used anywhere, or bump them to a currently supported major if they are still required transitively; check the package.json scripts and any other Jest references in the repo before deciding. Use the existing pnpm.overrides block and the jest/@types/jest keys to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.konflux/Containerfile.plugin:
- Around line 21-32: The container build in Containerfile.plugin uses an
unpinned pnpm install and disables lockfile enforcement, so update the build to
use a pinned pnpm version via Corepack or an explicit version tied to
packageManager, and switch both pnpm install steps back to frozen-lockfile
behavior. Adjust the install flow around the pnpm setup and the two pnpm install
commands so the image build always follows the committed pnpm-lock.yaml and does
not drift with newer pnpm releases.
In `@Dockerfile`:
- Around line 5-6: The build stage is invoking pnpm without first making pnpm
available, so update the Dockerfile’s setup before the RUN pnpm config set and
RUN pnpm install/build steps by enabling corepack or installing pnpm in this
node:23 stage. Also correct the pnpm timeout configuration in the same
Dockerfile setup from network-timeout to fetchTimeout so the configuration is
recognized.
In `@package.json`:
- Around line 8-17: The package manifest no longer exposes the test-related
scripts expected by the documentation, so the contributor workflow is broken. In
package.json, restore the missing test, test:update, and test:coverage entries
if they are still supported, or update the README Testing section to reference
the current script names instead of yarn test*. Keep the commands aligned with
the existing npm scripts alongside build, start, and lint.
In `@webpack.config.ts`:
- Around line 50-53: The allow-list regex in the webpack config is checking the
wrong package scope, so `@openshift-console/plugin-shared` is still being
excluded from the sass/css loader. Update the `exclude` predicate in
`webpack.config.ts` to match the actual dependency name used by
`@openshift-console/plugin-shared` (alongside the existing `@patternfly`
exception), so the intended package is included correctly.
---
Nitpick comments:
In @.npmrc:
- Line 1: The public-hoist-pattern entry is too broad because the *types* glob
can match unrelated packages like mime-types, so tighten the hoist rule in
.npmrc to target only `@types` scoped packages. Update the existing hoist pattern
to a more specific matcher using the same npm config entry so only intended
TypeScript type packages are hoisted and phantom dependencies are avoided.
In `@package.json`:
- Around line 86-100: The pnpm overrides still pin jest and `@types/jest` to the
obsolete 21.x line even though the Jest scripts were removed from the package
scripts. Update the package.json overrides by removing these entries if Jest is
no longer used anywhere, or bump them to a currently supported major if they are
still required transitively; check the package.json scripts and any other Jest
references in the repo before deciding. Use the existing pnpm.overrides block
and the jest/@types/jest keys to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: c2cdaacf-7d49-4e2b-8034-6ce11f3edfe8
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
.konflux/Containerfile.plugin.npmrcDockerfileREADME.mdpackage.jsonwebpack.config.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #229 +/- ##
==========================================
- Coverage 11.92% 11.91% -0.02%
==========================================
Files 154 156 +2
Lines 6272 6321 +49
Branches 2028 2122 +94
==========================================
+ Hits 748 753 +5
- Misses 5524 5568 +44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
keithchong
left a comment
keithchong
left a comment
There was a problem hiding this comment.
Hi @dkarpele , could you rebase this?
Signed-off-by: dkarpele <karpelevich@gmail.com>
Signed-off-by: dkarpele <karpelevich@gmail.com>
| "consolePlugin": { | ||
| "name": "gitops-plugin", | ||
| "version": "0.0.17", | ||
| "version": "0.0.16", |
There was a problem hiding this comment.
@dkarpele , the version is downgraded. Could you change this back to 0.0.17?
| "version": "0.0.16", | ||
| "displayName": "GitOps Plugin", | ||
| "description": "OpenShift Console plugin for GitOps.", | ||
| "exposedModules": { |
There was a problem hiding this comment.
I think you might be using an older version of the console plugin. Could you remove this? We got rid of ./components in master.
| COPY ["package.json", "pnpm-lock.yaml", "./"] | ||
|
|
||
| RUN yarn install --no-progress --non-interactive | ||
| RUN pnpm install --no-frozen-lockfile --ignore-scripts |
There was a problem hiding this comment.
This brings up a good question. Here, it is --no-frozen-lockfiler, but in the Dockerfile, we are using -frozen-lockfile to ensure we are using the same dependencies specified in the pnpm-lock.yaml file. Do you think we should be updating the lockfile for the konflux build?
Signed-off-by: dkarpele <karpelevich@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/unit-tests.yml:
- Line 17: Align the unit-test workflow’s node-version setting with the
production container build’s Node.js major version: change it from v24 to v22,
unless the project is intentionally migrating the container image to v24.
In @.konflux/Containerfile.plugin:
- Around line 24-29: Move the WORKDIR /usr/src/app instruction before the
package-file COPY and initial pnpm install so dependencies install in the
application directory. Keep ADD . /usr/src/app after dependency installation,
ensuring source changes do not invalidate the cached dependency layer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 0e4d24d9-5d31-455a-a7c9-debed52907ff
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
.github/workflows/unit-tests.yml.konflux/Containerfile.plugin.npmrcDockerfileREADME.mdpackage.jsonwebpack.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .npmrc
- webpack.config.ts
…encies. - Align Node.js version with the container build environment. Signed-off-by: dkarpele <karpelevich@gmail.com>
|
|
||
| RUN yarn install --network-timeout 600000 && \ | ||
| yarn cache clean | ||
| COPY ["package.json", "pnpm-lock.yaml", "./"] |
There was a problem hiding this comment.
Copy .npmrc before installing dependencies?
| COPY ["package.json", "pnpm-lock.yaml", "./"] | |
| COPY ["package.json", "pnpm-lock.yaml", ".npmrc", "./"] |
4 participants
I tested commands from Readme.md (Option 1) and package.json: