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

Add docker file, docker-compose, and env changes #624

Closed
wants to merge 20 commits into from
Closed

Add docker file, docker-compose, and env changes #624

wants to merge 20 commits into from

Conversation

theref
Copy link
Member

@theref theref commented Sep 22, 2023

This is an attempt to simplify the dev process for newcomers. To test, follow the instructions in the README regarding updating package.json and .env, then: docker-compose run --build. First build will take a while, but after that should be very quick.

Example logs can be seen here https://app.warp.dev/block/qnf8YCpPJIuxvSQRwnRJ1i

@github-actions
Copy link

@beaurancourt
Copy link

Hey @theref, thanks for opening this up!

Can you include in the PR description some context (what the change is and why you're making it), testing procedures, and a screenshot of docker running beautifully in the console?

It doesn't have to be too in-depth, but it's nice for our future selves to have the record :D

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

Copy link
Member

@manumonti manumonti left a comment

Choose a reason for hiding this comment

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

I had some troubles running the docker-compose command. Maybe these changes can help to make it more clear.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@github-actions
Copy link

manumonti
manumonti previously approved these changes Oct 11, 2023
Copy link
Member

@manumonti manumonti left a comment

Choose a reason for hiding this comment

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

LGTM 🙌

@derekpierre
Copy link
Member

derekpierre commented Oct 11, 2023

@theref , looks good.

Couple questions:

  1. Does it make sense to tuck the docker-compose.yml and Dockerfile.dev files into a docker folder or some other named folder instead of having them directly in the base directory? If so, may need to update paths used in config files.
  2. Is it helpful to sneak docker-compose run --build or docker-compose run -f ./docker/docker-compose.yml run build (if you move the files from 1. above) into a yarn script command in the package.json, so then the user runs yarn docker-run or something like that? This is meant to be phrased as a question, not a suggestion - I'm less familiar with yarn best practices.

@theref theref marked this pull request as draft October 16, 2023 13:57
@theref
Copy link
Member Author

theref commented Oct 16, 2023

This PR is no longer ready as the build is no longer successful. There seems to have been a lot of changes in upstream packages that are causing issues here. I am attempting to narrow them down. As I make discoveries I will continue to post them here.

EDIT - fixed

@theref theref marked this pull request as ready for review October 17, 2023 11:33
- "3000:3000"
volumes:
- .:/app # Bind mount the current directory to /app in the container
- /app/node_modules # This will prevent node_modules from being overwritten by the local volume
Copy link
Member

Choose a reason for hiding this comment

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

Adding node_modules to .dockerignore instead would also have an additional benefit of a smaller build context. (Even thought we don't COPY it, docker still has to index it before starting a build process).

Comment on lines +8 to +10
environment:
- PYTHON=/usr/bin/python3
- NODE_OPTIONS=--max_old_space_size=3072
Copy link
Member

Choose a reason for hiding this comment

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

We've already set these up in Dockerfile.dev. Any reason why we set these in two different places?


# Set the environment variables
ENV PYTHON=/usr/bin/python3
ENV NODE_OPTIONS=--max_old_space_size=3072
Copy link
Member

Choose a reason for hiding this comment

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

Could we document the motivation for using this Node.js flag?

@@ -0,0 +1,22 @@
# Use the specified image
FROM node:14-buster-slim
Copy link
Member

Choose a reason for hiding this comment

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

Node.js 14 has already reached the end-of-life, and Node.js 16 is going to reach EOL shortly.

I suggest using node:16-buster-slim since it looks from the contents of package.json

"@types/node": "^16.9.1",
Suggested change
FROM node:14-buster-slim
FROM node:16-buster-slim

We can address transition from 16 to 18 (the next LTS version) in another PR.

@piotr-roslaniec
Copy link
Member

You may want to consider using multistage builds to slightly speed up the building process. Not sure if this is helpful here, just dropping a hint:

# The first stage is for installing dependencies
FROM node:14-buster-slim  AS build-stage

WORKDIR /app

RUN apt-get update && apt-get install -y python3 make g++ git openssh-client ca-certificates && \
    git config --global url."https://".insteadOf git:// && \
    rm -rf /var/lib/apt/lists/* && \
    apt-get clean

ENV PYTHON=/usr/bin/python3
ENV NODE_OPTIONS=--max_old_space_size=3072

COPY package*.json yarn.lock ./

RUN yarn install

# Start a new stag
FROM node:14-buster-slim

WORKDIR /app

# Copy needed files from the previous stage
# This works like a cache, effectively
COPY --from=build-stage /app ./

# Expose port 3000
EXPOSE 3000

@theref
Copy link
Member Author

theref commented Dec 20, 2023

replaced by #625

@theref theref closed this Dec 20, 2023
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.

6 participants