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

Move docker images to ubuntu #620

Merged
merged 11 commits into from
Oct 5, 2022
Merged

Conversation

acalcutt
Copy link
Collaborator

@acalcutt acalcutt commented Oct 1, 2022

This moves the docker images from a node debian image to an ubuntu image. This makes it so libjpeg-turbo8 and libicu66 don't have to be sideloaded into debian to get it to work with maplibre-native.

Due to no longer using a node image, I had to install my own node. I decided to use nodesource since it seems to be a recommended way by nodejs.org

The original image ran as a node user, which is recommended for security. I tried to do the same in these images, but I had to make the users and group since they do not exist by default.

@scara
Copy link

scara commented Oct 1, 2022

Hi @acalcutt,
this looks a nice approach and improvement!

Just got a quick look, you could set multiple ENV VARs in one line reducing the number of layers e.g.:

ENV \
    NODE_ENV="production" \
    CHOKIDAR_USEPOLLING=1 \
    CHOKIDAR_INTERVAL=500

Besides I'd remove wget at the end of the build process since it looks like not required at runtime e.g.:

RUN set -ex; \
    wget -qO- https://deb.nodesource.com/setup_16.x | bash; \
    apt-get install -y nodejs; \
    apt remove wget

or, better, something like (the example targets not the light version):

RUN set -ex; \
    export DEBIAN_FRONTEND=noninteractive; \
    apt-get -qq update; \
    apt-get -y --no-install-recommends install \
      ca-certificates \
      wget \
      pkg-config \
      xvfb \
      libglfw3-dev \
      libuv1-dev \
      libjpeg-turbo8 \
      libicu66 \
      unzip \
      libcurl4-openssl-dev; \
    wget -qO- https://deb.nodesource.com/setup_16.x | bash; \
    apt-get install -y nodejs; \
    apt remove wget; \
    apt-get -y --purge autoremove; \
    apt-get clean; \
    rm -rf /var/lib/apt/lists/*;

This will reduce the cacheability at build time but even the image size at runtime - not tested both the impacts yet.

Another quick suggestion: IMHO use apt and apt-get no more; portability is not required here since the script runs at build time in a known environment (now ubuntu:focal) and doesn't require to be exported out there..

HTH,
Matteo

@acalcutt
Copy link
Collaborator Author

acalcutt commented Oct 1, 2022

I tried to add some of your suggestions, does this look better to you? I think it's a bit cleaner

on the apt vs apt-get I decided to keep apt-get because the docker console complains that apt can be unpredictable and shouldn't be used in scripts.

@acalcutt
Copy link
Collaborator Author

acalcutt commented Oct 1, 2022

Also, this is something I don't know that I'm wondering if someone can explain.

In the main 'Dockerfile', is the separation of 'builder' and 'final' still needed, or could it be simpler like the Dockerfile_light

@scara
Copy link

scara commented Oct 1, 2022

Hi @acalcutt,

is the separation of 'builder' and 'final' still needed

I'll look at it spare time permitted: the idea being a multistage build is to provide the exact changes and layers required at runtime.

Right now, except for the required libs at runtime which are more than those required at build time, the first stage has just a call to npm install but no other difference, so I concur that now the multistage build is losing the reason to be there: there is nothing like e.g. npm run build in the builder stage so it looks like useless.

HTH,
Matteo

@acalcutt
Copy link
Collaborator Author

acalcutt commented Oct 3, 2022

After some talking with @mnutt , I think it makes sense to keep the two stage build, since it allows you to have the packages needed to build in the first step and just the ones needed run in the second. seems better now that it is reworked a bit.

I guess there were a few more packages needed when building on arm due to canvas needing to be built there.

@acalcutt acalcutt requested a review from mnutt October 4, 2022 13:27
@acalcutt acalcutt merged commit f8a0ab6 into maptiler:master Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants