-
Notifications
You must be signed in to change notification settings - Fork 6.2k
docs: remove references to installing with yarn in favor of npm #5518
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
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.
Woo! Thanks for doing this!
dd7c740 to
ead5c68
Compare
Codecov Report
Merging #5518 (ae4d210) into main (101d4ee) will not change coverage.
The diff coverage isn/a.
Additional details and impacted files
@@ Coverage Diff @@ ## main #5518 +/- ## ======================================= Coverage 72.44% 72.44% ======================================= Files 30 30 Lines 1673 1673 Branches 366 366 ======================================= Hits 1212 1212 Misses 398 398 Partials 63 63
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update 101d4ee...ae4d210. Read the comment docs.
Actually... Where's my head. Forgot about the --unsafe-perm that npm requires... Updating coming up!
ead5c68 to
ae4d210
Compare
Actually... Where's my head. Forgot about the --unsafe-perm that npm requires... Updating coming up!
Nice catch! Remind me again, why do we need this?
Nice catch! Remind me again, why do we need this?
https://github.com/coder/code-server/blob/main/ci/build/npm-postinstall.sh#L95-L101
Now, is it actually needed? Not sure. Definitely something I'm happy to look into in the future.
I honestly can't remember. @code-asher might know. Probably fine to add for now then we can revert/fix if needed.
I think we only need it if someone is installing code-server with root because NPM will drop permissions in the post install scripts making the code-server install fail.
I think we only need it if someone is installing code-server with root because NPM will drop permissions in the post install scripts making the code-server install fail.
So we should add a check for the user running the script, and only show the warning if it's root? Happy to play around with that and send a separate PR for that in the next couple of days.
Follow-up of #5071 (comment) to update the docs.
Only left references to
yarncommands for the development process.