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

Use consistent LTS node version: v20 aka lts/iron #5946

Merged
merged 2 commits into from
Aug 7, 2024

Conversation

thejonroberts
Copy link
Contributor

@thejonroberts thejonroberts commented Jul 26, 2024

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 and bin/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.

@github-actions github-actions bot added dependencies Pull requests that update a dependency file javascript for use by Github Labeler to mark pull requests that update Javascript code labels Jul 26, 2024
Comment on lines 27 to +28
"ghcr.io/devcontainers/features/node:1": {
"version": "latest"
"version": "20.16.0"
Copy link
Contributor Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

interesting

Dockerfile Outdated Show resolved Hide resolved
@thejonroberts thejonroberts force-pushed the node-version-20 branch 3 times, most recently from 4a02ae1 to 996ac14 Compare July 31, 2024 20:33
@thejonroberts thejonroberts marked this pull request as ready for review July 31, 2024 20:44
Copy link
Collaborator

@elasticspoon elasticspoon left a 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
Comment on lines 110 to 111
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.
Copy link
Collaborator

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.

Copy link
Contributor Author

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:

  1. 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
  1. 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
  1. 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

Copy link
Contributor Author

@thejonroberts thejonroberts Aug 1, 2024

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)

Copy link
Contributor Author

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.
Copy link
Collaborator

@elasticspoon elasticspoon left a 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)

@thejonroberts
Copy link
Contributor Author

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.

@elasticspoon elasticspoon merged commit 965e39d into rubyforgood:main Aug 7, 2024
17 of 18 checks passed
@thejonroberts thejonroberts deleted the node-version-20 branch August 7, 2024 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file javascript for use by Github Labeler to mark pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants