-
Notifications
You must be signed in to change notification settings - Fork 5
ci: Use a matrix for publishing docker images #359
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
Conversation
|
/lgtm review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🦉 lgtm Review
Score: Abandon ❌
🔍 Summary
This PR introduces a matrix strategy to the Docker publishing job, which is a flawed approach. The current implementation will break the multi-platform Docker image, causing only the last-built architecture to be available under the shared tags.
Furthermore, the updated script for deleting old cache entries contains a critical bug. It will incorrectly identify and delete production images instead of old cache layers, and also introduces a race condition between the matrix jobs.
Due to these critical issues, I recommend reverting these changes. The original structure of having a single publishing job that creates a multi-platform manifest is the correct one.
More information
- Id:
773c00b0bbf24ba98d41e87a288f109a - Model:
gemini-2.5-pro - Created at:
2025-10-02T15:07:54.896284+00:00
Usage summary
- Request count:
2 - Request tokens:
11,070 - Response tokens:
9,471 - Total tokens:
20,541
See the 📚 lgtm-ai repository for more information about lgtm.
| permissions: | ||
| contents: read | ||
| packages: write | ||
| strategy: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🦉 🎯 Correctness
Severity: HIGH 🔴
strategy:
matrix:
include:
- platform: linux/amd64
cache-ref: buildcache-amd64
- platform: linux/arm64
cache-ref: buildcache-arm64Adding a matrix strategy to this publishing job is incorrect. It will cause the job to run multiple times, once for each platform. Each run will build and push a single-architecture image, overwriting the tags from the previous runs. For example, the latest tag will end up pointing only to the linux/arm64 image, breaking the multi-platform support.
This job should run only once to build and push a single multi-platform manifest that supports all target architectures.
More information about this comment
- File:
.github/workflows/publish.yml - Line:
119 - Relative line:
4 - With suggestion:
No - Suggestion ready for replacement:
No
| # Extract version IDs that do NOT have any buildcache-* tags (buildcache-amd64, buildcache-arm64, etc.) | ||
| ids_to_delete=$(echo "$versions" | jq -r '.[] | select(.metadata.container.tags | map(test("^buildcache-")) | any | not) | .id') | ||
| # Get all versions (paginate + slurp into one array) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🦉 🎯 Correctness
Severity: HIGH 🔴
# Get all versions (paginate + slurp into one array)
versions=$(gh api "orgs/elementsinteractive/packages/container/twyn/versions" --paginate | jq -s 'add')
# Pattern for the current cache ref (GitHub Actions expression expands before script runs)
cache_ref="${{ matrix.cache-ref }}"
# Extract version IDs that do NOT have the cache_ref tag
ids_to_delete=$(echo "$versions" | jq -r --arg pattern "$cache_ref" '
.[]
| select(.metadata.container.tags | map(test($pattern)) | any | not)
| .id
')This script has a critical flaw and will delete your production Docker images.
The jq filter select(.metadata.container.tags | map(test($pattern)) | any | not) selects all package versions that are not tagged with the current matrix job's cache tag (e.g., buildcache-amd64). This means it will target for deletion:
- The cache images for all other architectures (e.g.,
buildcache-arm64). - The actual versioned images you just published (e.g.,
v1.2.3,latest), as they do not havebuildcache-*tags.
Running this in a matrix also creates a race condition where both jobs will attempt to delete the same images. This step is destructive and needs to be completely re-evaluated.
It's worth noting that the original implementation also appeared to be incorrect, as it would delete all non-cache images. The goal of cache pruning needs to be clarified and implemented safely, likely in a separate job after the publish step is complete.
More information about this comment
- File:
.github/workflows/publish.yml - Line:
157 - Relative line:
35 - With suggestion:
No - Suggestion ready for replacement:
No
No description provided.