Skip to content
Open
Changes from all commits
Commits
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
172 changes: 117 additions & 55 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,11 @@ defaults:
run:
shell: bash

jobs:
env:
DOCKERHUB_REPO: postgis/postgis
Copy link
Member

Choose a reason for hiding this comment

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

rename to:
DOCKERHUB_REPO: postgis/docker-postgis-test

GITHUB_REPO: postgis/docker-postgis
Copy link
Member

Choose a reason for hiding this comment

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

(optional) add env : LATEST_VERSION: 17-3.5

  • to define which build is tagged as latest, and implement the related tagging.


jobs:
make-docker-images:
strategy:
matrix:
Expand Down Expand Up @@ -67,71 +70,130 @@ jobs:
env:
VERSION: ${{ matrix.postgres }}-${{ matrix.postgis }}
VARIANT: ${{ matrix.variant }}
DOCKER_APT_PKG_VER: '5:28.3.3-1~ubuntu.24.04~noble'

steps:
- name: Install/config specific version of Docker packages
- name: Checkout source
uses: actions/checkout@v5

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v3

- name: Build Docker image for ${{ env.VERSION }} ${{ env.VARIANT }}
id: build
uses: docker/build-push-action@v5
with:
context: ${{ env.VERSION }}${{ env.VARIANT == 'alpine' && '/alpine' || ''}}
file: ${{ env.VERSION }}${{ env.VARIANT == 'alpine' && '/alpine' || ''}}/Dockerfile
load: true
push: false # don't push until after testing

- name: Check out official-images repo
uses: actions/checkout@v5
with:
repository: docker-library/official-images
path: official-images
sparse-checkout: |
test

- name: Run official-images test script
run: |
echo "***** Removing any currently installed conflicting packages..."
for pkg in docker.io docker-doc docker-compose docker-compose-v2 podman-docker containerd runc; do sudo apt-get remove $pkg; done
echo "***** Setting up Docker's APT repo..."
sudo apt-get update
sudo apt-get install ca-certificates curl
sudo install -m 0755 -d /etc/apt/keyrings
sudo curl -fsSL https://download.docker.com/linux/ubuntu/gpg -o /etc/apt/keyrings/docker.asc
sudo chmod a+r /etc/apt/keyrings/docker.asc
echo \
"deb [arch=$(dpkg --print-architecture) signed-by=/etc/apt/keyrings/docker.asc] https://download.docker.com/linux/ubuntu \
$(. /etc/os-release && echo "${UBUNTU_CODENAME:-$VERSION_CODENAME}") stable" | \
sudo tee /etc/apt/sources.list.d/docker.list > /dev/null
sudo apt-get update
echo "***** Check available docker-ce versions ."
sudo apt policy docker-ce
echo "***** Installing Docker packages..."
sudo apt-get install docker-ce=${{ env.DOCKER_APT_PKG_VER }} docker-ce-cli=${{ env.DOCKER_APT_PKG_VER }} containerd.io docker-buildx-plugin docker-compose-plugin
echo "***** Verifying initial Docker installation..."
docker run hello-world
echo "***** Displaying Docker information..."
docker info
echo "***** Configuring Docker for containerd image store and builder keepStorage..."
echo "{ \"features\": { \"containerd-snapshotter\": true }}" | sudo tee /etc/docker/daemon.json
echo $'{
"features": {
"containerd-snapshotter": true
},
"builder": {
"gc": {
"defaultKeepStorage": "20GB",
"enabled": true
}
}
}' | sudo tee /etc/docker/daemon.json
sudo systemctl restart docker
docker info -f '{{ .DriverStatus }}'

- name: Load binfmt platforms for QEMU
./official-images/test/run.sh -c ./official-images/test/config.sh -c test/postgis-config.sh ${{ steps.build.outputs.imageid }}
Copy link
Member

Choose a reason for hiding this comment

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

(optional) backup the log ; and in the next step : Implement a CI check to ensure the official test output contains: ( 'postgis-basics' [6/6]...passed )

Copy link
Author

Choose a reason for hiding this comment

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

Not familiar with this output, but the [6/6] bit seems fragile -- I assume that would change with the number of test cases.
In your experience would it be reasonable to regex away the counts and rely on passed?

Copy link
Member

Choose a reason for hiding this comment

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

I think any solution would be good that detects whether the
[postgres-basics, postgres-initdb, postgis-basics] tests actually ran.

These tests are currently defined here:
https://github.com/postgis/docker-postgis/blob/master/test/postgis-config.sh
and the "postgis/postgis" name is hard-coded:

testAlias[postgis/postgis]=postgres
imageTests[postgis/postgis]='
	postgis-basics
'

If this isn't adjusted to match the image name being tested, then these tests won't run.

If I'm seeing correctly, in your earlier test log, only 3 tests ran:

Run ./official-images/test/run.sh -c ./official-images/test/config.sh -c test/postgis-config.sh sha256:90c1d3051135610d96c6578f2c379a039c5a293da0ef66aded00529d1d8b1760
testing sha256:90c1d3051135610d96c6578f2c379a039c5a293da0ef66aded00529d1d8b1760
	'utc' [1/3]...passed
	'no-hard-coded-passwords' [2/3]...passed
	'override-cmd' [3/3]...passed

All 6 tests should run:

/home/runner/official-images/test/run.sh -c /home/runner/official-images/test/config.sh -c test/postgis-config.sh postgis/postgis:17-3.5
testing postgis/postgis:17-3.5
	'utc' [1/6]...passed
	'no-hard-coded-passwords' [2/6]...passed
	'override-cmd' [3/6]...passed
	'postgres-basics' [4/6]...passed
	'postgres-initdb' [5/6]...passed
	'postgis-basics' [6/6]...passed	

Several solutions are possible:

1. Since it's easy to misconfigure, and DOCKERHUB_REPO is freely configurable, we should probably modify postgis-config.sh to provide protection for any fork case. For example, configure ["postgis/postgis", "postgis-test", and if it exists, ${DOCKERHUB_REPO} as well]. However, in this case, the first docker build step would need to tag as "postgis-test" to ensure the test runs. The log check would be just an extra test to detect any future configuration issues.

2. The other solution is much simpler. For example, the first docker build for testing could be tagged with a special name to indicate it's not for publishing, while still running the tests: "postgis/postgis:__local_testing__"

Not familiar with this output, but the [6/6] bit seems fragile -- I assume that would change with the number of test cases.
In your experience would it be reasonable to regex away the counts and rely on passed?

Currently 6 tests run, and as far as I remember, they change very rarely. If any of the 6 tests fails, the process stops with an error.
However, we should verify that both the postgres and postgis tests have run. That's why I thought it would be sufficient to test for the presence of this line with grep:
'postgis-basics' [6/6]...passed
This assumes that the postgres tests have also run successfully.
But I'm open to other suggestions as well.


Of course, ideally we should check for the presence of all 6 tests.
That is, we should verify these 6 lines:

  • 'utc' [1/6]...passed
  • 'no-hard-coded-passwords' [2/6]...passed
  • 'override-cmd' [3/6]...passed
  • 'postgres-basics' [4/6]...passed
  • 'postgres-initdb' [5/6]...passed
  • 'postgis-basics' [6/6]...passed


- name: Login to dockerhub
id: login-dockerhub
uses: docker/login-action@v3
if: ${{ (github.repository == env.GITHUB_REPO) && (github.ref == 'refs/heads/master') && (github.event_name != 'pull_request') }}
with:
username: ${{ secrets.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_ACCESS_TOKEN }}

- name: Push image by digest
id: push
uses: docker/build-push-action@v5 # Build is cached, this is really just a push
if: ${{ (github.repository == env.GITHUB_REPO) && (github.ref == 'refs/heads/master') && (github.event_name != 'pull_request') }}
with:
context: ${{ env.VERSION }}${{ env.VARIANT == 'alpine' && '/alpine' || ''}}
file: ${{ env.VERSION }}${{ env.VARIANT == 'alpine' && '/alpine' || ''}}/Dockerfile
outputs: type=image,"name=${{ env.DOCKERHUB_REPO }}",push-by-digest=true,name-canonical=true,push=true

- name: Export digest
run: |
docker run --privileged --rm tonistiigi/binfmt --install all
docker images --tree
mkdir -p ${{ runner.temp }}/digests
digest="${{ steps.push.outputs.digest }}"
touch "${{ runner.temp }}/digests/${digest#sha256:}"

- name: Checkout source
uses: actions/checkout@v4
- name: Upload digests
uses: actions/upload-artifact@v4
Copy link
Member

Choose a reason for hiding this comment

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

imho: The workflow must never push or publish any artifacts, manifests, or Docker images during a Pull Request test run.

so please add :
if: ${{ (github.repository == env.GITHUB_REPO) && (github.ref == 'refs/heads/master') && (github.event_name != 'pull_request') }}

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, this may be the gotcha I ran into before but forgot about. I'd have to come up to speed again but I remember something about attempting to adhere to the "no registery pushes during PRs" heuristic conflicting with "Need to do a non multiplatform image push to the registry first in order for the multiplatform image manifest creation to be possible" 😬 (but I may be wrong)

Copy link
Author

Choose a reason for hiding this comment

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

Could certainly make PRs behave differently.

Do you care to have both architectures build on PR, or is either fine? If I recall, the current process doesn't yield anything someone can download, so either should be fine unless there's concern that something could break on only one arch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you care to have both architectures build on PR, or is either fine? If I recall, the current process doesn't yield anything someone can download, so either should be fine unless there's concern that something could break on only one arch.

I'm not sure if this answers your question, but ideally we'd want to have a PR trigger CI to build x86 image on an x86 runner and arm64 image on an arm64 runner (so that both builds happen natively from their own perspective) but the architecture of the runner that handles building the multiplatform manifest and potentially pushing the images/manifests to the registry is probably less of a concern.

with:
name: digests-${{ env.VERSION }}-${{ env.VARIANT }}-${{ matrix.runner-platform }}
Copy link
Member

Choose a reason for hiding this comment

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

imho: The workflow must be protected against parallel or consecutive runs causing artifact name collisions.

proposal add a ${{ github.run_id }}

name: digests-${{ github.run_id }}-${{ env.VERSION }}-${{ env.VARIANT }}-${{ matrix.runner-platform }}

Copy link
Author

Choose a reason for hiding this comment

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

Unless I'm missing your point, this should already be handled because the upload is implicitly scoped to the run in progress. Other runs (and even re-executing the same run) are isolated.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the feedback. You're right that actions/upload-artifact@v4 is indeed isolated, and theoretically this isn't strictly necessary.

Of course, if we want to be humorously over-cautious, we could include it with a comment noting that the -${{ github.run_id }} suffix isn't theoretically needed due to v4's isolation, but serves as a placebo to help some maintainers sleep better at night 😄


A related thought:

Since the actions/upload-artifact@v4 overwrite: parameter defaults to false, in very rare cases - if a problem occurs right after upload and someone tries to re-run it - the re-run won't succeed. But I think this is still better than setting it to true.

This exceptional case should be documented, and if such a problem occurs, a completely new job run needs to be manually triggered.

path: ${{ runner.temp }}/digests/*
if-no-files-found: error
retention-days: 1
Copy link
Member

Choose a reason for hiding this comment

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

The retention-days: should be 10 days instead.

The current 1 day makes detailed troubleshooting and debugging very difficult, because as volunteers, we can't always respond to issues within 24 hours.

Additionally, it can sometimes be useful to examine artifacts from the previous week's run, which is why I'd suggest 10 days (as a cautious safety measure).


- name: Build docker image for ${{ env.VERSION }} ${{ env.VARIANT }}
run: make test
merge-manifests:
name: Merge manifests and push to DockerHub
needs: make-docker-images
runs-on: ubuntu-24.04-arm # Always on arm, because why not
if: ${{ (github.repository == env.GITHUB_REPO) && (github.ref == 'refs/heads/master') && (github.event_name != 'pull_request') }}
env:
VERSION: ${{ matrix.postgres }}-${{ matrix.postgis }}
VARIANT: ${{ matrix.variant }}
strategy:
matrix:
# Copy from above, minus the runner-platform
postgres: [13, 14, 15, 16, 17]
postgis: ['3.5']
variant: [default, alpine]
include:
- postgres: 16
postgis: master
variant: default
- postgres: 17
postgis: master
variant: default
- postgres: 17
postgis: '3.6'
variant: alpine
- postgres: 18
postgis: '3.6'
variant: alpine
- postgres: 18
postgis: '3.6'
variant: default

steps:
- name: Login to dockerhub
id: login-dockerhub
uses: docker/login-action@v3
if: ${{ (github.repository == 'postgis/docker-postgis') && (github.ref == 'refs/heads/master') && (github.event_name != 'pull_request') }}
with:
username: ${{ secrets.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_ACCESS_TOKEN }}

- name: Push docker image to dockerhub
# !!!! ONLY push the images when built on ubuntu-24.04 x86 runner for now, NOT for ubuntu-24.04-arm runners
if: ${{ (github.repository == 'postgis/docker-postgis') && (github.ref == 'refs/heads/master') && (github.event_name != 'pull_request') && ( matrix.runner-platform == 'ubuntu-24.04' ) }}
env:
DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }}
DOCKERHUB_ACCESS_TOKEN: ${{ secrets.DOCKERHUB_ACCESS_TOKEN }}
run: make push
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v3

- name: Download digests
uses: actions/download-artifact@v4
with:
path: ${{ runner.temp }}/digests
pattern: digests-${{ env.VERSION }}-${{ env.VARIANT }}-*
merge-multiple: true

- name: Docker Metadata
uses: docker/metadata-action@v5
with:
images: |
${{ env.DOCKERHUB_REPO }}
tags: |
type=raw,value=${{ env.VERSION }}${{ env.VARIANT == 'alpine' && '-alpine' || ''}}

- name: Create manifest list and push
working-directory: ${{ runner.temp }}/digests
run: |
docker buildx imagetools create $(jq -cr '.tags | map("-t " + .) | join(" ")' <<< "$DOCKER_METADATA_OUTPUT_JSON") \
$(printf '${{ env.DOCKERHUB_REPO }}@sha256:%s ' *)

Copy link
Member

Choose a reason for hiding this comment

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

(optional) imho: need an <DOCKERHUB_REPO>:latest tag ; based on the the environment value LATEST_VERSION

- name: Inspect image # Purely for debugging
run: |
sleep 5
docker buildx imagetools inspect ${{ env.DOCKERHUB_REPO }}:${{ env.VERSION }}${{ env.VARIANT == 'alpine' && '-alpine' || ''}}