Optimize Docker builds and E2E

[rolodato]

In #14, we introduced the yarn start run-command, which is now the documented way to run this sample.

I noticed a few improvement opportunities for the current setup:

Don't build a Metabase image

Building a Metabase image is slow, unnecessary and error-prone. This sample should not create its own Metabase image, and instead only use external ones. If the core monorepo depends on this behavior for testing, it should be accommodated some other way such as a separate Compose file, CLI arguments, etc. There should not be implicit behavior such as changing what gets built if a JAR happens to be in some directory.

I've not investigated yet what the implications of this change are for the core monorepo, but wanted to get some validation on this approach before I do that.

Also, instead of building the config.yml into the image, we now just mount it into the Metabase container.

Client image gets rebuilt always

Running yarn run start would always result in the client image being built, even when no changes were made. This was because the .dockerignore was not targeting the nested node_modules directory, which would always get changed during a build, which caused the next build to not get cached. Fixed by adding glob patterns to the .dockerignore.

Don't purge everything on docker:down

Currently, yarn run docker:down purges everything, including built images and volumes. This is usually unnecessary, makes the next yarn run start much slower, and is confusing if you expect this command to behave similar to docker compose down with no arguments.

I changed yarn run docker:down to just do docker compose down. Removing volumes had no effect anyway, since there are no persistent volumes in this setup. Removing images is unnecessary, since we're rebuilding them on startup if needed. This should have no effect in CI.

Because image build caching now works correctly, we also add --build to yarn run start, so that images are always rebuilt if necessary on startup.

Fix race condition and use healthcheck in E2E tests

The E2E tests only check if the client app accepts TCP connections using a hardcoded port, but nothing about if it's healthy or not, or if other containers are healthy. Instead, I've added a healthcheck to the client container, and use docker compose up --wait to wait until all healthchecks succeed before running the E2E tests.

[sanex3339]

The thing is that all our sample app e2e tests in the core app rely on running the docker:up to run a MB based on a locally built MB uberjar.
The same way, they rely on docker:down to completely kill everything related to the container (container, image)

So changing the behavior of these commands will break the core app e2e.

But we can override docker:up/docker:down commands in core app for each app to run something different

So:

• let's introduce docker:e2e:up and docker:e2e:down commands that behave like before
• the new docker-compose-e2e file for e2e should be added that behaves as before, i.e. it builds an image based on Dockerfile, Dockerfile should be restored. If we can extend the docker-compose.yml in it and just override image with build - would be nice
• this docker-compose-e2e should be used in docker:e2e:up/docker:e2e:down
• docker:e2e:up should be used in CI workflow in this repo (instead of docker:up)
• core-app changes (the usage of docker:e2e:up/docker:e2e:down can be handled by our team (me), but this PR must be merged ONLY after we merge a core-app PR, otherwise we break CI in core app

[rolodato]

That makes sense to me, though I think we can still avoid the build by directly mounting the JAR into the container instead? This should give us a decent speedup.

We could introduce a new docker-compose.e2e.yaml (or whatever) file, like so:

services:
  metabase:
    volumes:
      - ./local-dist/metabase.jar:/app/metabase.jar

Then, introduce a docker:e2e:up command that runs:

docker compose -f docker-compose.yml -f docker-compose.e2e.yml --env-file .env.docker up

It's not strictly the same as before but should still work I think. And we can add the docker:e2e:down command as you suggested.

This has the added benefit of failing early if we're running an E2E test without a custom JAR, which I think should never happen but is technically possible with the current setup.

[sanex3339]

Let me check tomorrow if this approach will work with Shoppy (we want to have a consistent setup for all Sample Apps).

At the first glance - it should but i want to be 100% sure, that there won't be issues with a custom entrypoint (defined in docker-compose-e2e)

[sanex3339]

services:
  metabase:
    volumes:
      - ./local-dist/metabase.jar:/app/metabase.jar

Just this - won't work well if the metabase.jar is missing, and it may be missing. When a container is running, an empty metabase.jar folder is created and it looks weird and annoying.
But we can use a long syntax with create_host_path: false

services:
  metabase:
    volumes:
      - type: bind
        source: ./local-dist/metabase.jar
        target: /app/metabase.jar
        read_only: true
        bind:
          create_host_path: false

It also has cons: ideally if a file is not here - we want just to avoid the creation of a bind volume. With the approach above for a missing file it will throw an error. But after 57 it's fine, because to test SDK locally we anyway MUST copy a locally built metabase.jar because it will contain the SDK hosted bundle code.

Based on Shoppy adjustments, the proposal:

• docker:up, docker:down commands keep them as in your PR currently
• rename your rm into docker:rm
• add docker:e2e:up for e2e on the level of this repo - for this repo and for Nextjs app it should just call docker:up under the hood. (For Shoppy it will run an additional docker-compose.e2e.yml with overrides)
• add docker:local-dist:up command. It should run docker compose -f docker-compose.yml -f docker-compose.local-dist.yml --env-file .env.docker up
• add docker-compose.local-dist.yml file with the content:

services:
  metabase:
    volumes:
      - type: bind
        source: ./local-dist/metabase.jar
        target: /app/metabase.jar
        read_only: true
        bind:
          create_host_path: false

• in e2e-tests.yml let's run npm run docker:e2e:up instead of docker:up

So we will have the docker:e2e:up command for e2e CI workflows of this repo, that currently behaves just as a regular docker:up command
We will have the docker:local-dist:up command that will be used locally AND in e2e on the level of the core app to start containers based on the locally built metabase.jar and Embedding SDK package dist (note, that locally it can be either a local metabase.jar OR a local SDK dist, or both).

Wdyt?

[sanex3339]

Also i created a Shoppy PR with changes mentioned above
metabase/shoppy#161

[rolodato]

That makes sense to me. Most of the complexity (conditionally using a local or not) seems to be caused by using Docker Compose, since it doesn't support logic for mounting volumes. At some point it might be worth moving away from Compose and directly calling the Docker API/CLI, though definitely not in this PR :)

I don't work on these codebases enough to have an opinion on whether the increased complexity is worth it, so I'll defer to you to go ahead and merge this PRs and the others you've created. Thanks for taking this onboard @sanex3339!

[sanex3339]

see comment