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.

Sign up for GitHub

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

Jump to bottom

Test with Node 20, updated deps + js-wacz #3486

Merged
merged 2 commits into from Mar 27, 2024
Merged

Test with Node 20, updated deps + js-wacz #3486

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4fc0a5
Test with Node 20, updated deps + js-wacz
matteocargnelutti Mar 27, 2024
efa78c0
Pin Node.js
matteocargnelutti Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 2 additions & 4 deletions perma_web/Dockerfile
View file
Open in desktop
Expand Up @@ -36,10 +36,8 @@ RUN apt-get update \
libxml2-dev \
libxslt-dev

# pin node version -- see https://github.com/nodesource/distributions/issues/33
RUN curl -o nodejs.deb https://deb.nodesource.com/node_14.x/pool/main/n/nodejs/nodejs_14.19.3-deb-1nodesource1_$(dpkg --print-architecture).deb \
&& dpkg -i ./nodejs.deb \
&& rm nodejs.deb
# node.js (needs to be pinned?)
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend pinning everything in docker, ideally to the version in prod -- anything you don't pin will eventually waste an hour when it breaks on CI instead of on your local, or breaks on prod instead of CI. If keeping the pin updated is wasting more time than that I could be talked out of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The mechanism for installing node has changed; I believe it's no longer possible to pin to a specific .deb file exactly like we used to; the new mechanism is at nodesource/distributions#33 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

ideally to the version in prod

I note that we do not run nodejs in prod for Perma: it only ever runs inside the docker container

Without pinning here, I think the biggest possible consequences are, a discrepancy between different developers' environments, and an accidental upgrade to a new minor release breaking something that was previously working. Neither of those feel like big risks to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

See also https://github.com/nodesource/distributions#new-update-%EF%B8%8F and https://github.com/nodesource/distributions/wiki/How-to-select-the-Node.js-version-to-install

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I think this is fair @jcushman - I personally feel okay using this Node.js 20.x installer unpinned since it Node 20 is already in LTS mode. That of course doesn't mean it's not going to receive an update that is going to break our setup, but makes it less likely 😅 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bensteinberg @jcushman I believe this would work, should I push and see if it actually does?

# node.js 
RUN curl -fsSL https://deb.nodesource.com/setup_20.x | bash - && apt-get install -y nodejs=20.12.0-1nodesource1

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect, I was about to make this exact suggestion, and point out that I just installed Debian updates for node on a few machines.

Copy link
Contributor

Choose a reason for hiding this comment

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

@matteocargnelutti is there a build relic to remove after the fact, like the rm move we had before? asking just because this image is so big

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rebeccacremona As in: it got much bigger with that change? I am not sure 👀 !

Copy link
Contributor

Choose a reason for hiding this comment

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

@matteocargnelutti no, I just saw that there wasn't an rm or clean or whatever and thought maybe that was by accident. Ben just checked and it is no big deal, so never mind :-)

RUN curl -fsSL https://deb.nodesource.com/setup_20.x | bash - && apt-get install -y nodejs

# npm
COPY perma_web/package.json /perma/perma_web
Expand Down
15,105 changes: 8,912 additions & 6,193 deletions perma_web/npm-shrinkwrap.json
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions perma_web/package.json
View file
Open in desktop
Expand Up @@ -57,6 +57,7 @@
},
"dependencies": {
"@babel/runtime-corejs3": "^7.10.4",
"@harvard-lil/js-wacz": "^0.1.0",
"@sentry/browser": "^7.5.1",
"vue": "^3.4.21",
"vue-loader": "^17.4.2",
Expand Down