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

Docker branch #27

Merged
merged 7 commits into from
Aug 25, 2022
Merged

Docker branch #27

merged 7 commits into from
Aug 25, 2022

Conversation

harsh8051
Copy link
Contributor

This changes have made to add docker support
Project running with docker on local machine

@VadimGush
Copy link
Member

Hi! Very happy to see someone contributing to the project! Thanks for opening a pull request!

I'm not sure that Docker image will be necessary very soon because we're hosting Kazan on Github Pages currently. But if we will need to deploy it somewhere else, it might be good option to have this application already packed in Docker image. So good idea.

I have also a couple of comments for this PR.

kazan:
build: .
volumes:
- ./:/usr/app
Copy link
Member

Choose a reason for hiding this comment

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

I usually prefer to not introduce docker-compose if there are no additional services required to run a docker container with the application. Docker Compose was build exactly to simplify deployment of multiple services. For just one docker container, it introduces unnecessary complexity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had same thought on adding docker-compose.yml . My take was if a developer is not familiar with IT Operational Tools/Docker it can go with single command
And as purpose of docker here is for development Bind Mount are necessary and some people might struggle with
binding local directory with docker directory as there issues like OS/ space in names of directory

if docker-compose.yml is not there we have remove docker compose instructions and add basic guide like -v $(pwd):/use/appfor Linux in Redmi.md

RUN npm install

# Set up a default command
CMD [ "npm","run","dev" ]
Copy link
Member

Choose a reason for hiding this comment

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

So is this docker container is created only for local development? Then I don't quite understand which problem it solves.

If you're working on the project locally, you need to have node and npm in order to develop this application (no IDE will work with the project without having node or npm installed). So any web-developer will already have all necessary dependencies and utilities installed in order to run this project locally.

Because of that, Docker (at least in my company) is usually used to package application for deployment to production servers. Where required utilities and dependencies might not be present.

Copy link
Member

Choose a reason for hiding this comment

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

If you have another approach to local development and you can tell about benefits of packaging application in Docker container for dev environment, then I will be happy to hear about it.

Copy link
Member

Choose a reason for hiding this comment

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

Though, it might be beneficial for people who want to try to run this project locally for the first time without installing any dependencies and tools. Ok, then I will approve. But I propose to remove docker-compose because it is currently unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi ,
Thank you for considering my contribution
Yes this is not user perspective change. This is for contributor's convenience. You are right about "it might be beneficial for people who want to try to run this project locally for the first time without installing any dependencies"

  • It was my personal choice. I have been working with docker so I thought why not add 3 files to remote if someone in future consider it . They do not have to put extra efforts or have to do less efforts
  • Second thought was in case someone face dependency/Platform issue in future we do not have to change local env setup or play around dependency version

Copy link
Member

Choose a reason for hiding this comment

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

@harsh8051 make sense. Sorry for asking you to remove docker-compose. I will add it back.

Copy link
Member

@VadimGush VadimGush left a comment

Choose a reason for hiding this comment

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

Just remove docker-compose, otherwise looks good to me!
And again, thanks you for contributing! 👍

@harsh8051
Copy link
Contributor Author

Done

@VadimGush VadimGush merged commit 42b5268 into gush-labs:master Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants