-
Notifications
You must be signed in to change notification settings - Fork 2k
docs: update examples to Node.js 22 and modernize Docker Compose syntax #2243
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?
Conversation
Signed-off-by: Mert Şişmanoğlu <[email protected]>
I'm also thinking about updating docker compose example too. Is it OK to do it in this PR? services:
node:
image: "node:22"
environment:
- NODE_ENV=production
volumes:
- ./:/home/node/app
ports: # use if it is necessary to expose the container to the host machine
- "8081:8081"
command: ["npm", "start"] |
Yes please 🙂 |
Signed-off-by: Mert Şişmanoğlu <[email protected]>
84f117e
to
d9df67d
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.
Pull Request Overview
Updates Node.js version references in documentation from older versions (8 and 16) to the latest LTS version 22, ensuring developers have current examples for Docker usage.
- Updated Node.js Docker image tags from
node:8
andnode:16
tonode:22
- Modernized docker-compose configuration by removing deprecated version field and updating syntax
- Updated port mappings and command format in docker-compose example
image: "node:8" | ||
user: "node" | ||
working_dir: /home/node/app | ||
image: "node:22" |
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 docker-compose example removes important configuration like user: "node"
and working_dir: /home/node/app
without explanation. These settings are security and functionality best practices that should be retained or their removal should be justified.
image: "node:22" | |
image: "node:22" | |
user: "node" | |
working_dir: /home/node/app |
Copilot uses AI. Check for mistakes.
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.
Really?
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 sorry for deleting them. I realized it's a best practice for not using root user. Also working_dir field is necessary because we're copying all files to /home/node/app
I think we can commit the Copilot's suggested commit.
@coderabbitai, could you provide a better PR subject and description? |
Summary from GitHub Copilot:
|
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.
Please also include any related changes in the PR title and description, not just the Node.js version. Thank you.
Hi @PeterDaveHello I updated the title and description. Sorry for the untested changes like deleting |
@mertssmnoglu no problem, just let me know when it's ready. |
Update examples to Node.js 22(LTS) and modernize Docker Compose syntax.
Description
Node.js Version
node:8
andnode:16
image tags tonode:22
in README.Docker Compose
command
to array syntax for clarity.expose
,user
, andworking_dir
fields for simplicity.user
field is against of Best Practices. Andworking_dir
is required to point the correct dir. Related commentMotivation and Context
As a developer, I would like to see latest LTS version in the documentation with the up to date Docker Compose example.
Types of changes
Checklist