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 
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis 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 2176 +148
==========================================
+ Hits 748 753 +5
+ Misses 5524 5345 -179
- Partials 0 223 +223
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", "./"] |
- install .npmrc before deps Signed-off-by: dkarpele <karpelevich@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/unit-tests.yml (1)
15-22: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRestore dependency caching for pnpm.
The prior Yarn cache setting was removed, which could slow down CI builds. You can restore caching by adding
cache: 'pnpm'to thesetup-nodeaction.Note:
actions/setup-noderequirespnpmto be installed in order to determine the cache paths, sopnpm/action-setupmust be placed beforeactions/setup-node.♻️ Proposed refactor
- - uses: actions/setup-node@v4 - with: - node-version: '22' - - uses: pnpm/action-setup@v4 with: run_install: false + + - uses: actions/setup-node@v4 + with: + node-version: '22' + cache: 'pnpm'🤖 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 @.github/workflows/unit-tests.yml around lines 15 - 22, Restore pnpm dependency caching by moving the pnpm/action-setup step before actions/setup-node, then add cache: 'pnpm' to the setup-node configuration. Preserve the existing Node 22 version and run_install: false settings.
🤖 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.
Nitpick comments:
In @.github/workflows/unit-tests.yml:
- Around line 15-22: Restore pnpm dependency caching by moving the
pnpm/action-setup step before actions/setup-node, then add cache: 'pnpm' to the
setup-node configuration. Preserve the existing Node 22 version and run_install:
false settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 5ac0d5f9-240a-49ba-92b1-cfbbeb1f664a
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
.github/workflows/unit-tests.yml.konflux/Containerfile.plugin
4 participants
I tested commands from Readme.md (Option 1) and package.json: