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

Moves build tasks from build.sh to Dockerfile (#2223) #2244

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

Conversation

nthiad
Copy link
Contributor

@nthiad nthiad commented Apr 12, 2023

Here is a first attempt at #2223.

I tried to make this minimal but I have some questions:

  • Should any of these tasks run as a non-root user?
  • Is yarnpkg from apt ok or should it use a more recent non-apt source?
  • Should mix deps.get use --only prod? I took it out to match build.sh but perhaps that is wrong here.

I didn't remove build.sh entirely in this patch since it has other tasks like dockerBuildAndPush but perhaps this could be removed if somebody who knows more about this can comment.

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

To answer your questions:

The task can run as root. Docker makes it a nightmare to try to use another user. Still, a good practice is to try to run the eventual command as non-root... but we don't do it (yet, probably something to consider).

Using system packages is fine 👍

Yes, we'd like to keep mix deps.get --only prod. Looking at mix.exs and package.json it seem that the elixir dependencies with an impact on the frontend in production are installed.

To further improve:

Ideally, we'd like to avoid having yarnpkg and more importantly the node_modules in the resulting image, we only need the built assets, and node_modules grows very big.

Sadly we can't just delete them, because a Docker image is made of all the intermediary layers; it's not a copy of the end image state. Each command in the Dockerfile is a layer, and each command is responsible to cleanup, hence the RUN apt update && apt install && apt clean && rm command for example, solely to avoid artifacts.

This is where multistage images are useful. In this Dockerfile, we take advantage of it to have a single Dockerfile to build a dev image (as requested by target: dev in docker compose), with just the bare system, enough for local development, that we segue into a release image to build the actual image to deploy.

The advantage of multistage is that we can build something, then start again from another image, and copy the bits we want from it in the final image. There is an example in Citta's Dockerfile: we compile using a crystal image, then start the actual release image using a bare alpine system and copy the built binaries.

I think you can intertwine multiple stages?

Sorry for the long description, but you said you're not very familiar with Docker, so I went in verbose mode 🙇

NOTE: now that I think about it, I'm wondering if we need the elixir deps or even the Elixir image at all after we compile? and... apparently we don't but let's save that for later.

Dockerfile Outdated
@@ -4,7 +4,7 @@ FROM elixir:1.10 AS dev
RUN sed -i '/^mozilla\/DST_Root_CA_X3/s/^/!/' /etc/ca-certificates.conf && update-ca-certificates -f

RUN apt -q update && \
apt -q install -y default-mysql-client inotify-tools festival && \
apt -q install -y default-mysql-client inotify-tools festival yarnpkg && \
Copy link
Member

Choose a reason for hiding this comment

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

We're not using yarn in the development image (since we're running a webpack service in the docker-compose.yml). And we definitely not need it in the production one.

Dockerfile Outdated
Comment on lines 29 to 30
RUN yarnpkg install --no-progress
RUN yarnpkg deploy
Copy link
Member

Choose a reason for hiding this comment

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

We should add yet another stage in the file, using the same image we're running for the webpack service right now, and build the UI as we're now doing in the build.sh file - but in this Dockerfile, so we can then COPY --from=webpack those resulting files.

@nthiad
Copy link
Contributor Author

nthiad commented Apr 17, 2023

As an update to this, I have a multi-stage version where elixir builds a binary and the node vm builds a frontend asset bundle separately from the final image, but I was getting crashes and other issues from the erlang binary that I haven't figured out yet. I might also try building the final image including the erlang runtime components to avoid that issue.

@nthiad
Copy link
Contributor Author

nthiad commented Apr 17, 2023

The updated patch uses a separate image for building the frontend assets. I couldn't quite get a release image without the erlang runtime to work but this goes part of the way.

It also looks like I might need to copy the priv/ directory over too? I'm not entirely sure how things are set up and still a bit unsure about how to test the system holistically still.

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

@nthiad Nice, we're almost there!

The only issue is setting MIX_ENV=prod for the dev build. Maybe we could squeeze a deps stage? For example:

dev -> deps -> js
            -> release

So that dev would only have the bare erlang image (where we can install, update or remove deps at will), but allow js and release to have the erlang deps installed once.

And a nitpick: maybe rename js as frontend?

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