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 Dockerfile to match Next.js example closely, use "standalone" output, and with 40% smaller image size. #355

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jckw
Copy link

@jckw jckw commented May 2, 2024

This PR is an overhaul of the small calcom-docker repo that takes advantage of Next.js's new-ish "standalone" output to create smaller image sizes (1.03GB -> 636MB).

It also removes various miscellaneous files in the repo that make using Docker more confusing.

Background: why this PR is needed

I think there is a conversation to be had about what calcom-docker is for - is it for people running production applications, or for local development? A lot has changed in the Cal/Next.js/Docker landscape since most of this code was written.

My assumption is that in 2024, it is primarily for production applications - calcom/cal.com can easily be run locally, and building Docker images is no longer a pain for people on certain system architectures as it was before.

The first thing to note is that the Docker image build step has been failing to build for the last 2+ months and nothing has been done about it. This indicates that having a single standardised Docker image is not crucial for Cal (and I agree with this!).

All of this suggests that moving away from a single standardised Docker image that uses a few tricks to make it possible to use on different domains would be okay, and that having users build their own images is probably better for them and for Cal.

With this in mind, the Dockerfile should optimise a few things:

  • minimise the image size
  • minimise the startup time
  • make it as easy as possible to configure an image any of the NEXT_PUBLIC_ / env vars available
  • standardise the build process so that it is less likely to fail, and when it fails, people can fix it

As it stands, the Docker image has a couple of tricks and complexities, useless lines, etc., that make it hard to work with.

Changes

This PR removes miscellaneous files that make this repo more confusing, and focuses on plain Docker with an improved Dockerfile.

The Dockerfiles is based off of the example that Next.js provides, which uses the output: "standalone" feature in Next.js. Therefore this PR depends on a change to be merged in the calcom/cal.com repo.

There are some large changes to be aware of, which can probably be replaced, but I removed for simplicity / maintainability:

  • no longer running the slow "replace-placeholder" script; all env vars should be passed as --build-args when building the image
  • using CI=1 to skip linting and type checking during building
  • not using npx to run prisma since this is slow on startup
  • removing support for docker-compose, mainly because I haven't tested it with this setup, and this allows us to skip the 'wait-for-it' script

@jckw
Copy link
Author

jckw commented May 9, 2024

Note that if you want to run this now, you have to do a couple of things since the Cal build is broken (unrelated to these changes).

diff --git a/apps/web/next.config.js b/apps/web/next.config.js
index a8a755010..da807fa3b 100644
--- a/apps/web/next.config.js
+++ b/apps/web/next.config.js
@@ -158,6 +158,7 @@ const matcherConfigUserTypeEmbedRoute = {
 
 /** @type {import("next").NextConfig} */
 const nextConfig = {
+  output: "standalone",
   experimental: {
     // externalize server-side node_modules with size > 1mb, to improve dev mode performance/RAM usage
     serverComponentsExternalPackages: ["next-i18next"],
diff --git a/package.json b/package.json
index 5bfff32c1..8903302fb 100644
--- a/package.json
+++ b/package.json
@@ -57,7 +57,7 @@
     "lint:fix": "turbo run lint:fix",
     "lint:report": "turbo run lint:report",
     "lint": "turbo run lint",
-    "postinstall": "husky install && turbo run post-install",
+    "postinstall": "echo 'Not running postinstall because it breaks docker'",
     "pre-commit": "lint-staged",
     "predev": "echo 'Checking env files'",
     "prisma": "yarn workspace @calcom/prisma prisma",

And then also run yarn install - no idea why this last step is needed.

@keithwillcode
Copy link
Contributor

keithwillcode commented May 9, 2024

Appreciate this contribution a lot @jckw.

Couple of notes:

  • I just recently made the minimal changes necessary to get the Docker builds working again.
  • we have another PR in the main cal.com repo that completely overhauls how we work with Docker. We are closely examining that implementation and consideration to move it into the main repo.
  • With this consideration, we are also planning to officially support the Docker builds moving forward, whereas they have not been officially supported by our team up to this point (as noted in the README) and is a big reason why there are delays getting updates here.
  • as part of the review of that PR, we will consider the changes you’ve suggested here.

Comment on lines +3 to +5
**/node_modules
**/.next
Copy link
Member

Choose a reason for hiding this comment

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

Why are we doing **/<folder> here? Wouldn't simply <folder> do the same thing?
Ref

Copy link
Author

Choose a reason for hiding this comment

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

I don't think that's true. Dockerignore works differently to gitignore and is a bit unintuitive because of that:

https://shisho.dev/blog/posts/how-to-use-dockerignore/#:~:text=Syntax%20difference%20between%20.dockerignore%20and%20.gitignore

All paths are relative to the root

Copy link
Member

Choose a reason for hiding this comment

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

I see. Surprising that this isn't mentioned in the official docker docs. Thank you for the reference.

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