-
Notifications
You must be signed in to change notification settings - Fork 34
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
Docker containerisation of MVP #43
Conversation
1. Added Docker support using Dockerfile. 2. Added docker-compose to avoid the MongoDB setup. 3. Updated the README.md
@shubhamlatkar Can you please add the below bind commands for running the docker with local changes?
|
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 have reviewed the changes made and they seem to be necessary. Also can we add the bind commands in the README.md? Another warning that I came across was regrading the image's platform when building the image on arm64 devices, I shall look into it and update, please update if we already have any idea of getting rid of the warning.
Hi @AbhinavReddy-Dev could you add the logs of warnings you are getting. |
@shubhamlatkar sure, here's the warning for the image platform. |
Hi @AbhinavReddy-Dev you are mounting the wrong directory |
@shubhamlatkar Thank you, I was able to run it successfully using the below command. |
Actually |
yes, you're right! |
I am getting below error while executing docker run command - (base) sourabhkulkarni@Sourabhs-MacBook-Pro PantryNode % docker run -p 3000:3000 my-app
/app/node_modules/send/node_modules/http-errors/index.js:261 TypeError: Cannot read properties of undefined (reading '100') |
It doesn't look like the docker error but what is the build command that you have used? |
I used the below command to build the docker container - PS - The application runs on my local machine if I use my MongoDB Atlas instance, but I am getting the error mentioned only while using Docker. FYI, I am using mac with M2 chip. |
Can we make a merge of this? Please review @Jooms @sourabhk25 |
Hi @sourabhk25, @Jooms there seems to be an issue with http-errors package for ARM64 systems. |
How did you determine this? |
Do we need to worry about containerizing mongoDB? We are anyways migrating it to PostgreSQL, some coordination with backend team to provide with code that doesn't have mongo references and only PostgreSQL references should be enough to try containerizing this locally and then a merge into main. Please let me know if I have overlooked any issue in this approach. |
The issue is in node_modules/http-errors/index.js |
I'm on an AMD 64 system... So, how did you determine this?
|
MongoDB container was added to avoid the need for setting up a local database. However, we will need to remove it once the project is migrated to PostgreSQL. For now, we can skip setting up the database container. But I think there are no issues with the MongoDB container at this time. |
It's strange because I ran the program on a cloud platform that randomly assigns x86/AMD64 systems, and it worked without any issues. |
Figured it out When the Dockerfile is run on a repo that has already been built locally, there's a Solution: Add a Pushing a commit with the change. |
Runs locally for me. |
…etup' subsection to have similar styling to rest of readme
I updated the readme to exclude the old setup instructions as they weren't useful afaik for the docker container and updated the formatting under docker setup subsection to match formatting of the rest of the document. |
Nice catch @Jooms!!! |
Are the latest changes available in any branch to run it locally? Please quote it here. |
NVM was able to do it. |
Hi team can we merge the changes to the main? |
Fixed merge conflict. Approving once more Then @shubhamlatkar Can you merge as soon as there are 2 approvals? |
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.
Runs for me!
- [X] Use Docker to automate the build for the project, using the current dependencies and infrastructure - [X] Update the `README.md` to provide instructions on how to use the container(s)
README.md
to provide instructions on how to use the container(s)