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

Update admin install instructions for node 20 #1340

Merged
merged 4 commits into from
Dec 27, 2023

Conversation

audiodude
Copy link
Contributor

Instructions taken from https://github.com/nodesource/distributions, which is where the scary message in the ttps://deb.nodesource.com/setup_16.x setup script directs you.

Copy link

vercel bot commented Nov 20, 2023

@audiodude is attempting to deploy a commit to the Mastodon Team on Vercel.

A member of the Team first needs to authorize it.

@vmstan
Copy link
Contributor

vmstan commented Nov 20, 2023

The way you have it will install Node 20, but that's OK too.

@audiodude
Copy link
Contributor Author

The way you have it will install Node 20, but that's OK too.

Oops, thought I edited that. Thanks for the review!

@ThisIsMissEm
Copy link
Contributor

We actually expect Node 20 in 4.2.x, from the .nvmrc, so nodesource's node 20 is fine here.

@vmstan
Copy link
Contributor

vmstan commented Nov 20, 2023

I think leaving it at 20 is ideal. That's where the Docker setup is at now.

@vmstan
Copy link
Contributor

vmstan commented Nov 23, 2023

Fixes #1339

@vmstan
Copy link
Contributor

vmstan commented Nov 24, 2023

I think we could remove these lines as they're part of the previous instruction in the doc

sudo apt-get update
 sudo apt-get install -y ca-certificates curl gnupg

We can also remove the sudo elevation from the instructions as they're already running as root here.

We could also just remove the NODE_MAJOR variable and set it as 20, to keep it clearer.

@vmstan vmstan mentioned this pull request Nov 24, 2023
@vmstan vmstan added the install-guide Installing a Mastodon instance label Nov 24, 2023
@audiodude
Copy link
Contributor Author

I think we could remove these lines as they're part of the previous instruction in the doc

sudo apt-get update
 sudo apt-get install -y ca-certificates curl gnupg

We can also remove the sudo elevation from the instructions as they're already running as root here.

We could also just remove the NODE_MAJOR variable and set it as 20, to keep it clearer.

Updated, PTAL, thanks!

@vmstan
Copy link
Contributor

vmstan commented Nov 25, 2023

There's still a couple sudo in the pipe commands.

content/en/admin/install.md Outdated Show resolved Hide resolved
@vmstan vmstan requested a review from renchap November 25, 2023 17:33
@@ -45,7 +46,7 @@ apt install -y \
g++ libprotobuf-dev protobuf-compiler pkg-config nodejs gcc autoconf \
bison build-essential libssl-dev libyaml-dev libreadline6-dev \
zlib1g-dev libncurses5-dev libffi-dev libgdbm-dev \
nginx redis-server redis-tools postgresql postgresql-contrib \
nginx nodejs redis-server redis-tools postgresql postgresql-contrib \
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I dont like nodejs being in the "System packages" section here, and I would prefer apt update && apt install -y nodejs to be in the "Node.js" section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only thing is that there is a section for nodejs, and a section for PostgreSQL. Both sections instruct you to install a package repository for the respective software. It seems incongruous that for PostgreSQL you would then install it as part of "system packages" but not nodejs?

@ThisIsMissEm
Copy link
Contributor

ThisIsMissEm commented Nov 30, 2023

We could perhaps add documentation about upgrading to a next major version of Node.js, as 22 will become active LTS in October 2024.

This requires changing the sources from:

echo "deb [signed-by=/etc/apt/keyrings/nodesource.gpg] https://deb.nodesource.com/node_20.x nodistro main" | tee /etc/apt/sources.list.d/nodesource.list

to:

echo "deb [signed-by=/etc/apt/keyrings/nodesource.gpg] https://deb.nodesource.com/node_22.x nodistro main" | tee /etc/apt/sources.list.d/nodesource.list

@vmstan
Copy link
Contributor

vmstan commented Nov 30, 2023

We don't really target that yet though.

@ThisIsMissEm
Copy link
Contributor

We don't really target that yet though.

Yeah, but it's more to say "You've gotta go back add edit your sources to get updated versions!"

@audiodude
Copy link
Contributor Author

We could perhaps add documentation about upgrading to a next major version of Node.js, as 22 will become active LTS in October 2024.

This requires changing the sources from:

echo "deb [signed-by=/etc/apt/keyrings/nodesource.gpg] https://deb.nodesource.com/node_20.x nodistro main" | tee /etc/apt/sources.list.d/nodesource.list

to:

echo "deb [signed-by=/etc/apt/keyrings/nodesource.gpg] https://deb.nodesource.com/node_22.x nodistro main" | tee /etc/apt/sources.list.d/nodesource.list

I think this is outside of the scope of this PR.

@audiodude audiodude changed the title Update admin install instructions for node 18 (min version for 4.2.1) Update admin install instructions for node 20 Dec 1, 2023
@vmstan vmstan requested review from renchap and a team December 5, 2023 15:00
@andypiper andypiper self-assigned this Dec 7, 2023
@andypiper
Copy link
Sponsor Member

I'd prefer @renchap to review / merge this one (as I've not been through these steps myself).

@andypiper andypiper requested a review from a team December 11, 2023 22:48
@andypiper andypiper requested a review from a team December 11, 2023 22:48
Co-authored-by: tibequadorian <9560587+tibequadorian@users.noreply.github.com>
@vmstan
Copy link
Contributor

vmstan commented Dec 14, 2023

Good catch on the gpg location.

@renchap renchap merged commit ec5ea76 into mastodon:master Dec 27, 2023
1 check failed
@vmstan vmstan mentioned this pull request Dec 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
install-guide Installing a Mastodon instance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants