refactor: address review feedback across all files

naorpeled · naorpeled · commit 4b75469bfb4c · 2026-03-23T00:51:41.000+02:00
Dockerfile:
- Remove redundant apt-get install of already-installed build tools
- Add --depth 1 to pgvector git clone
- Purge lsb-release and gnupg in cleanup step

Workflows (shared):
- Add versions.json as single source of truth for version matrix
- Both workflows read from versions.json via load-versions job
- Add fail-fast: false to all matrix strategies
- Add timeout-minutes to all jobs
- Add GHA build cache with per-version per-runner scoping

test.yml:
- Replace sleep 15 with pg_isready health check polling
- Add PostGIS functional test (ST_Point)
- Add version assertions (verify installed matches requested)
- Simplify container cleanup to docker rm -f

publish.yml:
- Add explicit platform field mapped from runner
- Add name= to push-by-digest output for unambiguous registry reference
- Add imagetools inspect verification after manifest creation
diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml
@@ -10,18 +10,42 @@ env:
   IMAGE_NAME: ${{ github.repository_owner }}/${{ github.event.repository.name }}
 
 jobs:
+  load-versions:
+    runs-on: ubuntu-latest
+    outputs:
+      pg_versions: ${{ steps.set-matrix.outputs.pg_versions }}
+      postgis_versions: ${{ steps.set-matrix.outputs.postgis_versions }}
+      pgvector_versions: ${{ steps.set-matrix.outputs.pgvector_versions }}
+      versions: ${{ steps.set-matrix.outputs.versions }}
+    steps:
+      - uses: actions/checkout@v6
+      - id: set-matrix
+        run: |
+          echo "pg_versions=$(jq -c '[.[].pg_version] | unique' versions.json)" >> "$GITHUB_OUTPUT"
+          echo "postgis_versions=$(jq -c '[.[].postgis_version] | unique' versions.json)" >> "$GITHUB_OUTPUT"
+          echo "pgvector_versions=$(jq -c '[.[].pgvector_version] | unique' versions.json)" >> "$GITHUB_OUTPUT"
+          echo "versions=$(jq -c '.' versions.json)" >> "$GITHUB_OUTPUT"
+
   build:
+    needs: load-versions
     runs-on: ${{ matrix.runner }}
+    timeout-minutes: 30
     permissions:
       contents: read
       packages: write
 
     strategy:
+      fail-fast: false
       matrix:
         runner: [ubuntu-latest, ubuntu-24.04-arm]
-        pg_version: ["17.9", "16.13", "15.17", "14.22"]
-        postgis_version: ["3.6.2"]
-        pgvector_version: ["0.8.2"]
+        pg_version: ${{ fromJSON(needs.load-versions.outputs.pg_versions) }}
+        postgis_version: ${{ fromJSON(needs.load-versions.outputs.postgis_versions) }}
+        pgvector_version: ${{ fromJSON(needs.load-versions.outputs.pgvector_versions) }}
+        include:
+          - runner: ubuntu-latest
+            platform: linux/amd64
+          - runner: ubuntu-24.04-arm
+            platform: linux/arm64
 
     steps:
       - name: Checkout repository
@@ -49,14 +73,15 @@ jobs:
         with:
           context: .
           push: true
+          platforms: ${{ matrix.platform }}
           labels: ${{ steps.meta.outputs.labels }}
           build-args: |
             PG_VERSION=${{ matrix.pg_version }}
             POSTGIS_VERSION=${{ matrix.postgis_version }}
             PGVECTOR_VERSION=${{ matrix.pgvector_version }}
           cache-from: type=gha,scope=${{ matrix.pg_version }}-${{ matrix.runner }}
           cache-to: type=gha,mode=max,scope=${{ matrix.pg_version }}-${{ matrix.runner }}
-          outputs: type=image,push-by-digest=true,name-canonical=true,push=true
+          outputs: type=image,name=${{ env.REGISTRY }}/${{ env.IMAGE_NAME }},push-by-digest=true,name-canonical=true,push=true
 
       - name: Export digest
         run: |
@@ -74,30 +99,16 @@ jobs:
 
   merge:
     runs-on: ubuntu-latest
-    needs: build
+    timeout-minutes: 10
+    needs: [load-versions, build]
     permissions:
       contents: read
       packages: write
 
     strategy:
+      fail-fast: false
       matrix:
-        include:
-          - pg_version: "17.9"
-            postgis_version: "3.6.2"
-            pgvector_version: "0.8.2"
-            latest: true
-          - pg_version: "16.13"
-            postgis_version: "3.6.2"
-            pgvector_version: "0.8.2"
-            latest: false
-          - pg_version: "15.17"
-            postgis_version: "3.6.2"
-            pgvector_version: "0.8.2"
-            latest: false
-          - pg_version: "14.22"
-            postgis_version: "3.6.2"
-            pgvector_version: "0.8.2"
-            latest: false
+        include: ${{ fromJSON(needs.load-versions.outputs.versions) }}
 
     steps:
       - name: Download digests
@@ -130,3 +141,7 @@ jobs:
           docker buildx imagetools create \
             $(jq -cr '.tags | map("-t " + .) | join(" ")' <<< "$DOCKER_METADATA_OUTPUT_JSON") \
             $(printf '${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}@sha256:%s ' *)
+
+      - name: Verify multi-arch manifest
+        run: |
+          docker buildx imagetools inspect $(jq -cr '.tags[0]' <<< "$DOCKER_METADATA_OUTPUT_JSON")
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -8,14 +8,31 @@ on:
   workflow_dispatch:
 
 jobs:
+  load-versions:
+    runs-on: ubuntu-latest
+    outputs:
+      pg_versions: ${{ steps.set-matrix.outputs.pg_versions }}
+      postgis_versions: ${{ steps.set-matrix.outputs.postgis_versions }}
+      pgvector_versions: ${{ steps.set-matrix.outputs.pgvector_versions }}
+    steps:
+      - uses: actions/checkout@v6
+      - id: set-matrix
+        run: |
+          echo "pg_versions=$(jq -c '[.[].pg_version] | unique' versions.json)" >> "$GITHUB_OUTPUT"
+          echo "postgis_versions=$(jq -c '[.[].postgis_version] | unique' versions.json)" >> "$GITHUB_OUTPUT"
+          echo "pgvector_versions=$(jq -c '[.[].pgvector_version] | unique' versions.json)" >> "$GITHUB_OUTPUT"
+
   test:
+    needs: load-versions
     runs-on: ${{ matrix.runner }}
+    timeout-minutes: 15
     strategy:
+      fail-fast: false
       matrix:
         runner: [ubuntu-latest, ubuntu-24.04-arm]
-        pg_version: ["17.9", "16.13", "15.17", "14.22"]
-        postgis_version: ["3.6.2"]
-        pgvector_version: ["0.8.2"]
+        pg_version: ${{ fromJSON(needs.load-versions.outputs.pg_versions) }}
+        postgis_version: ${{ fromJSON(needs.load-versions.outputs.postgis_versions) }}
+        pgvector_version: ${{ fromJSON(needs.load-versions.outputs.pgvector_versions) }}
 
     steps:
       - uses: actions/checkout@v6
@@ -33,8 +50,10 @@ jobs:
             PG_VERSION=${{ matrix.pg_version }}
             POSTGIS_VERSION=${{ matrix.postgis_version }}
             PGVECTOR_VERSION=${{ matrix.pgvector_version }}
+          cache-from: type=gha,scope=${{ matrix.pg_version }}-${{ matrix.runner }}
+          cache-to: type=gha,mode=max,scope=${{ matrix.pg_version }}-${{ matrix.runner }}
 
-      - name: Start PostgreSQL container and test extensions
+      - name: Start PostgreSQL container
         run: |
           IMAGE_TAG="postgres-test:pg${{ matrix.pg_version }}"
           echo "Testing image: $IMAGE_TAG (runner: ${{ matrix.runner }})"
@@ -43,26 +62,60 @@ jobs:
             -e POSTGRES_PASSWORD=test \
             -e POSTGRES_USER=test \
             -e POSTGRES_DB=test \
+            --health-cmd="pg_isready -U test -d test" \
+            --health-interval=2s \
+            --health-timeout=5s \
+            --health-retries=30 \
             $IMAGE_TAG \
             postgres -c shared_preload_libraries=vector
 
-          echo "Waiting for PostgreSQL to start..."
-          sleep 15
-
-          echo "PostgreSQL logs:"
-          docker logs test-db
+          echo "Waiting for PostgreSQL to become healthy..."
+          until [ "$(docker inspect -f '{{.State.Health.Status}}' test-db)" = "healthy" ]; do
+            if [ "$(docker inspect -f '{{.State.Status}}' test-db)" != "running" ]; then
+              echo "Container exited unexpectedly!"
+              docker logs test-db
+              exit 1
+            fi
+            sleep 1
+          done
+          echo "PostgreSQL is ready."
 
+      - name: Test PostGIS extension
+        run: |
           echo "Checking PostGIS version..."
           docker exec test-db psql -U test -d test -c "SELECT postgis_full_version();"
 
+          echo "Testing PostGIS spatial functionality..."
+          docker exec test-db psql -U test -d test -c "SELECT ST_AsText(ST_Point(1, 2));"
+
+      - name: Test pgvector extension
+        run: |
           echo "Checking pgvector extension version..."
           docker exec test-db psql -U test -d test -c "CREATE EXTENSION IF NOT EXISTS vector; SELECT extversion FROM pg_extension WHERE extname = 'vector';"
 
-          echo "Attempting to create a table with a vector column and insert data..."
+          echo "Testing vector operations..."
           docker exec test-db psql -U test -d test -c "CREATE TABLE items (id bigserial PRIMARY KEY, embedding vector(3)); INSERT INTO items (embedding) VALUES ('[1,2,3]'), ('[4,5,6]'); SELECT COUNT(*) FROM items;"
 
+      - name: Verify installed versions match requested
+        run: |
+          EXPECTED_PGVECTOR="${{ matrix.pgvector_version }}"
+          ACTUAL_PGVECTOR=$(docker exec test-db psql -U test -d test -tAc \
+            "SELECT extversion FROM pg_extension WHERE extname = 'vector';")
+          if [ "$ACTUAL_PGVECTOR" != "$EXPECTED_PGVECTOR" ]; then
+            echo "FAIL: expected pgvector $EXPECTED_PGVECTOR, got $ACTUAL_PGVECTOR"
+            exit 1
+          fi
+          echo "pgvector version OK: $ACTUAL_PGVECTOR"
+
+          EXPECTED_POSTGIS="${{ matrix.postgis_version }}"
+          ACTUAL_POSTGIS=$(docker exec test-db psql -U test -d test -tAc \
+            "SELECT extversion FROM pg_extension WHERE extname = 'postgis';")
+          if [ "$ACTUAL_POSTGIS" != "$EXPECTED_POSTGIS" ]; then
+            echo "FAIL: expected PostGIS $EXPECTED_POSTGIS, got $ACTUAL_POSTGIS"
+            exit 1
+          fi
+          echo "PostGIS version OK: $ACTUAL_POSTGIS"
+
       - name: Stop and remove container
         if: always()
-        run: |
-          docker stop test-db || true
-          docker rm test-db || true
+        run: docker rm -f test-db || true
diff --git a/Dockerfile b/Dockerfile
@@ -40,10 +40,7 @@ RUN POSTGIS_MAJOR=$(echo "${POSTGIS_VERSION}" | cut -d. -f1) \
     "postgresql-${PG_MAJOR}-postgis-${POSTGIS_MAJOR}-scripts=${POSTGIS_VERSION}+dfsg*"
 
 # Build and install pinned pgvector version from source
-RUN apt-get update \
-    && apt-get install -y --no-install-recommends git make gcc "postgresql-server-dev-${PG_MAJOR}" \
-    && mkdir -p /usr/src/pgvector \
-    && git clone --branch "v${PGVECTOR_VERSION}" https://github.com/pgvector/pgvector.git /usr/src/pgvector \
+RUN git clone --branch "v${PGVECTOR_VERSION}" --depth 1 https://github.com/pgvector/pgvector.git /usr/src/pgvector \
     && cd /usr/src/pgvector \
     && make \
     && make install
@@ -56,6 +53,8 @@ RUN apt-get purge -y --auto-remove \
     gcc \
     "postgresql-server-dev-${PG_MAJOR}" \
     wget \
+    lsb-release \
+    gnupg \
     && apt-get clean \
     && rm -rf /var/lib/apt/lists/* \
     && rm -rf /usr/src/pgvector
diff --git a/versions.json b/versions.json
@@ -0,0 +1,26 @@
+[
+  {
+    "pg_version": "17.9",
+    "postgis_version": "3.6.2",
+    "pgvector_version": "0.8.2",
+    "latest": true
+  },
+  {
+    "pg_version": "16.13",
+    "postgis_version": "3.6.2",
+    "pgvector_version": "0.8.2",
+    "latest": false
+  },
+  {
+    "pg_version": "15.17",
+    "postgis_version": "3.6.2",
+    "pgvector_version": "0.8.2",
+    "latest": false
+  },
+  {
+    "pg_version": "14.22",
+    "postgis_version": "3.6.2",
+    "pgvector_version": "0.8.2",
+    "latest": false
+  }
+]
