-
Notifications
You must be signed in to change notification settings - Fork 10
chore: Tag Images with Version Number #2130 #2175
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
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -24,6 +24,34 @@ jobs: | |||||||||||||||||||||||||||||||||||||||||||||||||
| GIT_BRANCH: ${{ github.ref_name }} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| GIT_COMMIT: ${{ github.sha }} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| S3_ENDPOINT: ${{ vars.S3_ENDPOINT }} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Extract project version | ||||||||||||||||||||||||||||||||||||||||||||||||||
| id: get_version | ||||||||||||||||||||||||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||||||||||||||||||||||||
| VERSION="$(jq -r .version package.json)" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if [ -z "$VERSION" ] || [ "$VERSION" = "null" ]; then | ||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "ERROR: Could not extract version from package.json" >&2 | ||||||||||||||||||||||||||||||||||||||||||||||||||
| exit 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||||||||||||||||||||||
| VERSION_WITHOUT_BUILD="${VERSION%%+*}" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "version=v${VERSION_WITHOUT_BUILD}" >> "$GITHUB_OUTPUT" | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+27
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing The dev workflow's "Extract project version" step opens with
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: .github/workflows/production-build.yml
Line: 27-36
Comment:
**Missing `set -euo pipefail` vs. dev workflow**
The dev workflow's "Extract project version" step opens with `set -euo pipefail`, but the equivalent step in `production-build.yml` does not. Without `pipefail`, a failure in a sub-command that feeds into a variable assignment via `$()` can be silently masked (the assignment itself returns exit 0). Although the explicit null-check guard on lines 31–34 covers the most common failure mode, adding `set -euo pipefail` makes the script consistently defensive and brings parity with the dev workflow.
```suggestion
run: |
set -euo pipefail
VERSION="$(jq -r .version package.json)"
if [ -z "$VERSION" ] || [ "$VERSION" = "null" ]; then
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+30
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No version string sanitization for Docker tag compatibility The dev workflow sanitizes the raw version with Docker tags are restricted to
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: .github/workflows/production-build.yml
Line: 30-36
Comment:
**No version string sanitization for Docker tag compatibility**
The dev workflow sanitizes the raw version with `tr -c 'A-Za-z0-9_.-' '-'` before using it as a Docker tag, guarding against any non-standard characters in `package.json`. The production workflow only strips SemVer build metadata (`%%+*`) but does not apply the same sanitization.
Docker tags are restricted to `[a-zA-Z0-9_.-]`. While valid SemVer strings are already Docker-safe after stripping the `+` build segment, adding the same `tr` sanitization step would be a low-cost consistency improvement that prevents surprises if the version string ever contains unexpected characters.
```suggestion
VERSION_WITHOUT_BUILD="${VERSION%%+*}"
SAFE_VERSION="$(printf '%s' "$VERSION_WITHOUT_BUILD" | tr -c 'A-Za-z0-9_.-' '-')"
echo "version=v${SAFE_VERSION}" >> "$GITHUB_OUTPUT"
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||||||||||||||||
| - working-directory: . | ||||||||||||||||||||||||||||||||||||||||||||||||||
| name: push the docker image | ||||||||||||||||||||||||||||||||||||||||||||||||||
| run: docker compose -f docker-compose.prod.yml push api-server frontend queue-consumer documentation artifact-uploader | ||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Tag and push versioned images | ||||||||||||||||||||||||||||||||||||||||||||||||||
| env: | ||||||||||||||||||||||||||||||||||||||||||||||||||
| VERSION: ${{ steps.get_version.outputs.version }} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if docker manifest inspect rslethz/kleinkram:api-server-${VERSION} > /dev/null 2>&1; then | ||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "ERROR: Version tag ${VERSION} already exists in registry." >&2 | ||||||||||||||||||||||||||||||||||||||||||||||||||
| exit 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||||||||||||||||||||||
| docker tag rslethz/kleinkram:api-server-latest rslethz/kleinkram:api-server-${VERSION} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| docker push rslethz/kleinkram:api-server-${VERSION} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| docker tag rslethz/kleinkram:frontend-latest rslethz/kleinkram:frontend-${VERSION} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| docker push rslethz/kleinkram:frontend-${VERSION} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| docker tag rslethz/kleinkram:queue-consumer-latest rslethz/kleinkram:queue-consumer-${VERSION} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| docker push rslethz/kleinkram:queue-consumer-${VERSION} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| docker tag rslethz/kleinkram:docs-latest rslethz/kleinkram:docs-${VERSION} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| docker push rslethz/kleinkram:docs-${VERSION} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| docker tag rslethz/kleinkram:artifact-uploader-latest rslethz/kleinkram:artifact-uploader-${VERSION} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| docker push rslethz/kleinkram:artifact-uploader-${VERSION} | ||||||||||||||||||||||||||||||||||||||||||||||||||
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.
Partial overwrite protection — only
api-serveris guardedThe existence check only inspects
rslethz/kleinkram:api-server-${VERSION}. The remaining four images (frontend,queue-consumer,docs,artifact-uploader) are tagged and pushed without any duplicate check.Two concrete failure scenarios arise:
api-serverandfrontendbut then failed, re-running will correctly error on theapi-servercheck, making the run unrecoverable without bumping the version. No image can be retried.api-server-${VERSION}does not yet exist butfrontend-${VERSION}does (e.g. from a manual push), the check passes and thefrontendimage is silently overwritten.Consider extending the guard to all five services:
The same issue exists in
production-build.ymllines 44–57.Prompt To Fix With AI