feat: replace deb.nodesource.com with n, fixes #5509 #5521

hanoii · 2023-11-09T12:22:56Z

I figured this one was quick to do if you want to give it a try.

github-actions · 2023-11-09T12:29:05Z

Download the artifacts for this pull request:

See Testing a PR

rfay

Requires installation in the Dockerfile right?

hanoii · 2023-11-09T18:59:42Z

Requires installation in the Dockerfile right?

what do you mean? It modifies the Dockerfile yes, but not the main ddev image, just the one for each project. It modifies as before when you specify a version.

rfay · 2023-11-09T19:57:17Z

I think the initial nodejs installation should be done via n probably?

ddev/containers/ddev-php-base/Dockerfile

Lines 64 to 65 in 4932337

    
           RUN curl -sSL https://deb.nodesource.com/gpgkey/nodesource-repo.gpg.key | gpg --dearmor -o /usr/share/keyrings/nodesource.gpg && \ 
        
               echo "deb [signed-by=/usr/share/keyrings/nodesource.gpg] https://deb.nodesource.com/node_$NODE_VERSION.x nodistro main" > /etc/apt/sources.list.d/nodesource.list

hanoii · 2023-11-09T21:02:56Z

The recommended way is actually installing it through npm, so it assumes node is already in the system, while this sound as cat and mouse, I find it very useful, since It works much easier than nvm and i can use it on platform.sh for instance to change version.

I thought the node version was on the original image coming form debian, not that you were setting a default initial version yourself.

So we can do either, continue installing the default node version the way you are doing and use n only for changing it, or, as per https://github.com/tj/n#installation you can do:

curl -fsSL https://raw.githubusercontent.com/tj/n/master/bin/n | bash -s 18

18 or whatever you want.

From https://docs.platform.sh/languages/nodejs/node-version.html, if you compare the installation for n vs nvm, you see it's much more straightforward.

rfay

This seems like it's close, but the docs have to be updated explaining the range of versions allowed, and the template.go comment instructions need statement that minor versions are supported right?

What about adding info about "Why do we need nvm at all then"? (Do we?)

rfay · 2023-11-16T02:17:59Z

pkg/ddevapp/nodejs_test.go

@@ -41,7 +40,8 @@ func TestNodeJSVersions(t *testing.T) {
 	err = app.Start()
 	require.NoError(t, err)

-	for _, v := range nodeps.GetValidNodeVersions() {
+	// Testing some random versions
+	for _, v := range []string{"6", "10", "14", "16", "20"} {


Shouldn't this have some examples with minor versions?

hanoii · 2023-11-16T11:59:58Z

What about adding info about "Why do we need nvm at all then"? (Do we?)

This an interesting question. I don't think we do, but some people are used to it, so removing it altogether is a bit step, I guess that if you removed phpmyadmin you can do that with this and move it to an add on, which might not be a bad idea but I'd do that as a separate issue/PR.

I'll add some more info to template.go, I did change that to account for any "node version" but I can try thinking of something more specific.

hanoii · 2023-11-16T12:04:15Z

Further more, using n once you start the project is not possible (this can be improved with permissions):

n install 18
  installing : node-v18.18.2
       mkdir : /usr/local/n/versions/node/18.18.2
mkdir: cannot create directory ‘/usr/local/n/versions/node/18.18.2’: Permission denied

  Error: sudo required (or change ownership, or define N_PREFIX)

Obviously N_PREFIX can point to something else, even the shared mnt, but again, if people are used to nvm I thought easier to just leave n as a system node installer and nvm as a user runtime node installer. I wonder how much nvm is used, maybe you have that information out of your analytics?

hanoii · 2023-11-16T12:30:20Z

Updated docs and tests

stasadev

Impressive simplification!

It also "fixes" the problem described here:

ci: gather debugging information about possible TestExtraPackages failure #5473

rfay · 2023-11-18T01:30:23Z

Rebased.

rfay

Rebased and added minor docs changes. I think this is pretty fantastic. It will resolve the conflict people always feel between major version and nvm.

mandrasch · 2024-01-24T20:02:22Z

Awesome change, thanks so much @hanoii! 👏 👏 👏

rfay · 2024-01-26T11:27:33Z

I'm quite a fan of this as well. Simply fantastic.

tyler36 · 2024-01-28T23:58:22Z

Brilliant!

rfay · 2024-02-03T17:55:33Z

A followup on this:

nodejs inside the container is still the one in /usr/bin
node is in /usr/local/bin/node
Docs refer to nodejs a few places
We could easily update to add a link /usr/local/bin/nodejs -> /usr/local/bin/node to solve this.

hanoii requested a review from a team as a code owner November 9, 2023 12:22

github-actions bot added the enhancement label Nov 9, 2023

rfay marked this pull request as draft November 9, 2023 18:19

rfay reviewed Nov 9, 2023

View reviewed changes

rfay requested changes Nov 16, 2023

View reviewed changes

hanoii force-pushed the nodejs-version-n branch from 49a398c to 6094654 Compare November 16, 2023 12:01

hanoii requested a review from rfay November 16, 2023 12:29

rfay marked this pull request as ready for review November 17, 2023 16:23

rfay requested a review from a team as a code owner November 17, 2023 16:23

rfay requested a review from stasadev November 17, 2023 16:25

stasadev approved these changes Nov 17, 2023

View reviewed changes

rfay force-pushed the nodejs-version-n branch from 951f259 to b96c0f4 Compare November 18, 2023 01:30

rfay added this to the v1.22.5 milestone Nov 18, 2023

hanoii and others added 3 commits November 18, 2023 07:57

Replace deb.nodesource.com with n

7067a94

Tests and docs

ed4f20e

minor docs updates [skip ci]

a477a9e

rfay force-pushed the nodejs-version-n branch from b96c0f4 to a477a9e Compare November 18, 2023 14:58

rfay approved these changes Nov 18, 2023

View reviewed changes

run tests again

015c497

rfay merged commit c1d9bc6 into ddev:master Nov 19, 2023
17 checks passed

jfauske mentioned this pull request Dec 1, 2023

Error building web image after upgrading to 1.22.5 when nodejs_version set to <18 #5596

Closed

1 task

rfay mentioned this pull request Feb 3, 2024

fix: nodejs in web container should be link to /usr/local/bin/node #5774

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: replace deb.nodesource.com with n, fixes #5509 #5521

feat: replace deb.nodesource.com with n, fixes #5509 #5521

hanoii commented Nov 9, 2023

github-actions bot commented Nov 9, 2023 •

edited

rfay left a comment

hanoii commented Nov 9, 2023

rfay commented Nov 9, 2023

hanoii commented Nov 9, 2023 •

edited

rfay left a comment

rfay Nov 16, 2023

hanoii Nov 16, 2023

hanoii commented Nov 16, 2023

hanoii commented Nov 16, 2023 •

edited

hanoii commented Nov 16, 2023

stasadev left a comment

rfay commented Nov 18, 2023

rfay left a comment

mandrasch commented Jan 24, 2024

rfay commented Jan 26, 2024

tyler36 commented Jan 28, 2024

rfay commented Feb 3, 2024

feat: replace deb.nodesource.com with n, fixes #5509 #5521

feat: replace deb.nodesource.com with n, fixes #5509 #5521

Conversation

hanoii commented Nov 9, 2023

github-actions bot commented Nov 9, 2023 • edited

rfay left a comment

Choose a reason for hiding this comment

hanoii commented Nov 9, 2023

rfay commented Nov 9, 2023

hanoii commented Nov 9, 2023 • edited

rfay left a comment

Choose a reason for hiding this comment

rfay Nov 16, 2023

Choose a reason for hiding this comment

hanoii Nov 16, 2023

Choose a reason for hiding this comment

hanoii commented Nov 16, 2023

hanoii commented Nov 16, 2023 • edited

hanoii commented Nov 16, 2023

stasadev left a comment

Choose a reason for hiding this comment

rfay commented Nov 18, 2023

rfay left a comment

Choose a reason for hiding this comment

mandrasch commented Jan 24, 2024

rfay commented Jan 26, 2024

tyler36 commented Jan 28, 2024

rfay commented Feb 3, 2024

github-actions bot commented Nov 9, 2023 •

edited

hanoii commented Nov 9, 2023 •

edited

hanoii commented Nov 16, 2023 •

edited