Skip to content

docs: remove references to installing with yarn in favor of npm #5518

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Aug 30, 2022

Conversation

edvincent
Copy link
Contributor

Follow-up of #5071 (comment) to update the docs.

Only left references to yarn commands for the development process.

@edvincent edvincent requested a review from a team as a code owner August 29, 2022 20:19
Copy link
Contributor

@jsjoeio jsjoeio left a 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!

@jsjoeio jsjoeio self-assigned this Aug 29, 2022
@jsjoeio jsjoeio added the docs Documentation related label Aug 29, 2022
@jsjoeio jsjoeio added this to the August 2022 milestone Aug 29, 2022
@edvincent edvincent force-pushed the docs-yarn branch 2 times, most recently from dd7c740 to ead5c68 Compare August 29, 2022 20:31
@edvincent edvincent requested a review from jsjoeio August 29, 2022 21:13
@codecov
Copy link

codecov bot commented Aug 29, 2022

Codecov Report

Merging #5518 (ae4d210) into main (101d4ee) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           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.

@edvincent
Copy link
Contributor Author

Actually... Where's my head. Forgot about the --unsafe-perm that npm requires... Updating coming up!

@jsjoeio
Copy link
Contributor

jsjoeio commented Aug 29, 2022

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?

@edvincent
Copy link
Contributor Author

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.

@jsjoeio
Copy link
Contributor

jsjoeio commented Aug 29, 2022

I honestly can't remember. @code-asher might know. Probably fine to add for now then we can revert/fix if needed.

@code-asher
Copy link
Member

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.

@edvincent
Copy link
Contributor Author

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.

@code-asher
Copy link
Member

code-asher commented Aug 30, 2022 via email

@code-asher code-asher merged commit ef3f4e8 into coder:main Aug 30, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
docs Documentation related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants