Skip to content
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

docker based build #4415

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

docker based build #4415

wants to merge 8 commits into from

Conversation

clement-igonet
Copy link

@clement-igonet clement-igonet commented Jul 17, 2024

Fixes #57

@codecov-commenter
Copy link

codecov-commenter commented Jul 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.58%. Comparing base (4b98e75) to head (d11a438).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4415   +/-   ##
=======================================
  Coverage   87.57%   87.58%           
=======================================
  Files         246      246           
  Lines       33396    33396           
  Branches     2223     2198   -25     
=======================================
+ Hits        29247    29249    +2     
- Misses       3130     3138    +8     
+ Partials     1019     1009   -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@louwers
Copy link
Collaborator

louwers commented Jul 17, 2024

Can this run the browser tests? I had some trouble with that on an M1 Mac.

@@ -0,0 +1,15 @@
services:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does a docker-compose really make sense with only one service?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would reccomnd to add an entrypoint to the docker file by using npm run start and export 9966 port.
Also add some explanation to the CONTRIBUTING.md file on how to use this image.

Copy link
Author

Choose a reason for hiding this comment

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

Does a docker-compose really make sense with only one service?

I already imagine at least 1 or 2 other services (so I start by a first):

  • a headless web browser for testing purposes
  • a web server to mak run a release with some examples

Copy link
Collaborator

Choose a reason for hiding this comment

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

I still don't see a huge value here...
You can simply mount the entire cloned repo instead of a selective number of folders.
You can switch the entry point command to run different scripts from the package.json file.
Bottom line, this can be dramatically simplified.

Dockerfile Outdated

RUN echo "hello world"

RUN apt-get update && apt-get install -y \
Copy link
Collaborator

@louwers louwers Jul 17, 2024

Choose a reason for hiding this comment

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

The .devcontainer should probably use the Dockerfile, otherwise this is duplicated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this docker container is published, so I'm not sure .devcontainer can use this docker.
I would reccoment trying to find a container that already has most of these installed and simply use it instead of installing everything.

Copy link
Author

Choose a reason for hiding this comment

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

I would reccoment trying to find a container that already has most of these installed and simply use it instead of installing everything.

I'm sure to get this specific point. Are you talking about .devcontainer stuff ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

OK, good point.

I did not consider to integrate docker facilities so far away, I just though about not having to change my environment (installing node and dependencies).

So, I do not aim to manage .devcontainer in that specific P.R.

Is it something mandatory here ?

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated
xvfb \
&& rm -rf /var/lib/apt/lists/*

RUN apt-get update && apt-get install -y \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this split into two lines? Why not installing everything at one?

Copy link
Author

Choose a reason for hiding this comment

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

Why is this split into two lines? Why not installing everything at one?

Well, I'm just following documentation from CONTRIBUTING.md : https://github.com/maplibre/maplibre-gl-js/blob/main/CONTRIBUTING.md#linux-and-by-extension-github-codespaces

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably a mistake there too, or done with readability in mind.
We should aim at reducing docker layers.

Copy link
Author

Choose a reason for hiding this comment

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

OK, it will be done today.

@clement-igonet
Copy link
Author

Can this run the browser tests? I had some trouble with that on an M1 Mac.

Good question.
What is the npm command to launch for that ?
I'm ready to have a try on my side, and to look for a fix if needed.

@HarelM
Copy link
Collaborator

HarelM commented Jul 19, 2024

Checkout the CI code, it has most of what the CI is running, which is stuff in most cases you should run locally to make sure the PR checks pass.
npm run test-unit, npm run test-integration, npm run test-render

@clement-igonet
Copy link
Author

clement-igonet commented Jul 19, 2024

npm run test-unit, npm run test-integration, npm run test-render

Only npm run test-unit runs well in docker way for the moment.
I suggest to focus on npm run test-integration and npm run test-render in another step

@HarelM
Copy link
Collaborator

HarelM commented Jul 20, 2024

Not sure what you mean by another step, but if the docker image is not good at the moment, I suggest to fix it before merging it, otherwise it misses the point a bit...
Also the part about devcontainer is worth looking into as part of this PR.

@clement-igonet
Copy link
Author

clement-igonet commented Jul 20, 2024

Not sure what you mean by another step, but if the docker image is not good at the moment, I suggest to fix it before merging it, otherwise it misses the point a bit... Also the part about devcontainer is worth looking into as part of this PR.

I"ll do my best. github CI is not a part I know well. It seems to be github specific, as I though to suggest something more independent, not linked to github CI.

@HarelM
Copy link
Collaborator

HarelM commented Jul 20, 2024

It might be interesting to publish this dev container to ghcr for every push to main so that people won't need to build it locally...

@clement-igonet
Copy link
Author

clement-igonet commented Jul 20, 2024

It might be interesting to publish this dev container to ghcr for every push to main so that people won't need to build it locally...

OK.
It could be this docker tag: ghcr.io/maplibre/maplibre-gl-js:dev
In that case, do we already have a github docker repository for maplibre ?
secrets.GITHUB_TOKEN would be also required


RUN \
# Configure global npm install location, use group to adapt to UID/GID changes
if ! cat /etc/group | grep -e "^npm:" > /dev/null 2>&1; then groupadd -r npm; fi \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use a node docker image as a base image instead of all this complexity?

Copy link
Author

Choose a reason for hiding this comment

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

OK. fixed.

&& chmod g+s ${NPM_GLOBAL} \
&& npm config -g set prefix ${NPM_GLOBAL} \
&& su ${USERNAME} -c "npm config -g set prefix ${NPM_GLOBAL}" \
# Install eslint
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why you would need to install eslint globally, it is installed locally as part of npm ci command.

Copy link
Author

Choose a reason for hiding this comment

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

OK, fixed.

@@ -1,4 +1,5 @@
{
"name": "MapLibre container using node.js",
"build": { "dockerfile": "Dockerfile" },
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would assume that using a docker image would avoid the need for all the below installations.
Also, if we publish an image as part of the commits to main I would expect to be able to use this image for codespaces...?

@@ -6,6 +6,26 @@ on:
workflow_dispatch:

jobs:
push-store-image:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
push-store-image:
push-dev-image:

Copy link
Author

Choose a reason for hiding this comment

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

OK, applied.

working-directory: './devcontainer'
steps:
- name: 'Checkout GitHub Action'
uses: actions/checkout@main
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to use a version, like in other places in the CI.

Copy link
Author

Choose a reason for hiding this comment

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

Better to use a version, like in other places in the CI.

OK. So I used this:

ghcr.io/maplibre/maplibre-gl-js:${{ steps.package-version.outputs.current-version }}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant v4 or so for the checkout action...

libgif-dev \
&& rm -rf /var/lib/apt/lists/*

CMD npm ci && npm run start
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add npm run build-dist here.
npm run start should be the entry point to allow replacing it with other commands.
This will allow for better ergonomics and avoid the need for docker compose.

Copy link
Author

Choose a reason for hiding this comment

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

OK, done

@clement-igonet
Copy link
Author

A clarification point: what is expected for CI ?
What released docker image should do ?

  • Just contains environment to run any node comment for maplibre ?
  • Contains also source code ?
  • Contain only distribution folder content ?

@HarelM
Copy link
Collaborator

HarelM commented Jul 21, 2024

CI should stay the same, besides pushing an image for every commit to main.
Docker image should contain everything to allow running any maplibre script written in package.json (besides docker within a docker I guess).
It should contain the src and dist, and should allow a developer to replace them with their code.

@clement-igonet
Copy link
Author

clement-igonet commented Jul 21, 2024

CI should stay the same, besides pushing an image for every commit to main. Docker image should contain everything to allow running any maplibre script written in package.json (besides docker within a docker I guess). It should contain the src and dist, and should allow a developer to replace them with their code.

I'll work on that way.

However, in term of use cases, here is what I have in mind:

  • I'd like to rely on an environment that is easy to reproduce (in dev environment or production environment)
    • So a Dockerfile without sources is welcome, for dev and CI purposes
  • As a developer, I'd like to make run maplibre from my own code
    • So a Dockerfile without sources is enough, as docker or docker-compose can bind the current local code inside the docker container
  • As a user, I'd like to have a try with any version of maplibre-gl-js library
    • In that way, I do not need docker as I can manage maplibre-gl-js version in html source code.

=> So I would suggest not to include source code inside provided docker image

  • It does not really cover a clearly defined use case
  • It would provide a big docker image for no value

What do you think ?

@HarelM
Copy link
Collaborator

HarelM commented Jul 21, 2024

I don't think it will change the size dramatically, so the argument is weak.
Since you can override the code I don't think it will matter much.
In order to run a debug version (npm run start) you'll need the code, otherwise it won't work.
And I think that for completeness and simplicity it's better to include it.

We can always remove it later on, I suggest to include it, see how it feels and then see if we truly need to optimize it.

@clement-igonet
Copy link
Author

F.Y.I.: For testing purpose, I realize that google-chrome must be installed.

@@ -0,0 +1,38 @@
ARG VARIANT=20-bookworm-slim
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we are using puppeteer for e2e tests I would recommend a base image that already has chrome and node like:
https://pptr.dev/guides/docker
And avoid all these complexity of installing chrome etc.

@HarelM
Copy link
Collaborator

HarelM commented Jul 27, 2024

I would consider starting from the following docker image and see if there are missing stuff that needs to be added instead of bootstrapping an image:
cypress/browsers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggest docker solution to help on setting up build/test/run environment
4 participants