-
-
Notifications
You must be signed in to change notification settings - Fork 579
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
Make nodejs 18.14.0 installation work, changed npm --unsafe-perm behavior #4615
Make nodejs 18.14.0 installation work, changed npm --unsafe-perm behavior #4615
Conversation
Fixes #4614
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.
Thanks so much for the contribution. I don't think this is relevant to this issue; npm at this point in the container is npm 8.19.2. Are you able to recreate the problem you're having with HEAD? brew install --HEAD --fetch-head ddev
?
@rfay After I test that, what would be the brew command to run to go back to the stable release like normal? |
|
Testing now on local. |
@@ -79,7 +79,7 @@ RUN apt-get -qq install --no-install-recommends --no-install-suggests -y \ | |||
postgresql-client \ | |||
sqlite3 | |||
|
|||
RUN npm config set unsafe-perm true && npm install --global gulp-cli yarn | |||
RUN npm install --unsafe-perm=true --global gulp-cli yarn |
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.
The reality is I'm not completely sure why we even install nodejs this early, when we're going to do it later in post-build.
At this point, though, we're building with NODE_LTS=16, so is node 16 affected by this change?
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.
I don't see this error on node 16 - but I also haven't tested this PR to know it fixes things for us.
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.
Maybe we should remove this step here then?
Tried testing this on my local (which needs the pre.Dockerfile.certificates to load in extra certificates) and I got the following error:
|
I don't have the same cert setup as @davereid-pfg 's project, and head worked fine! So, I don't think this PR is needed.
|
Since NODE_LTS here is 16, this PR won't be necessary, but we'll definitely have to sort out when we switch the LTS default to 18. Which should be pretty soon. Thanks so much for taking the time to do this. Continuing conversation on short-term approaches in |
@davereid-pfg this PR couldn't not have worked OOTB because it doesn't reference the docker image that would be built from it. |
The Issue
Fixes #4614
How This PR Solves The Issue
Implements the same change to the npm command in the Dockerfile as #4610 did.
Manual Testing Instructions
Automated Testing Overview
Related Issue Link(s)
Release/Deployment Notes