-
Notifications
You must be signed in to change notification settings - Fork 35
Fix #1512 #1515
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
I finally decided to read the Corepack documentation and skim its code. It turns out `corepack enable` doesn't actually do any installing. https://github.com/nodejs/corepack?tab=readme-ov-file#utility-commands
🦋 Changeset detectedLatest commit: dc2e510 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
To avoid this issue, modify (1) Buildkite pipelines to cache on `package.json`, and (2) Dockerfiles to mount `package.json` and run `corepack install`: | ||
|
||
```diff | ||
seek-oss/docker-ecr-cache#v2.1.0: | ||
cache-on: | ||
- .npmrc | ||
+ - package.json | ||
- pnpm-lock.yaml | ||
``` |
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.
@samchungy I won't release this until we sort out seek-oss/docker-ecr-cache-buildkite-plugin#51, so you can modify this changeset later.
Alternatively, we could actually revert #1512. It feels wrong but pnpm install --offline
can invoke the Corepack shim and (online) install the appropriate version as required. The reason one of my repos hit a stdin hang is because it wasn't using propagate-environment
so Corepack thought it wasn't running in CI=true
.
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.
I defer to sam
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.
Nah I think what you've got here makes sense. If we're pinning the version of pnpm, we want to ensure it is using the pinned version, just in case a bad release creeps in.
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.
I can probably write up a patch to target this
I finally decided to read the Corepack documentation and skim its code. It turns out
corepack enable
doesn't actually do any installing.https://github.com/nodejs/corepack?tab=readme-ov-file#utility-commands