Skip to content

Improve Dockerfile #455

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

Merged
merged 15 commits into from
Oct 31, 2019
Merged

Improve Dockerfile #455

merged 15 commits into from
Oct 31, 2019

Conversation

pietroalbini
Copy link
Member

@pietroalbini pietroalbini commented Oct 29, 2019

This PR changes the Dockerfile, improving multiple aspects of it. The changes will allow the image to be deployed on our infrastructure.

  • The base image is now Ubuntu 18.04, to be the same as the other infra ones.
  • The image has been shrunk from 3.2GB to 500MB, thanks to moving the index outside of the image and adopting multi-stage builds.
  • Environment variables have been shuffled around, moving docker-compose specific ones either in docker-compose.yml or a new gitignored .env.
  • The entrypoint has been changed to retry migrating the database multiple times, allowing PostgreSQL to initialize during the first startup.
  • Unneeded steps have been removed.

Resulting Dockerfile

If someone also builds docs.rs outside of Docker they need to have a
place to store the prefix and the rustwide workspace. This commit adds
a directory named ignored to both the gitignore and the dockerignore.
The user is not actually used to run the application (is run as root),
but only during parts of the build process. It's not really needed, so
this removes it.
- The CRATESFYI_CONTAINER_NAME variable was removed as it's not needed
  anymore.

- The CRATESFYI_DATABASE_URL variable was moved to docker-compose.yml as
  it's only correct when starting the website with it.

- The CRATESFYI_GITHUB_USERNAME and CRATESFYI_GITHUB_ACCESSTOKEN
  variables were moved to a new, gitignored .env file in the root of the
  repository, to allow people changing them without accidentally
  committing them.

- The DOCS_RS_DOCKER variable was moved to the entrypoint just to remove
  yet another useless layer from the image.
This prevents a stale index from being backed into the image.
Using multi-stage builds got us from 2.8GB to 500MB, which is a great
space saving.
@pietroalbini
Copy link
Member Author

This PR is best reviewed commit by commit.

@pietroalbini
Copy link
Member Author

Also added Docker builds on CI, and the code to upload the image to the infra team's registry on master.

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

This looks great! I have a few nits but I'm very impressed by how much you saved :)

The major point I want to address is the .env file - right now it adds an extra step to getting started with the project. I left some more comments in the file.

@pietroalbini
Copy link
Member Author

Addressed most of the review comments, left the only point I'm not sure how to solve unresolved (doh).

Co-Authored-By: Joshua Nelson <jyn514@gmail.com>
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for asking me to review, I'm honored :)

@pietroalbini
Copy link
Member Author

Thanks for the review! Merging this then.

@pietroalbini pietroalbini merged commit d38fc5c into master Oct 31, 2019
@pietroalbini pietroalbini deleted the improve-dockerfile branch October 31, 2019 15:00
jyn514 added a commit that referenced this pull request Nov 5, 2019
@miller-time ran into this when trying out docker.
When the Dockerfile got split up in #455
`gcc` never got installed in the second container, so there's no linker.
jyn514 added a commit to jyn514/docs.rs that referenced this pull request Nov 6, 2019
miller-time ran into this when trying out docker.
When the Dockerfile got split up in rust-lang#455
`gcc` never got installed in the second container, so there's no linker.
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.

3 participants