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 #441

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Docker #441

wants to merge 2 commits into from

Conversation

karllhughes
Copy link

This adds a Dockerfile and documentation for using Docker to run this project locally.

I didn't include any advanced commands assuming that anyone who's familiar with Docker will be able to figure those out, but if you run into trouble or think I could improve the docs, just let me know!

Copy link
Owner

@lionleaf lionleaf left a comment

Choose a reason for hiding this comment

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

I'm now back from traveling, apologies for the long delay.

Thanks! I finally had the excuse to play around with docker locally. It works well, however, I caught some typos in the readme; see suggested edits.

The other question is whether to rely on nikolaik/python-nodejs. I think I'd prefer to build something myself specifically for dwitter. But if everyone starts from ubuntu it would be slow to build for the first time, so the best approach is for me to maintain a docker image and host it somewhere?

I'm happy to keep nikolaik/ for now though, as it does work.

@@ -0,0 +1,13 @@
FROM nikolaik/python-nodejs:latest
Copy link
Owner

Choose a reason for hiding this comment

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

As mentioned, I'm new to docker, but is there a way we can avoid this dependency without too much hassle? I think I'd much prefer to start with ubuntu and build dependencies on there, or potentially host a base image myself.

Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you could start from the official docker python image and then install nodejs.

https://hub.docker.com/_/python

Or start from the official nodejs image and add python, if that turns out to be easier/better.

Copy link
Owner

Choose a reason for hiding this comment

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

I mean, we shouldn't even need nodejs. So the official python image sounds like the way to go! Might be missing some django stuff, but we can install that during setup?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would assume anything you need after python will be installed automatically with pip when running make which is already here. So just swapping out the base image for the python one should work if this is only intended for deployment. I see that the current node dependency is to install devDependencies from package.json for eslint support which is needed for development locally.

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds good. Official python image is the way to go then :)

@@ -37,6 +37,12 @@ Inspired by [arkt.is/t/](http://arkt.is/t/Yy53aWR0aD0yZTM7eC5maWxsUmVjdCgxNTAsMT
5. Get back in the main directory (`cd ../.. && make`) and use `make` command (install dependencies and set up database)
6. Continue with the fourth step from **Linux setup**.

#### **Docker**
1. Build the Docker image using the `Dockerfile` at the root directory of this project: `docker build -t dwitter .`.
2. Run the migrations: `docker run -it -v $(pwd):/app dwitter make migrate`.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
2. Run the migrations: `docker run -it -v $(pwd):/app dwitter make migrate`.
2. Run the migrations: `docker run -it -v $(pwd):/dwitter dwitter make migrate`.

#### **Docker**
1. Build the Docker image using the `Dockerfile` at the root directory of this project: `docker build -t dwitter .`.
2. Run the migrations: `docker run -it -v $(pwd):/app dwitter make migrate`.
3. Create the admin account: `docker run -it -v $(pwd):/app dwitter python manage.py createsuperuser`.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
3. Create the admin account: `docker run -it -v $(pwd):/app dwitter python manage.py createsuperuser`.
3. Create the admin account: `docker run -it -v $(pwd):/dwitter dwitter python manage.py createsuperuser`.

1. Build the Docker image using the `Dockerfile` at the root directory of this project: `docker build -t dwitter .`.
2. Run the migrations: `docker run -it -v $(pwd):/app dwitter make migrate`.
3. Create the admin account: `docker run -it -v $(pwd):/app dwitter python manage.py createsuperuser`.
4. Run the app in a Docker container: `docker run -it -v $(pwd):/app -p 8000:8000 dwitter`.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
4. Run the app in a Docker container: `docker run -it -v $(pwd):/app -p 8000:8000 dwitter`.
4. Run the app in a Docker container: `docker run -it -v $(pwd):/dwitter -p 8000:8000 dwitter`.

@karllhughes
Copy link
Author

Thanks for the feedback @lionleaf!

I'll go through each comment individually, make edits, and let you know when it's ready for another review 👍

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.

None yet

3 participants