Skip to content
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

feat!: prompt user before downloading software #360

Merged
merged 4 commits into from
Jan 30, 2024
Merged

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 23, 2024

Adds a prompt to let user validate each download . No prompt is shown when the software is already in the cache. The prompt can be opt-in/opt-out using an env variable, by default it's only shown when "not using the corepack binary" (i.e. when using the binaries created by corepack enable)

@merceyz
Copy link
Member

merceyz commented Jan 24, 2024

The problem with logging anything by default is that we'll break anything that depends on the CLI output of various commands.
For example yarn -v, pnpm -v, and npm -v wouldn't be guaranteed to print a valid SemVer string anymore.

@styfle
Copy link
Member

styfle commented Jan 24, 2024

Isn’t this already handled with “DEBUG=corepack” env var?

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 24, 2024

The issue this PR is trying to address is how can we make sure that if Node.js ships e.g. a pnpm binary with the default installer, how can we make sure the user is well aware that Corepack is involved and is downloading software from the internet.
Maybe we can think of a solution that would only apply to the jumper binaries, as I think it's fair to assume folks running corepack pnpm are well aware they're using Corepack.

Isn’t this already handled with “DEBUG=corepack” env var?

The issue of that is it's opt-in, when I think what we would need would be an opt-out.

@styfle
Copy link
Member

styfle commented Jan 24, 2024

Maybe print to stderr instead of stdout?

@mhdawson
Copy link
Member

@aduh95 I'm not sure a console log is enough in this case. I think the example where npx stops and asks if it's ok to download/install something is a good example of what might be.

@aduh95 aduh95 marked this pull request as ready for review January 26, 2024 00:24
README.md Outdated Show resolved Hide resolved
@aduh95 aduh95 changed the title feat: log network activity by default feat!: prompt user before downloading software Jan 30, 2024
@aduh95 aduh95 merged commit 6b8d87f into main Jan 30, 2024
10 checks passed
@aduh95 aduh95 deleted the log-network-activity branch January 30, 2024 22:38
@merceyz
Copy link
Member

merceyz commented Feb 21, 2024

Note that this change breaks our own tests because Corepack now prints stuff to stderr that the tests aren't expecting.
Ref #392.

Like I wrote in #360 (comment); this change is problematic.

@jgarber623
Copy link

this change is problematic.

Concur. This change broke an automated Dev Container project setup script that calls yarn install --check-files.

@@ -32,6 +33,7 @@ async function main() {
const entryPath = path.join(distDir, `${binaryName}.js`);
const entryScript = [
`#!/usr/bin/env node`,
`process.env.COREPACK_ENABLE_DOWNLOAD_PROMPT??='1'`,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"??=" may be used only for node 15.14+, but corepack added in 14.19.0+ (https://nodejs.org/docs/latest-v14.x/api/corepack.html)

If i use node 14.19.3, я get error for command "corepack enable" from last release

Copy link
Member

@styfle styfle Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corepack requires node.js 18

"node": "^18.17.1 || >=20.10.0"

If this change slipped into Node.js 14 its probably too late now since it won't receive updates. Its been about a year since the last update (went EOL on 2023-04-30). Although I doubt it since this PR merged after that, on 2024-01-30.

https://github.com/nodejs/Release/blob/main/README.md#:~:text=14.x,2023%2D04%2D30

eskultety added a commit to eskultety/cachi2 that referenced this pull request Jun 27, 2024
Starting with corepack 0.25.0 [1], corepack now defaults to asking
users if they're okay if its yarn shim downloads a particular version
from the registry:

  ! Corepack is about to download https://repo.yarnpkg.com/.../yarn.js
  ? Do you want to continue? [Y/n]

^This will break tests once we either explicitly move our
corepack == 0.20.0 to corepack >= 0.25.0 implicitly by adopting a newer
NodeJS releases in the multi-stage Docker build conversion future
patches will bring.
Either way, let's prepare for it by setting the new env variable
corepack introduced for this COREPACK_ENABLE_DOWNLOAD_PROMPT [2].

[1] https://github.com/nodejs/corepack/releases/tag/v0.25.0
[2] nodejs/corepack#360 (comment)

Signed-off-by: Erik Skultety <eskultet@redhat.com>
eskultety added a commit to eskultety/cachi2 that referenced this pull request Jun 28, 2024
Starting with corepack 0.25.0 [1], corepack now defaults to asking
users if they're okay if its yarn shim downloads a particular version
from the registry:

  ! Corepack is about to download https://repo.yarnpkg.com/.../yarn.js
  ? Do you want to continue? [Y/n]

^This will break tests once we either explicitly move our
corepack == 0.20.0 to corepack >= 0.25.0 implicitly by adopting a newer
NodeJS releases in the multi-stage Docker build conversion future
patches will bring.
Either way, let's prepare for it by setting the new env variable
corepack introduced for this COREPACK_ENABLE_DOWNLOAD_PROMPT [2].

[1] https://github.com/nodejs/corepack/releases/tag/v0.25.0
[2] nodejs/corepack#360 (comment)

Signed-off-by: Erik Skultety <eskultet@redhat.com>
github-merge-queue bot pushed a commit to containerbuildsystem/cachi2 that referenced this pull request Jun 28, 2024
Starting with corepack 0.25.0 [1], corepack now defaults to asking
users if they're okay if its yarn shim downloads a particular version
from the registry:

  ! Corepack is about to download https://repo.yarnpkg.com/.../yarn.js
  ? Do you want to continue? [Y/n]

^This will break tests once we either explicitly move our
corepack == 0.20.0 to corepack >= 0.25.0 implicitly by adopting a newer
NodeJS releases in the multi-stage Docker build conversion future
patches will bring.
Either way, let's prepare for it by setting the new env variable
corepack introduced for this COREPACK_ENABLE_DOWNLOAD_PROMPT [2].

[1] https://github.com/nodejs/corepack/releases/tag/v0.25.0
[2] nodejs/corepack#360 (comment)

Signed-off-by: Erik Skultety <eskultet@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants