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

Create prod Dockerimage from alpine base image #222

Closed
wants to merge 1 commit into from

Conversation

alexlaw528
Copy link
Member

@alexlaw528 alexlaw528 commented Oct 30, 2023

Fixes #177

What changes did you make?

  • Added a production Docker image using alpine as base-image.

Why did you make the changes (we will use this info to test)?

  • Current Docker image is meant for development. It utilizes a larger base-image and contains dependencies unused in production.
  • Reduced image size from 1.26 GB to 392.06 MB
Screenshot 2023-10-29 at 7 29 38 PM

Deploy prod environment locally

  • Run following commands from /peopledepot root directory:

  • Build production image as "alpine-pd"
    docker build -t alpine-pd -f ./app/prod/Dockerfile ./app/prod

  • Start up production services
    docker-compose -f docker-compose.prod.yml up

@alexlaw528 alexlaw528 requested a review from fyliu October 30, 2023 02:29
@fyliu
Copy link
Member

fyliu commented Oct 31, 2023

@alexlaw528 I think the "alpine" is usually part of the image tags. Like -t pd:latest-alpine

I'm not sure if there's a best practice for managing dev and prod dockerfiles. I see some people do Dockerfile.prod, or use the same Dockerfile for both with conditionals. I guess putting it in a directory is as good as any. We can think about this later.

We should also do multistage build to maybe avoid part of the 211MB that system dependencies take up. This is just a note for a later issue.

Copy link
Member

@fyliu fyliu left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Please excuse some of the comments that read more like I'm not sure if it needs a change. I think there's some miscommunication in the issue. The issue was intended to convert the development Dockerfile to use alpine and apk. It's okay to do it for production too. I see you made copies of some files and removed the development dependencies. It's just that it's missing elements that I would consider to be needed for production, so I'm not sure if I should ask for them since they're not in the issue.

Or would it be okay if you just make the change to the development setup and then do the same for production? I'm not sure what the best way to do this is.

@@ -0,0 +1,14 @@
black~=22.3.0
Copy link
Member

Choose a reason for hiding this comment

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

I would remove black, drf-spectacular, pydot, and maybe django-linear-migrations from the production build. I don't think there's any use for them outside development.

ports:
- 8000:8000
env_file:
- ./.env.dev
Copy link
Member

Choose a reason for hiding this comment

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

Maybe have a separate .env or .env.prod file for production

- POSTGRES_USER=people_depot
- POSTGRES_PASSWORD=people_depot
- POSTGRES_DB=people_depot_dev
container_name: people_depot_db
Copy link
Member

Choose a reason for hiding this comment

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

Setting the container name might not be a good idea. I'll have to look up the reasoning for this again.

Copy link
Member

@fyliu fyliu Nov 2, 2023

Choose a reason for hiding this comment

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

Oh yeah, the container_name allows you to refer to it by name in a docker command. If you intend to refer to it using docker-compose, like docker-compose run db... then you only need to use the service name. Setting the container name also restricts docker-compose from generating multiple instances of it. I'm not sure if there's an advantage to that or not. So the reasoning for not setting it is really that it's not very useful in the docker-compose context.

@@ -10,6 +10,7 @@ services:
- 8000:8000
env_file:
- ./.env.dev
container_name: people_depot_dev
Copy link
Member

Choose a reason for hiding this comment

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

Same reasoning as above for not setting the container name. We're mainly referring to containers using docker-compose.

Copy link
Member

Choose a reason for hiding this comment

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

This is probably fine as a conversion from apt to apk. It's missing some production things that we can add in other issues. Multi-stage build, --no-compile flag, maybe apk cache cleanup.

@ExperimentsInHonesty
Copy link
Member

@ExperimentsInHonesty ExperimentsInHonesty deleted the 177-alpine-base-image branch May 3, 2024 00:25
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.

Switch to alpine base image in Dockerfile
3 participants