Skip to content

Optimize Docker builds and E2E #33

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 2, 2025
Merged

Optimize Docker builds and E2E #33

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
62e3875
Docker build improvements
rolodato Sep 30, 2025
6050b14
Use Compose --wait instead of nc'ing to the client port
rolodato Sep 30, 2025
eaddd56
Adjust based on proposal
sanex3339 Oct 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .dockerignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
.idea
node_modules
docker-compose.*
**/.idea
**/node_modules
**/docker-compose.*
.dockerignore
3 changes: 1 addition & 2 deletions .github/workflows/e2e-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ jobs:
- name: Run Sample App in Docker
run: |
cp .env.docker.example .env.docker &&
npm run docker:up -- -d &&
while ! nc -z localhost 4400; do sleep 1; done
npm run docker:up -- --wait

- name: Install Chrome v111
uses: browser-actions/setup-chrome@v1
Expand Down
13 changes: 9 additions & 4 deletions docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
services:
metabase:
build:
context: .
dockerfile: metabase/Dockerfile
image: metabase/metabase-enterprise:v1.55.x
environment:
MB_CONFIG_FILE_PATH: "./app/init-config.yml"
MB_CONFIG_FILE_PATH: /app/config.yml
MB_JETTY_PORT: "${MB_PORT}"
MB_EDITION: "ee"
MB_SITE_URL: "http://localhost:${MB_PORT}/"
Expand All @@ -19,6 +17,8 @@ services:
retries: 10
ports:
- "${MB_PORT}:${MB_PORT}"
volumes:
- ./metabase/config.yml:/app/config.yml

client:
depends_on:
Expand All @@ -35,6 +35,11 @@ services:
environment:
CLIENT_PORT: "${CLIENT_PORT}"
VITE_METABASE_INSTANCE_URL: "http://localhost:${MB_PORT}"
healthcheck:
test: curl --fail -X GET -I "http://localhost:${CLIENT_PORT}/" || exit 1
interval: 2s
timeout: 2s
retries: 5
ports:
- "${CLIENT_PORT}:${CLIENT_PORT}"
volumes:
Expand Down
11 changes: 0 additions & 11 deletions metabase/Dockerfile
View file
Open in desktop

This file was deleted.

0 metabase/init-config.yml → metabase/config.yml
View file
Open in desktop
File renamed without changes.
5 changes: 3 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@
"private": true,
"version": "0.0.0",
"scripts": {
"start": "npm run docker:up",
"start": "npm run docker:up -- --build",
"rm": "npm run docker:down -- --volumes --rmi all --remove-orphans",
"docker:up": "docker compose --env-file .env.docker up",
"docker:down": "docker compose --env-file .env.docker down --rmi all --volumes"
"docker:down": "docker compose --env-file .env.docker down"
Copy link
Collaborator

@sanex3339 sanex3339 Sep 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Author

@rolodato rolodato Sep 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@sanex3339 sanex3339 Sep 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

@sanex3339 sanex3339 Oct 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@sanex3339 sanex3339 Oct 1, 2025

Choose a reason for hiding this comment

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

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

Copy link
Author

@rolodato rolodato Oct 1, 2025

Choose a reason for hiding this comment

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

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!

}
}