Multistage Docker + Test in Docker #6345
Conversation
icirellik
force-pushed
the
multistage-docker
branch
from
229e1f1
to
c8a4327
Compare
.circleci/config.yml
Outdated
| name: build | ||
| pre-steps: | ||
| - run: | ||
| command: echo 'export BUILD_TARGET="builder"; export DOCKER_BUILDKIT=1; export BUILDKIT_PROGRESS=plain; export COMPOSE_DOCKER_CLI_BUILD=1;' >> $BASH_ENV |
There was a problem hiding this comment.
We don't "need" to do this. It enables buildkit, makes the output greppable, and tells docker compose to use the docker cli. If this works well, it would make sense to add this to the orb.
There was a problem hiding this comment.
Is there a minimum version of Docker that we need to use for buildkit support?
There was a problem hiding this comment.
The version that we are on is the minimum supported version.
| !test.config.js | ||
| !test.mocha.js | ||
| !tsconfig.json | ||
| !yarn.lock |
There was a problem hiding this comment.
This is one of those things that may surprise developers with unexpected CI failures when "everything worked locally." It's acceptable but something to monitor...
There was a problem hiding this comment.
I agree, however as this pattern is implemented in more places it should become second nature to check this when adding new folders. I would expect most new files to end up in src.
electron.Dockerfile
Outdated
| @@ -0,0 +1,21 @@ | |||
| FROM hokusai_force as data | |||
There was a problem hiding this comment.
Is <...>.Dockerfile the accepted naming convention? I feel like I've seen other styles like Dockerfile.dev but am not sure what's preferable.
There was a problem hiding this comment.
Honestly, I prefer Dockerfile.dev. I am at odds with syntax highlighting, it only works when Dockerfile is the suffix.
|
|
||
| mocha \ | ||
| --retries 5 \ | ||
| --require test.config.js \ | ||
| -t 60000 \ | ||
| -t 360000 \ |
There was a problem hiding this comment.
6 minutes seems really long! Is anything taking that long?
There was a problem hiding this comment.
On 2nd thought, this matches my experience locally, where initial requests take 1-2 minutes to get a response. I assumed that was a local problem, or different with "production" assets.
There was a problem hiding this comment.
This timeout seems to be in place to allow the initial startup, its not really a "timeout". I'm sure that I can find a lower setting with a few extra run, I'll see what I can do.
|
|
||
| # wait for it to accept connections | ||
| ./node_modules/.bin/wait-on http://localhost:5000 | ||
| sleep 10 |
There was a problem hiding this comment.
This is a bug, will remove. Nice catch!
.circleci/config.yml
Outdated
| name: build | ||
| pre-steps: | ||
| - run: | ||
| command: echo 'export BUILD_TARGET="builder"; export DOCKER_BUILDKIT=1; export BUILDKIT_PROGRESS=plain; export COMPOSE_DOCKER_CLI_BUILD=1;' >> $BASH_ENV |
There was a problem hiding this comment.
Is there a minimum version of Docker that we need to use for buildkit support?
icirellik
force-pushed
the
multistage-docker
branch
from
c8a4327
to
07dfd5a
Compare
.circleci/config.yml
Outdated
| steps: | ||
| - hokusai/setup-docker | ||
| - run: hokusai registry pull --tag "$CIRCLE_SHA1" | ||
| - run: docker run --rm -e NODE_ENV=test --entrypoint /bin/bash "hokusai_force" /usr/local/bin/yarn test:mocha |
There was a problem hiding this comment.
👏 this is much more consistent, it seems like we could package these into a run step in the orb as a follow up.
There was a problem hiding this comment.
I have a branch over here: artsy/orbs#111
Dockerfile
Outdated
| jest.config.js \ | ||
| package.json \ | ||
| relay.config.js \ | ||
| renovate.json \ |
There was a problem hiding this comment.
Seems unusual that we'd need renovate in the image.
| COPY patches ./patches | ||
| COPY src ./src | ||
| COPY webpack ./webpack | ||
| COPY .env.oss \ |
There was a problem hiding this comment.
Do we access the .env.oss in tests? I wonder if it makes sense to consolidate those requirements in the .env.test files.
There was a problem hiding this comment.
I'm not sure I will test this. I remember adding it explicitly however that may have been before I added .env.test.
Dockerfile
Outdated
| # Release stage. This stage creates the final docker iamge that will be | ||
| # released. It contains only production dependencies and artifacts. | ||
| # | ||
| # TODO: Make the COPY steps more explicit, the iamge still copies in the all raw |
There was a problem hiding this comment.
Thanks for finding this. I was able to remove the TODO completely!
icirellik
force-pushed
the
multistage-docker
branch
2 times, most recently
from
e28119d
to
7a4fc36
Compare
|
Maybe change the PR title to |
icirellik
force-pushed
the
multistage-docker
branch
12 times, most recently
from
24a2d2a
to
d1c8fd0
Compare
icirellik
force-pushed
the
multistage-docker
branch
10 times, most recently
from
7e5cd16
to
2525bce
Compare
This change updates force to use a multistage docker build and leverages the new docker build enginer, buildkit. Some of the benefits are better layer caching, meaning we should see decreases in the average image build time, we now run our tests in the same docker image that they are compiled in, and the production image is significantly smaller (down to 500MiB from 3GiB).
icirellik
force-pushed
the
multistage-docker
branch
from
2525bce
to
55df972
Compare
This change updates force to use a multistage docker build and leverages
the new docker build enginer, buildkit. Some of the benefits are better
layer caching, meaning we should see decreases in the average image
build time, we now run our tests in the same docker image that they are
compiled in, and the production image is significantly smaller (down to
500MiB from 3GiB).