Add Docker image build #17

hbruch · 2024-02-03T17:08:46Z

This PR adds a Dockerfile and an action to build a docker image for the master branches.

derhuerst

There is a typo in entrypoint.sh which doesn't cause it to fail because it doesn't run set -u.

derhuerst · 2024-03-11T12:24:44Z

Dockerfile

@@ -0,0 +1,21 @@
+FROM python:3.11


I suggest using a slim base image, it has the same glibc-based environment, but includes less irrelevant tools out-of-the-box (~160mb vs 1gb).

Suggested change

FROM python:3.11

FROM python:3.11-slim

Also, is there a reason why we don't use Python v3.12, given that the CI runs tests for it?

derhuerst · 2024-03-11T12:24:56Z

Dockerfile

+
+ENV RUN_MIGRATION=false
+
+RUN apt-get install 


What does this command do?

derhuerst · 2024-03-11T12:32:17Z

entrypoint.sh

@@ -0,0 +1,16 @@
+cd web


This script won't abort when commands fail and with unbound (a.k.a. undefined) variables (as with the typo below). However, given that some operating systems' sh shells seem to not support set -o pipefail yes, even though it should be supported everywhere, I suggest using bash instead.

Also, add a shebang, to avoid confusion about which shell to run this file with.

Suggested change

cd web

#!/bin/bash

set -eu -o pipefail

cd web

derhuerst · 2024-03-11T12:32:53Z

entrypoint.sh

+cd web
+
+# Run migration only if explicitly set via ENV
+if [ "$RUN_MIGRATION" != "false" ]; then


If we want to make this case opt-in, why not check for "$RUN_MIGRATION" = "true"?

derhuerst · 2024-03-11T12:33:25Z

entrypoint.sh

+	./manage.py migrate
+fi
+
+if [ "$ASSING_LOCATIONS" != "false" ]; then


Suggested change

if [ "$ASSING_LOCATIONS" != "false" ]; then

if [ "$ASSIGN_LOCATIONS" != "false" ]; then

derhuerst · 2024-03-11T12:33:50Z

entrypoint.sh

+fi
+
+# don't create an admin interface per default
+#./manage.py createsuperuser  


Should we put this behind an opt-in env var as well?

derhuerst · 2024-03-11T12:40:21Z

Dockerfile

+
+RUN apt-get install 
+RUN apt-get update && apt-get install -y \
+  libpq-dev libgdal-dev libproj-dev libgeos-dev \


According to apt, this installs 824mb worth of packages. Even with --no-install-recommends, it is still 641mb.

Are these dependencies build dependencies (in contrast to runtime dependencies)? Then I suggest adding a build step and copying the build output only.

derhuerst · 2024-03-11T13:30:55Z

entrypoint.sh

+#./manage.py createsuperuser  
+
+# start the server
+./manage.py $@


You might want to use exec here.

[…] If we call prog1 -> prog2 -> prog3 -> prog4 etc. and never go back, then make each call an exec. It saves resources (not much, admittedly, unless repeated) and makes shutdown simplier.

StackOverflow explainer about exec's purposes

After entrypoint.sh is run, the container CMD should be executed. IMHO exec would prevent this(?)

I think we have a misunderstanding here. I referred to the exec Bash built-in. How would it prevent running the command specified in CMD?

derhuerst · 2024-03-11T13:31:15Z

Dockerfile

+
+RUN apt-get install 
+RUN apt-get update && apt-get install -y \
+  libpq-dev libgdal-dev libproj-dev libgeos-dev \


Suggested change

libpq-dev libgdal-dev libproj-dev libgeos-dev \

git \

libpq-dev libgdal-dev libproj-dev libgeos-dev \

i.e. * switch to prebuilt gdal image * multistaged docker build * allow superuser creation via flag * use bash for script execution * set -euo pipefial for entrypoint.sh

derhuerst · 2024-09-24T13:46:00Z

Dockerfile

@@ -0,0 +1,34 @@
+FROM ghcr.io/osgeo/gdal:alpine-small-3.9.2 AS builder


This is not a "floating tag", but hard-codes as specific set of libraries, which means that we won't get security patches "automatically" with a rebuild. We should make sure that there is a tool like Mend Renovate (bot) that notifies about updates to this image.

derhuerst · 2024-09-24T13:51:32Z

.github/workflows/build-and-publish.yaml

+        file: Dockerfile
+        push: true
+        tags: |
+          ghcr.io/${{ steps.lower-repo.outputs.lowercase }}:latest,ghcr.io/${{ steps.lower-repo.outputs.lowercase }}:${{ steps.version.outputs.builddate }}


This will create an image named parkendd/parkapi2, where as ParkAPI v3 images are named parkendd/park-api-v3. Do we want to align the v2 naming scheme with the v3 one?

derhuerst · 2024-09-24T13:57:35Z

.github/workflows/build-and-publish.yaml

As the CI is configured right now, the image will not have (meaningful) OCI annotations, except the (deprecated) plain maintainer label in Dockerfile.

I suggest adding helpful OCI annotations like org.opencontainers.image.source (to find the underlying Git repo) or org.opencontainers.image.licenses (to quickly find out ParkAPI2's license(s)), either

via the docker/metadata-action, which will do it automatically (preferred, as they will be much more thorough); or

by hard-coding some of them via Dockerfile LABELs, e.g. like postgis-gtfs-importer does it; or

both of the above methods.

derhuerst · 2024-09-24T14:01:22Z

.github/workflows/build-and-publish.yaml

+
+    - name: lowercase repo name
+      id: lower-repo
+      uses: ASzc/change-string-case-action@v6


It seems like relying on this unpinned "random" third-party dependency is not necessary because there are various built-in utilities to lower-case a string.

Suggested change

uses: ASzc/change-string-case-action@v6

run: |

echo "lowercase=$(echo -n '${{ github.repository }}' | tr '[:upper:]' '[:lower:]')" >> $GITHUB_OUTPUT

derhuerst · 2024-09-24T14:03:15Z

entrypoint.sh

+
+cd web
+if [ "$RUN_MIGRATION" -eq "1" ]; then
+	echo "RUN MIGRATION is TRUE"


[nitpick]
You might want to log this to stderr.

Suggested change

echo "RUN MIGRATION is TRUE"

>&2 echo "RUN MIGRATION is TRUE"

derhuerst · 2024-09-24T14:07:24Z

entrypoint.sh

+#./manage.py createsuperuser  
+
+# start the server
+./manage.py $@


I think we have a misunderstanding here. I referred to the exec Bash built-in. How would it prevent running the command specified in CMD?

derhuerst · 2024-09-24T14:08:53Z

entrypoint.sh

+fi
+
+# start the server
+./manage.py $@


Just using $@ verbatim has unintended consequences! ⚠️ Compare these two:

bash -c 'python3 -c "import sys;print(sys.argv[2:])" -- $@' -- foo 'bar baz' --hello --world # ['foo', 'bar', 'baz', '--hello', '--world'] bash -c 'python3 -c "import sys;print(sys.argv[2:])" -- "$@"' -- foo 'bar baz' --hello --world # ['foo', 'bar baz', '--hello', '--world']

hbruch requested a review from the-infinity February 3, 2024 17:08

derhuerst suggested changes Mar 11, 2024

View reviewed changes

hbruch requested a review from derhuerst September 21, 2024 06:50

hbruch added 5 commits September 22, 2024 13:29

Add Dockerfile and example docker-compose.yml

8484dd4

Add workflow to build image via action

6e6ae67

make migration optional

2e74e87

add optional find_location on startup

12593aa

incorporate review

f908a0a

i.e. * switch to prebuilt gdal image * multistaged docker build * allow superuser creation via flag * use bash for script execution * set -euo pipefial for entrypoint.sh

hbruch force-pushed the docker branch from 3424e26 to f908a0a Compare September 22, 2024 11:30

derhuerst suggested changes Sep 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Docker image build #17

Add Docker image build #17

hbruch commented Feb 3, 2024

derhuerst left a comment

derhuerst Mar 11, 2024

derhuerst Mar 11, 2024

derhuerst Mar 11, 2024

derhuerst Mar 11, 2024

derhuerst Mar 11, 2024

derhuerst Mar 11, 2024

derhuerst Mar 11, 2024

derhuerst Mar 11, 2024

hbruch Sep 21, 2024

derhuerst Sep 24, 2024

derhuerst Mar 11, 2024

derhuerst Sep 24, 2024

derhuerst Sep 24, 2024

derhuerst Sep 24, 2024

derhuerst Sep 24, 2024

derhuerst Sep 24, 2024

derhuerst Sep 24, 2024

derhuerst Sep 24, 2024

-cd web
+#!/bin/bash
+set -eu -o pipefail
+cd web

	if [ "$ASSING_LOCATIONS" != "false" ]; then
	if [ "$ASSIGN_LOCATIONS" != "false" ]; then

	libpq-dev libgdal-dev libproj-dev libgeos-dev \
	git \
	libpq-dev libgdal-dev libproj-dev libgeos-dev \

		@@ -0,0 +1,34 @@
		FROM ghcr.io/osgeo/gdal:alpine-small-3.9.2 AS builder

	uses: ASzc/change-string-case-action@v6
	run: \|
	echo "lowercase=$(echo -n '${{ github.repository }}' \| tr '[:upper:]' '[:lower:]')" >> $GITHUB_OUTPUT

	echo "RUN MIGRATION is TRUE"
	>&2 echo "RUN MIGRATION is TRUE"

Add Docker image build #17

Are you sure you want to change the base?

Add Docker image build #17

Conversation

hbruch commented Feb 3, 2024

derhuerst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment