Skip to content

Comments

Feat/docker#104

Merged
pooranjoyb merged 1 commit intosristy17:dwoc-2025from
AritraDey-Dev:feat/docker
Apr 2, 2025
Merged

Feat/docker#104
pooranjoyb merged 1 commit intosristy17:dwoc-2025from
AritraDey-Dev:feat/docker

Conversation

@AritraDey-Dev
Copy link
Contributor

@AritraDey-Dev AritraDey-Dev commented Feb 13, 2025

Fixes Issue #103

Changes proposed

Set up the project using one single command(docker compose up -d --build)

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Screenshots

Screenshot from 2025-02-13 20-33-20

Copy link
Owner

@sristy17 sristy17 left a comment

Choose a reason for hiding this comment

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

Great job, @AritraDey-Dev! 🎉 Thank you for submitting your pull request. Your contribution is valuable and we appreciate your efforts to improve our project.

We will promptly review your changes and offer feedback. Keep up the excellent work! Kindly remember to check our contributing guidelines

Copy link
Owner

@sristy17 sristy17 left a comment

Choose a reason for hiding this comment

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

app crashed?

@sristy17 sristy17 requested a review from pooranjoyb February 14, 2025 17:08
@AritraDey-Dev
Copy link
Contributor Author

app crashed?

No I have tested the required things on my side after running docker compose.

@sristy17 sristy17 added dwoc-2025 Issue under DWOC 2025 level 3 labels Feb 14, 2025
@sristy17
Copy link
Owner

app crashed?

No I have tested the required things on my side after running docker compose.

will see but it is crashing from my side while reviewing

@AritraDey-Dev
Copy link
Contributor Author

app crashed?

No I have tested the required things on my side after running docker compose.

will see but it is crashing from my side while reviewing

yes a small change you have to do on your end.You have to add NODE_ENV=development
in your .env if you are running locally(Basically when you are using mongo db atlas as your db).
Screenshot from 2025-02-14 22-59-03

@AritraDey-Dev
Copy link
Contributor Author

AritraDey-Dev commented Feb 14, 2025

app crashed?

No I have tested the required things on my side after running docker compose.

will see but it is crashing from my side while reviewing

yes a small change you have to do on your end.You have to add NODE_ENV=development in your .env if you are running locally(Basically when you are using mongo db atlas as your db). Screenshot from 2025-02-14 22-59-03

Should I create env.ex file for reference?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you're creating docker-compose.yaml then we don't need DockerFile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no we will still need it,otherwise it cann't build the docker image right.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, it's nothing like that, both serve different use cases. The application is just one Node.js app with MongoDB and we don’t need to manage multiple containers or configurations separately, we can indeed just use a single Dockerfile for this. That's why I mentioned either use one. I'd suggest to go with Dockerfile

We will be running a single container (Node.js app + MongoDB), you can have the MongoDB container running directly using docker run as we have the connection handled directly within your Node.js app. Why to create a separate instance for MongoDB again.? Just go ahead with Dockerfile

Copy link
Contributor Author

@AritraDey-Dev AritraDey-Dev Feb 16, 2025

Choose a reason for hiding this comment

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

Yeah how the repo is set up,your idea is correct I will just make a single dockerfile.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are already provided with creds for DB, you shouldn't hardcode it. If you wanna pass them them do it with env in docker-compose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No like if we do docker compose up,then the db url will be mongodb://MindDrive:MindDrive@mongo:27017/dev-db?authSource=admin but if we look at db.conf.js file,it is taking the db url from there.
`export const DB_USER = process.env.DB_USER;
export const DB_PASSWORD = process.env.DB_PASSWORD;
export const DB_APPNAME = process.env.DB_APPNAME;

export const MONGODB_URI = mongodb+srv://${DB_USER}:${DB_PASSWORD}@dev-db.3tzynec.mongodb.net/?retryWrites=true&w=majority&appName=${DB_APPNAME};so when ever the user dodocker compose up -d ` it is not taking docker mogno db url it is taking from the db.conf file.So it will go down.

so i implemented it like this if someone mention NODE_ENV(which is not the case when you are doing docker compose) then it will take docker db url.Or if you are using set up like...
npm install followed by npm run dev in that case you have to add NODE_ENV in your .env.So that fixes the issue.Actually most of the cases we keep our db url in our .env which is not the case in this codebase.
I hope it helps.
Screenshot from 2025-02-16 03-12-27

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No like if we do docker compose up,then the db url will be mongodb://MindDrive:MindDrive@mongo:27017/dev-db?authSource=admin but if we look at db.conf.js file,it is taking the db url from there. `export const DB_USER = process.env.DB_USER; export const DB_PASSWORD = process.env.DB_PASSWORD; export const DB_APPNAME = process.env.DB_APPNAME;

export const MONGODB_URI = mongodb+srv://${DB_USER}:${DB_PASSWORD}@dev-db.3tzynec.mongodb.net/?retryWrites=true&w=majority&appName=${DB_APPNAME};so when ever the user dodocker compose up -d ` it is not taking docker mogno db url it is taking from the db.conf file.So it will go down.

so i implemented it like this if someone mention NODE_ENV(which is not the case when you are doing docker compose) then it will take docker db url.Or if you are using set up like... npm install followed by npm run dev in that case you have to add NODE_ENV in your .env.So that fixes the issue.Actually most of the cases we keep our db url in our .env which is not the case in this codebase. I hope it helps. Screenshot from 2025-02-16 03-12-27

@pooranjoyb what is your idea on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here also you dont need to hardcode it. jsut pass the ENV while running the image. Check Docker Variables for reference.

@AritraDey-Dev

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove unnecessary LOC or formatting.

restart: always
ports:
- "27017:27017"
environment:
Copy link
Collaborator

Choose a reason for hiding this comment

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

these shouldn't be hardcoded. but should be passed as the environment vars.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this @AritraDey-Dev

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove factoring.

@pooranjoyb
Copy link
Collaborator

squash all commits in 1 commit. @AritraDey-Dev

@AritraDey-Dev
Copy link
Contributor Author

AritraDey-Dev commented Feb 16, 2025

@pooranjoyb Here are the things we need to do for docker setup.

  • write a docker file.
  • in dockeffile we have to build the node Image only.
  • pass the env file while setting up docker locally.
  • finally the command should look something like this docker build --env_file=.env -p 8080:8080 -t web .
    Please review once i am correct or not.

@pooranjoyb
Copy link
Collaborator

@pooranjoyb Here are the things we need to do for docker setup.

* write a docker file.

* in dockeffile we have to build the node Image only.

* pass the env file while setting up docker locally.

* finally the command should look something like this `docker build  --env_file=.env -p 8080:8080 -t web .`
  Please review once i am correct or not.

LGTM 👍🏼

@AritraDey-Dev AritraDey-Dev force-pushed the feat/docker branch 3 times, most recently from a96cbbb to 4a86011 Compare February 16, 2025 20:21
README.md Outdated

GEMINI_API_ENDPOINT=https://generativelanguage.googleapis.com/v1beta/models/gemini-1.5-flash-latest:generateContent
GEMINI_API_KEY=<gemini-api-key>
NODE_ENV=development
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this

@pooranjoyb
Copy link
Collaborator

Squash the commits in 1. @AritraDey-Dev

This reverts commit c23e3ae, reversing
changes made to bf4d96b.

fix: issue in setup
@AritraDey-Dev
Copy link
Contributor Author

Squash the commits in 1. @AritraDey-Dev

yes done.

@pooranjoyb
Copy link
Collaborator

The build is failing please check. I could see that it is able to pull only node:13
please lemme know which versions are you running?

image

@AritraDey-Dev
Copy link
Contributor Author

Screenshot from 2025-02-18 23-25-11
i tried again locally,it is working for me.Make sure you are logged in to dockerHub.
Do docker login
https://forums.docker.com/t/help-on-error-internal-load-metadata-for-docker-io-library-node-18-alpine/144360
I think you are not logged in to docker hub may be that is the issue.

@AritraDey-Dev
Copy link
Contributor Author

Screencast.from.2025-02-19.00-11-03.webm

@AritraDey-Dev
Copy link
Contributor Author

Screencast.from.2025-02-19.00-11-03.webm

@pooranjoyb can you check try to update docker in your system if docker login doesn't work.
also try to run this cmd to check whether docker running properly or not sudo systemctl status docker

@pooranjoyb
Copy link
Collaborator

Screencast.from.2025-02-19.00-11-03.webm

@pooranjoyb can you check try to update docker in your system if docker login doesn't work. also try to run this cmd to check whether docker running properly or not sudo systemctl status docker

Yes that was docker cache that was creating the issue. anyways, docker is working fine now. jsut had few issues.
Please debug this one.

image

the auth is failing and that leads to app crash.
also please remove the npm run dev instead create a script for start, and map it with node app.js, we shouldn't use dev in docker

@AritraDey-Dev
Copy link
Contributor Author

AritraDey-Dev commented Feb 19, 2025

Screencast from 2025-02-19 23-08-24.webm
it is working fine in my side.
@pooranjoyb can you show logs of after crashing?it will help me to debug.

@AritraDey-Dev
Copy link
Contributor Author

@pooranjoyb also can you check whether it's able to connect to the db as sometimes in docker this issue is pretty regular.

@AritraDey-Dev
Copy link
Contributor Author

@pooranjoyb any update on this?

@pooranjoyb
Copy link
Collaborator

Will update accordingly. You can proceed with other issues if any.

@AritraDey-Dev
Copy link
Contributor Author

@pooranjoyb @sristy17 any update regarding this PR? Can this be merged?

@pooranjoyb pooranjoyb merged commit 585c366 into sristy17:dwoc-2025 Apr 2, 2025
@AritraDey-Dev AritraDey-Dev deleted the feat/docker branch April 17, 2025 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dwoc-2025 Issue under DWOC 2025 level 3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants