-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add Docker to deploy integration #7
base: master
Are you sure you want to change the base?
Conversation
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.
Hey @DanielSarmiento04 we'll need to make a few improvements to this PR before we can merge this (I've left a comment on the dockerfile). We also would need you to document the steps to use the Dockerfile by the end user to make this useful for everyone, thanks for you contribution!
Dockerfile
Outdated
|
||
COPY . . | ||
|
||
ENTRYPOINT [ "npm", "run", "start" ] |
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'll need to export port 4000 and manually copy the .env
file / add a way to expose environment variables to the container, like so:
COPY . .
COPY .env ./
ENTRYPOINT [ "npm", "run", "start" ]
EXPOSE 4000
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.
Sure,
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 add almost a commentaries to better explanation
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.
Let me know If I can improve something else
RUN npm install | ||
|
||
# Copy the rest of the source code | ||
COPY . . |
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.
Note that Copy statement will copy all files in folder no include in .dockerignore
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 add a step by step to run with docker, let me know is clear the information or if i need to add more information
The follow pull request try to improve the deployment method using docker, to do it I make the default 4000 port as inbound port and use oficial node image from docker hub