Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: docker monorepo build #1219
Changes from 7 commits
dca845a
9830ed6
4190d61
5c72a0b
7370bd5
e224d5d
1f809c6
dd8b449
990096b
02c1caf
4f65b5c
deaaa23
053bba7
e70bd42
c1d0179
6aab25f
5f12991
54e49b5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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-getsThere 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
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
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
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
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 nowThere 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#L242I use distroless specifically because it's debian based
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 specificstore-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).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
andRPC_WS_URL
hereThere 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