Skip to content

feat!: prompt user before downloading software - #360

Merged
aduh95 merged 4 commits into
mainfrom
log-network-activity
Jan 30, 2024
Merged

feat!: prompt user before downloading software#360
aduh95 merged 4 commits into
mainfrom
log-network-activity

Conversation

@aduh95

@aduh95aduh95 commented Jan 23, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

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

merceyz commented Jan 24, 2024•
edited
Loading

Copy link
Copy Markdown
Member

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

styfle commented Jan 24, 2024

Copy link
Copy Markdown
Member

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

@aduh95

aduh95 commented Jan 24, 2024•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

styfle commented Jan 24, 2024

Copy link
Copy Markdown
Member

Maybe print to stderr instead of stdout?

@mhdawson

mhdawson commented Jan 24, 2024

Copy link
Copy Markdown
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
styfle
styfle reviewed Jan 26, 2024
View reviewed changes
Comment threadREADME.md Outdated
@aduh95aduh95 changed the title feat: log network activity by defaultfeat!: prompt user before downloading softwareJan 30, 2024
@aduh95
aduh95 merged commit 6b8d87f into mainJan 30, 2024
@aduh95
aduh95 deleted the log-network-activity branch January 30, 2024 22:38
@github-actionsgithub-actionsBot mentioned this pull request Jan 30, 2024
Merged
@merceyzmerceyz mentioned this pull request Feb 20, 2024
Closed
@merceyz

merceyz commented Feb 21, 2024•
edited
Loading

Copy link
Copy Markdown
Member

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

jgarber623 commented Feb 22, 2024

Copy link
Copy Markdown

this change is problematic.

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

@aduh95aduh95 mentioned this pull request Feb 26, 2024
Closed
@targostargos mentioned this pull request Mar 5, 2024
Closed
@simonhaenischsimonhaenisch mentioned this pull request Mar 11, 2024
Merged
artemiyokulov
artemiyokulov reviewed Mar 14, 2024
View reviewed changes
Comment threadmkshims.ts
const entryPath = path.join(distDir, `${binaryName}.js`);
const entryScript = [
`#!/usr/bin/env node`,
`process.env.COREPACK_ENABLE_DOWNLOAD_PROMPT??='1'`,

@artemiyokulovartemiyokulovMar 14, 2024

Copy link
Copy Markdown

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

@styflestyfleMar 14, 2024•
edited
Loading

Copy link
Copy Markdown
Member

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

corepack/package.json

Line 13 in a3e3e97

"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/hermeto that referenced this pull request Jun 27, 2024
@eskultety
package_managers: yarn: Disable corepack prompt when fetching yarn
5144af5
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/hermeto that referenced this pull request Jun 28, 2024
@eskultety
package_managers: yarn: Disable corepack prompt when fetching yarn
61dbb00
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-queueBot pushed a commit to hermetoproject/hermeto that referenced this pull request Jun 28, 2024
@eskultety
package_managers: yarn: Disable corepack prompt when fetching yarn
b354632
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>
@kyori19kyori19 mentioned this pull request Jul 29, 2024
Merged
@ItamarGronich

ItamarGronich commented Mar 2, 2026

Copy link
Copy Markdown

This was a bad decision... broke so many flows...

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arcanisarcanisarcanis approved these changes
+2 more reviewers
@styflestyflestyfle left review comments
@artemiyokulovartemiyokulovartemiyokulov left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@aduh95@merceyz@styfle@mhdawson@jgarber623@ItamarGronich@arcanis@artemiyokulov