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

dockerfile written for building docker image #395

Merged
merged 3 commits into from
May 6, 2024

Conversation

TheMohit2003
Copy link
Contributor

PR description

How to test

  1. Ensure Docker is installed on your system.
  2. Navigate to the root directory of the project and run:
    docker build -t ruxalab .
  3. Once the image is built, run it using:
    docker run -d --env-file .env -p 5000:5000 ruxalab
  4. Visit http://localhost:5000 in your browser to view the application.

@TheMohit2003
Copy link
Contributor Author

@jvJUCA , i have written and tested the docker file . But , haven't update the readme with docker setup steps. And coming to env handling in the project , i have used --env-file flag to inject the env keys directly in the running container. I will soon send the updated readme on this branch itself .

Copy link

@asr2003 asr2003 left a comment

Choose a reason for hiding this comment

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

I would suggest having two docker files for prod and dev environments are helpful rather than a single Dockerfile.
@KarinePistili @jvJUCA Your suggestions also valuable here to enhance our project Deployment options

dockerfile Outdated
@@ -0,0 +1,28 @@

FROM node:16 AS build-stage
Copy link

Choose a reason for hiding this comment

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

Please use node:lts instead node:16 to mitigate risks of compatibility in future

Copy link
Member

Choose a reason for hiding this comment

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

Good suggestion!

dockerfile Outdated
RUN npm run build

# Serve the app using Node.js
FROM node:16 AS production-stage
Copy link

@asr2003 asr2003 Mar 22, 2024

Choose a reason for hiding this comment

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

Use node:alpine to reduce the size of image in final stage

Copy link
Member

Choose a reason for hiding this comment

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

Hello @asr2003 I think this are good suggestions!

Copy link
Member

Choose a reason for hiding this comment

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

I think it would good to use node latest here as well

Copy link

@asr2003 asr2003 Apr 11, 2024

Choose a reason for hiding this comment

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

Yes, we can use node lts for build stage. node-alpine is the lighter image which we can use safely to run the already built files in Stage-1. It's the mantra we follow to reduce docker image in setup :)

We can also use bookworm-slim image too

Copy link
Member

Choose a reason for hiding this comment

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

Cool!

@KarinePistili
Copy link
Member

@xemyst were you able to test it on your environment as well?

@KarinePistili KarinePistili requested review from MurilonND and removed request for xemyst April 11, 2024 18:22
Copy link

@MurilonND MurilonND left a comment

Choose a reason for hiding this comment

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

Hello @TheMohit2003, I just finished testing your commit, and for me, the code isn't working (tested after merging with the develop so this could be why), so I just changed the code and got it working.

Screenshot from 2024-04-15 15-03-16

Copy link
Member

@KarinePistili KarinePistili left a comment

Choose a reason for hiding this comment

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

Please review the changes provided by the reviewers and change accordingly

@TheMohit2003
Copy link
Contributor Author

@MurilonND , can you discuss the changes ?

@MurilonND
Copy link

Sure, what you want to know?

@TheMohit2003
Copy link
Contributor Author

Sure, what you want to know?

What error did you get, and what file changes did you make ?

@MurilonND
Copy link

Very simple, just changed the node version to alpine (optimization), and the ERROR that I got was:

[build-stage 6/6] RUN npm run build:
0.364 npm ERR! Missing script: "build"
0.364 npm ERR!
0.364 npm ERR! To see a list of scripts, run:
0.365 npm ERR! npm run
0.365
0.365 npm ERR! A complete log of this run can be found in:
0.366 npm ERR! /root/.npm/_logs/2024-04-15T17_30_45_835Z-debug-0.log

dockerfile:12

10 | COPY . .
11 |
12 | >>> RUN npm run build
13 |
14 | # Serve the app using Node.js

ERROR: failed to solve: process "/bin/sh -c npm run build" did not complete successfully: exit code: 1

The develop doesn't have a "build" script, instead, it has "build-dev" or "build-prod."

@KarinePistili
Copy link
Member

@TheMohit2003 will you be able to do the changes?

@TheMohit2003
Copy link
Contributor Author

@KarinePistili , My apologies for being late. I will send the commits by tomorrow. Just went through distro hopping ( linux )and need to setup stuff again.

@KarinePistili
Copy link
Member

Okay, thanks! @TheMohit2003

@TheMohit2003
Copy link
Contributor Author

Updated Dockerfile:

  • Changed the Node.js version in the build stage to node:lts to ensure long-term support and compatibility.
  • Switched to node:alpine in the production stage to reduce the final image size, improving deployment efficiency.
    @KarinePistili

@TheMohit2003
Copy link
Contributor Author

@KarinePistili , should I add docker image setup docs in readme ?

@KarinePistili
Copy link
Member

it would be nice @TheMohit2003 , thanks

Copy link

sonarcloud bot commented Apr 22, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@TheMohit2003
Copy link
Contributor Author

@KarinePistili, all tasks on my end are completed. If there are any further changes you need, please let me know. Otherwise, you are free to merge the PR when you are ready.

@asr2003
Copy link

asr2003 commented Apr 24, 2024

I will test your changes and provide any changes needed

@TheMohit2003
Copy link
Contributor Author

TheMohit2003 commented Apr 28, 2024

@asr2003 , did you review the changes ?

@JulioManoel
Copy link
Collaborator

Hey, how are you? @TheMohit2003, I just tested your pull request and for me, the code is not working

image

@JulioManoel
Copy link
Collaborator

JulioManoel commented May 6, 2024

I think it's interesting to put a dockerignore for node_modules

@TheMohit2003
Copy link
Contributor Author

@JulioManoel worked for me . Let me check again. Did you create a .env file with all the secrets before running the container?

@JulioManoel
Copy link
Collaborator

Wouldn't it be better to separate two dockerfiles, 1 for develop and another for production?

@JulioManoel
Copy link
Collaborator

@TheMohit2003 there was an error when building the container

@JulioManoel
Copy link
Collaborator

I have the .env, I gave npm i to the project and after that I tried to build docker using the README command:

docker build -t uxremotelab .

@TheMohit2003
Copy link
Contributor Author

npm ERR! network Client network socket disconnected before secure TLS connection was established
npm ERR! network This is a problem related to network connectivity.
npm ERR! network In most cases you are behind a proxy or have bad network settings.

@JulioManoel , This error indicates an issue with your internet connectivity. Please verify your internet connection and try building the Docker image again.

@JulioManoel
Copy link
Collaborator

Sorry it was an internet problem ksks 😅

@JulioManoel JulioManoel self-requested a review May 6, 2024 14:59
@JulioManoel JulioManoel merged commit 6685ad3 into ruxailab:develop May 6, 2024
2 checks passed
@KarinePistili
Copy link
Member

Thanks @TheMohit2003 !

@KarinePistili
Copy link
Member

Thank you for revision @JulioManoel @MurilonND @asr2003 and @xemyst

@TheMohit2003 TheMohit2003 deleted the docker branch May 6, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants