-
Notifications
You must be signed in to change notification settings - Fork 196
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
feat: docker monorepo build #1219
Conversation
🦋 Changeset detectedLatest commit: 5f12991 The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
6f17b51
to
e224d5d
Compare
RUN pnpm run -r build | ||
|
||
FROM builder AS store-indexer | ||
WORKDIR /app/packages/store-indexer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this dockerfile is specific to store-indexer, should we put this in packages/store-indexer/bin
? or does that make the rest of the infra complicated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are having one top level docker file that builds the entire monorepo, then takes specific artifacts and put them in different images. This dockerfile actually describes two images right now: a global builder
, and a specific store-indexer
. Over time we will add more of them, one for each TS service that requires a Docker image.
having one per package is actually worse, given we would need to build the entire monorepo for each of them (instead of looping through all targets -- currently only store-indexer
, and building + tagging them individually which will leverage the cache of the build stage).
Dockerfile
Outdated
WORKDIR /app/packages/store-indexer | ||
EXPOSE 3001 | ||
ENV DEBUG=* | ||
ENV NODE_ENV=production |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should now be able to add RPC_HTTP_URL
and RPC_WS_URL
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
additional nit: should add a docker-compose file. Some of my comments you should definitely do is why I'm requesting changes
@@ -0,0 +1,39 @@ | |||
FROM docker.io/library/debian:bullseye-slim as base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Could use the official nodejs bulssey-slim image with the exact version of node specified in engines
Easier to deal with node versioning and deletes some of the apt-gets
# pnpm | ||
RUN npm install pnpm --global | ||
|
||
FROM base AS builder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not a big deal but unnecessary to start a new image here
RUN npm install pnpm --global | ||
|
||
FROM base AS builder | ||
COPY . /app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be needlessly slow with bad caching. Everytime any file changes you will need to rebuild node modules.
What you should do is instead COPY pnpm-lock.yaml
and then run pnpm fetch. This installs node modules with only the pnpm lockfile so if that didn't change it gets cached.
See pnpm fetch documentation about docker for more info https://pnpm.io/cli/fetch
ENV PATH="${PATH}:/usr/local/go/bin" | ||
ENV PATH="${PATH}:/root/.foundry/bin" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see adding foundry to path here
|
||
ENV PATH="${PATH}:/usr/local/go/bin" | ||
ENV PATH="${PATH}:/root/.foundry/bin" | ||
RUN --mount=type=cache,id=pnpm,target=/pnpm/store pnpm install --frozen-lockfile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should use pnpm fetch followed by pnpm install --offline see comment above
RUN --mount=type=cache,id=pnpm,target=/pnpm/store pnpm install --frozen-lockfile | ||
RUN pnpm run -r build | ||
|
||
FROM builder AS store-indexer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice good use of multistage build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually looking at this now you could likely Copy in just the buld artifacts and then run pnpm i --production
to not install dev deps and have a smaller image. This is a nit though if the image is not too large don't need to optimize now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to make it super tiny you would use a smaller image than bullseye slim. You don't need very much to run just a node process usually requires more extra tooling to build an app than to run an app.
As an example I build with bullseye-slim. but then run the application with the much tinier gcr.io/distroless/nodejs18 as example-server-runner
image here https://github.com/roninjin10/stax/blob/main/Dockerfile#L242
I use distroless specifically because it's debian based
Dockerfile
Outdated
EXPOSE 3001 | ||
ENV DEBUG=* | ||
ENV NODE_ENV=production | ||
CMD [ "pnpm", "start:testnet" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might need to set HOST to 0.0.0.0 ymmv
Co-authored-by: Will Cory <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
going to save the other dockerfile optimizations for a later pass once we get this wired into CI
Co-authored-by: Will Cory <[email protected]>
No description provided.