-
-
Notifications
You must be signed in to change notification settings - Fork 485
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
Use consistent LTS node version: v20 aka lts/iron #5946
Conversation
"ghcr.io/devcontainers/features/node:1": { | ||
"version": "latest" | ||
"version": "20.16.0" |
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.
This looks as if it is pinned at the patch, but that little :1
in line 27 means use latest major version, see devcontainer feature usage.
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.
interesting
4a02ae1
to
996ac14
Compare
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'm open to changing my mind but I think for simplicity of setting up the project I would rather have the version pinned and make it possible to use both nvm and asdf easily.
I am open to adding an .asdfrc
the use legacy flag to make nvmrc
the source of truth for both tools and removing the node bit from tool_versions completely.
README.md
Outdated
1. Install a current LTS version of Node. Running `nvm install` from this directory will read the `.nvmrc` file to install the correct version (lts/iron). | ||
- If you are already using asdf, see [legacy file support](https://asdf-vm.com/manage/configuration.html#legacy-version-file) to utilize the .`nvmrc` file. `.tool-versions` is also supported, but may be a bit behind the latest LTS version. |
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.
This bit doesn't work super nicely https://github.com/asdf-vm/asdf-nodejs?tab=readme-ov-file#partial-and-codename-versions
You need an rc file and to set an ENV variable.
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.
Good find. Okay, asdf sure is opinionated! Needing an ENV var sort of makes it a per-machine decision rather than per-project decision, and that's a bummer. We should avoid that I think.
Choices:
- Pin the version to a patch level in .nvmrc, let asdf use .nvmrc via .asdfrc as linked here:
- -- we'll need to update node/the .nvmrc more often
- +++ we will have one acceptable node version, no variation
- +: asdf users will be synced up, where previously they were 2 versions ahead
- Delete this from README, leave .tool-versions as node version source for asdf:
- ++: simple/closest to where we were at
- +/- asdf users will be slightly behind, but at least not way ahead as before
- ++ less updating node versions manually
- Leave lts/codename, take node out of .tool-versions, make asdf users mess with their ENV to be able to read node version:
- --- I wouldn't like doing it, lot of setup involved to get started
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 think I like 2, but 1 is fine with me)
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.
@elasticspoon I looked at installing specific versions for alpine/Dockerfile for a short while and got frustrated -- I am not sure how to guarantee a specific version there with the current docker image setup. This discussion also mentions alpine/apk will remove package versions, which made me nervous. I'm not sure you can guarantee an exact version short of using nvm or grabbing a binary. I undid my change to the dockerfile.
I also removed the asdf bit from the readme, since it is misleading.
Current state of this PR at least moves in the right direction. What do you think of taking this baby step and thinking more on how to ensure a pinned version on all environments later?
Some documentation & .nvmrc had EOL v14 (lts/fremium), github actions were EOL lts/gallium (16), .tool-versions (asdf) was v22. This changes all references to the current LTS node version (lts/iron or v20). In an effort to avoid updating this often, the codenames or semver ranges are used where possible, and .nvmrc is used as source for version where possible. This means there will be fewer differences across environments.
996ac14
to
ca1a377
Compare
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'm good with your suggestion. Looking over the documentation a bit I seems like asdf isn't actually one of your supported ruby version managers.
If you could add a note that points to how you would install an lts iron version on asdf that would be great. Maybe here:
https://github.com/rubyforgood/casa?tab=readme-ov-file#common-issues?
with either a link to https://github.com/asdf-vm/asdf-nodejs?tab=readme-ov-file#partial-and-codename-versions or asdf-vm/asdf-nodejs#382 (comment)
I added a note to readme node section. |
What github issue is this PR for, if any?
None. We can create an issue if this needs to be tracked.
What changed, and why?
Some documentation & the .nvmrc file had EOL v14 (lts/fremium), github actions were EOL v16 (lts/gallium). .tool-versions (asdf) was v22. It seems Dockerfile was loading whatever version apk grabs (which happened to be v20.15)
See Node Release Schedule for support timelines. The active LTS node version is v20. This PR changes all references to the current lts version v20 (lts/iron).
Codenames or semver ranges are used where possible, and the .nvmrc file is used as the version reference where possible. This means there will be far fewer differences in the node versions between environments, and fewer things to update when we upgrade node!
The only tool that does not accept semver ranges by default is asdf -
.tool-versions
only accepts a patch level version. I added a note for asdf users in the readme linking how to set up legacy file support (read the .nvmrc instead of .tool-versions).How is this tested? (please write tests!) 💖💪
I tested locally and things seemed alright.
bin/update
andbin/dev
both run with the same warnings I saw before, anyway.docker build
ran successfully. If there is anything else I can check locally, please let me know.CI/GitHub Actions seems to running, although with some flakiness in rspec and docker jobs.