-
Notifications
You must be signed in to change notification settings - Fork 60
ci: upgrade pnpm to 9.1.2 #217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
/assign @RainbowMango |
Echo from https://github.com/karmada-io/dashboard/actions/runs/15388148918/job/43291266877:
What's the root cause of it?
Do you mean [email protected] always installs the latest dependencies? |
root cause is the mismatched version of pnpm(one in ci and the other one in pnpm-lock file) pnpm-lock file has extra field to indicate the version of pnpm, in the main branch, the pnpm-lock file is generate by pnpm@9 Lines 1 to 5 in 185adbd
but in the ci, we still use the outdated pnpm, outdated pnpm cannot follow the locked dependencies, it will install the latest dependencies and it will caused typescript types error. Because it will install the latest antd, but we developed the dashboard with a stable version of antd, and ci will fail finally. As for when will the newer pnpm-lock file be introducted, I think it's in some special time, the latest antd is compatible with the antd which we used, so the pnpm-lock file be merged. |
for the better way of pnpm management, we should follow the Lines 31 to 38 in 185adbd
|
@@ -23,7 +23,7 @@ jobs: | |||
- name: install node dependencies | |||
uses: pnpm/action-setup@v4 | |||
with: | |||
version: 8.15.6 | |||
version: 9.1.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pnpm version should follow the one from package.json, right?
Line 35 in 185adbd
"packageManager": "[email protected]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one of best practice is corepack=> https://github.com/nodejs/corepack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, how about adding one comment on it? like:
version: 9.1.2 | |
# keep in sync with the packageManager version in ui/package.json | |
version: 9.1.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, how about adding one comment on it? like:
+1
BTW, can I use your suggestion code directly on github, will it cause some DCO problem ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May met the DCO problem and we have add the same comment at all 3 places, so I'd suggest go with a manual push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done~
Signed-off-by: warjiang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
What this PR does / why we need it:
upgrade pnpm version otherwise pnpm will install latest dependencies instead of following the pnpm-lock file
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: