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

Fix : recreate the termux guide to adapt the recent changes #4472

Merged
merged 10 commits into from
Nov 12, 2021
Merged

Fix : recreate the termux guide to adapt the recent changes #4472

merged 10 commits into from
Nov 12, 2021

Conversation

arHSM
Copy link
Contributor

@arHSM arHSM commented Nov 9, 2021

Termux nodejs-lts changed from v14 to v16 and there are many issues people are facing such as with argon2. Hence I recommend changing it to this install process which is comparably better and has one less issue :^)

I've also added some extra things such as installing GO and Python, idk about the TOC tree but this is pretty much it.

Fixes
Sort of fixes
#4415
#4470
#4394

Termux nodejs-lts changed from v14 to v16 and there are many issues people are facing such as with argon2. Hence I recommend changing it to this install process which is comparably better and has one less issue :^)

I've also added some extra things such as installing GO and Python, idk about the TOC tree but this is pretty much it.
@arHSM arHSM requested a review from a team as a code owner November 9, 2021 06:59
@im-coder-lg
Copy link
Contributor

@arHSM this is good! @jsjoeio you can sanction the checks. @arHSM be sure to:

  1. Clone locally, run yarn fmt rebase(use the GitHub UI and merge the latest commits), and push the change up here
  2. not get scared that the docs are failing, since you edited on a fork, you don't have the tokens to publish changes to Coder's Vercel deployment, but it's not required. The build, e2e tests and pre-build checks are required though.

@arHSM
Copy link
Contributor Author

arHSM commented Nov 9, 2021

Ok its night rn, will do tomorrow

@jsjoeio jsjoeio self-assigned this Nov 9, 2021
@im-coder-lg
Copy link
Contributor

Okay, now, do these steps:

  1. Clone this repo and open a new tab with your code-server fork, and remember, be on your PR branch, ie: arHSM:patch-2
  2. Open your terminal of choice, make sure Node JS and Yarn is installed, the CLI works and on your terminal, run yarn install && yarn fmt.
  3. While it is running, switch to your branch inside your fork, and check for a Fetch and merge option. Fetch and merge the latest commits, and push the yarn fmt result here(there will be a minor change which is important)

Then, wait for the checks to complete(if it doesn't, wait for a reply from @jsjoeio, he will help on this.)

@im-coder-lg
Copy link
Contributor

im-coder-lg commented Nov 10, 2021

Oh no, only a maintainer can change the docs! Since a maintainer has access to the repo!

Let me explain elaborately. Imagine, just imagine that I am a maintainer of code-server. I'd have access to:

  1. the repo itself
  2. Coder's Vercel deployment(Coder.com)
  3. Secret tokens

So, it means that if I make a docs edit, I'd make it from inside the repo since we need the tokens(secret) so that the tokens are applied. Whenever I try merging another branch inside code-server to the main branch, I'd get a docs deployment reply, I'd follow steps on making the PR, like running yarn fmt locally, etc.
(Note: Imagination ends here)

So, if the docs are approved+the docs check finally works, great! If it doesn't, either the PR can get merged, or a maintainer can make the PR and we can review, which is better.

@im-coder-lg
Copy link
Contributor

im-coder-lg commented Nov 10, 2021

Now, merge the latest commits from the base branch to your PR branch. This would help with merging.

@arHSM
Copy link
Contributor Author

arHSM commented Nov 10, 2021

image
looks good

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.

Hi @arHSM!

First, thank you so much for your PR ♥️ We really appreciate it.

I think there is some hesitation with this doc change mainly because it feels like it introduces a lot of extra steps compared to our previous guide. I'm wondering if we could find a happy middle ground?

The Installing Python and Go additions are fantastic! I'm wondering if we can simplify this guide somehow. In my head, I'm thinking something like:

Get Termux
Install dependencies (i.e. Node/npm, nvm)
Install code-server
Run code-server
Do you think it's possible to keep it as simple as that? I'd rather us not suggest switching users or doing much beyond those four steps but maybe it's not possible due to our current limitations.

Let us know!

docs/termux.md Outdated
- [Backspace doesn't work](#backspace-doesnt-work)
- [Git won't work in `/sdcard`](#git-wont-work-in-sdcard)
- [Extra](#extra)
- [Install GO](#install-go)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- [Install GO](#install-go)
- [Install Go](#install-go)

@codecov
Copy link

codecov bot commented Nov 10, 2021

Codecov Report

Merging #4472 (a0a00af) into main (cdf0deb) will not change coverage.
The diff coverage is n/a.

❗ Current head a0a00af differs from pull request most recent head f7f6e1b. Consider uploading reports for the commit f7f6e1b to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4472   +/-   ##
=======================================
  Coverage   66.46%   66.46%           
=======================================
  Files          30       30           
  Lines        1625     1625           
  Branches      330      330           
=======================================
  Hits         1080     1080           
  Misses        463      463           
  Partials       82       82           

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 cdf0deb...f7f6e1b. Read the comment docs.

@arHSM
Copy link
Contributor Author

arHSM commented Nov 10, 2021

I'd rather us not suggest switching users or doing much beyond those four steps but maybe it's not possible due to our current limitations.

yeah i think we can remove those steps, but can we atleast add a note like "consider creating a new user instead of using root, read more [here](link to guide)"?

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 10, 2021

yeah i think we can remove those steps, but can we atleast add a note like "consider creating a new user instead of using root, read more [here](link to guide)"?

Sure, if you think there's a specific use case that solves, we could definitely add that as a guide! The guide could even by "how to add a new user" or "why you should consider adding a new user"

@arHSM
Copy link
Contributor Author

arHSM commented Nov 10, 2021

I have now removed those unnecessary steps and added a quote block at the end of the process stating the following

Consider using a new user instead of root, read here why using root is not recommended.
Learn how to add a user here.

and i also noticed that Install & Upgrade was ## and not ### so i made that change too

docs/termux.md Outdated
Comment on lines 59 to 60
> Consider using a new user instead of root, read [here](https://www.howtogeek.com/124950/htg-explains-why-you-shouldnt-log-into-your-linux-system-as-root/) why using root is not recommended.\
> Learn how to add a user [here](https://gist.github.com/arHSM/62242c343efc2827861ddc38e485d7df).
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not against using a gist here but if you accidentally delete it in the future, it breaks our docs 😛

What if we move this into it's own section under Extra? And then you can use the text directly instead of linking the gist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok!

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 10, 2021

and i also noticed that Install & Upgrade was ## and not ### so i made that change too

you rock! I requested one last change and then I think we'll be good to go! ♥️

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.

Awesome! I think we're good to go once CI passes (ignore Docs Preview)

@arHSM
Copy link
Contributor Author

arHSM commented Nov 10, 2021

Wait don't merge yet, I have found a typo in step 5

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 10, 2021

@arHSM you give me the thumbs up when this is ready to merge!

@jsjoeio jsjoeio merged commit 6606040 into coder:main Nov 12, 2021
GirlBossRush pushed a commit that referenced this pull request Nov 15, 2021
* Fix : recreate the termux guide to adapt the recent changes

Termux nodejs-lts changed from v14 to v16 and there are many issues people are facing such as with argon2. Hence I recommend changing it to this install process which is comparably better and has one less issue :^)

I've also added some extra things such as installing GO and Python, idk about the TOC tree but this is pretty much it.

* yarn-fmt and minor typos

#4472 (comment)

* Fix : replace unnecessary steps to be linked to a guide

* Change from private gist to a section in Extra

* Remove reference to non-existent step

* ready to merge!

Co-authored-by: Joe Previte <[email protected]>
GirlBossRush pushed a commit that referenced this pull request Nov 15, 2021
* Refactor vscode router to load async.

* Bump vscode.

* fix volumes (#4497)

* Fix : recreate the termux guide to adapt the recent changes (#4472)

* Fix : recreate the termux guide to adapt the recent changes

Termux nodejs-lts changed from v14 to v16 and there are many issues people are facing such as with argon2. Hence I recommend changing it to this install process which is comparably better and has one less issue :^)

I've also added some extra things such as installing GO and Python, idk about the TOC tree but this is pretty much it.

* yarn-fmt and minor typos

#4472 (comment)

* Fix : replace unnecessary steps to be linked to a guide

* Change from private gist to a section in Extra

* Remove reference to non-existent step

* ready to merge!

Co-authored-by: Joe Previte <[email protected]>

Co-authored-by: Jinu <[email protected]>
Co-authored-by: Han Seung Min - 한승민 <[email protected]>
Co-authored-by: Joe Previte <[email protected]>
@arHSM arHSM deleted the patch-2 branch November 24, 2021 09:22
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.

3 participants