-
-
Notifications
You must be signed in to change notification settings - Fork 17
Devcontainer support #161
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
base: main
Are you sure you want to change the base?
Devcontainer support #161
Conversation
code. minFormatted and maxFormatted add a bit of unnecessary overhead for a consumer to do any logic or tracking of the min/max range (eg, for telemetry).
39457b3 to
d65a553
Compare
d65a553 to
1f6f8c4
Compare
Dockerfile
Outdated
| @@ -0,0 +1,3 @@ | |||
| FROM node:24.4-bookworm-slim | |||
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.
Why bookworm?
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.
Oh. Huh. I think the first page I found with the releases and LTS schedule made it look like bookworm was the current LTS version. Found another chart that’s clearer.
How do you feel about alpine?
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.
Alpine is an unofficial build too. See: nodejs/docker-node#2363
but I guess that's ok for this project.
Also 24.6 is the last node version that honors the nodejs CLI flags.
| @@ -0,0 +1,3 @@ | |||
| FROM node:24.6-trixie-slim | |||
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.
Can we avoid fixing a semver-minor version? What about node:24-alpine only?
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.
Not with the outstanding issue with commandline flags and tests.
RafaelGSS
left a comment
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.
You should rebase and keep only the devcontainer.json changes. Feel free to land it once you remove the two unrelated commit.
While npmjs.org is trying to take important steps to reduce supply chain attacks, working with, or indeed on, other people's OSS projects is still a bit fraught at the moment.
While dev containers are really meant for more elaborate workflows, a barebones implementation seems to be pretty okay for isolating
npm installfrom having access to things it should not have access to.