feat: Add GitHub Actions workflow for GHCR image publishing#207
feat: Add GitHub Actions workflow for GHCR image publishing#207
Conversation
Code Review - PR #207: Add GitHub Actions workflow for GHCR image publishingOverall, this is a well-structured PR that adds automated Docker image publishing to GitHub Container Registry. The implementation is solid with good use of modern GitHub Actions practices. Below are my findings: ✅ Strengths
🐛 Potential IssuesCritical
High Priority
Medium Priority
🔒 Security Considerations
⚡ Performance Considerations
📋 Best Practices & Suggestions
🧪 Test Coverage
📝 Code Quality
SummaryRecommended Actions Before Merge:
Approval Status: Approve with minor changes requested The workflow is well-designed and follows good practices. The critical issue with the kafka-connect Dockerfile paths must be fixed to prevent build failures. The version tagging concern should be addressed to ensure proper image versioning. |
|
Thanks for the review! Fixed the issues:
Re: version tagging - the current |
Code Review: GitHub Container Registry Image PublishingSummaryThis PR adds automated Docker image publishing to GHCR for the server, demo-data, and kafka-connect images. The implementation is well-structured and follows GitHub Actions best practices. ✅ Strengths
🔍 Issues & Recommendations1. Critical: Missing
|
Code Review: GitHub Actions GHCR PublishingSummaryThis PR adds automated Docker image publishing to GitHub Container Registry with smoke tests. The approach is well-structured with proper separation of concerns. Here's my detailed feedback: ✅ Strengths
🔍 Issues & Concerns1. Duplicate Server Build (Performance/Cost)The server is built twice:
Impact: Wastes ~5-10 minutes and GitHub Actions minutes Recommendation: Build once, save as artifact, then push in publish job: - name: Build and save server image
run: |
nix develop --command gradle :server:jibDockerBuild
docker save ghcr.io/typestreamio/server:latest > /tmp/server.tar
- uses: actions/upload-artifact@v4
with:
name: server-image
path: /tmp/server.tarThen in publish job: - uses: actions/download-artifact@v4
- run: docker load < /tmp/server.tar
- run: docker tag ... && docker push ...2. Weak Server Smoke Test (Reliability)Line 53: Issues:
Recommendation: Add health check: # After waiting for "started"
- name: Health check
run: |
# Wait for gRPC server to respond (port 4242 based on server config)
timeout 10 bash -c 'until nc -z localhost 4242; do sleep 1; done'
echo "Server is responding on gRPC port"Or use - name: Install grpcurl
run: |
curl -sSL https://github.com/fullstorydev/grpcurl/releases/download/v1.8.9/grpcurl_1.8.9_linux_x86_64.tar.gz | tar -xz
- name: Health check
run: ./grpcurl -plaintext localhost:4242 list3. Missing Output Reuse (Minor)Line 19: Fix: Either use it in the publish job or remove it: # Remove this if not needed:
outputs:
image-tag: ${{ steps.meta.outputs.version }}4. Inconsistent Tagging Strategy
Recommendation: Be consistent. Either all get SHA tags or none: # In server build.gradle.kts, add SHA tag:
tags = mutableSetOf(
project.version.toString(),
"latest",
System.getenv("GITHUB_SHA")?.take(7) ?: "dev"
)5. Demo-data Smoke Test Could Be Better (Minor)Line 110-111: Using Current: docker run --rm demo-data:test --help || true
timeout 5 docker run --rm demo-data:test coinbase 2>&1 | head -20 || trueBetter approach: # Verify --help exits with 0
if ! docker run --rm demo-data:test --help; then
echo "FAIL: --help failed"
exit 1
fi
# Verify coinbase starts (expect it to fail on Kafka connection, not crash)
if ! timeout 5 docker run --rm demo-data:test coinbase 2>&1 | grep -E "(coinbase|Kafka)"; then
echo "FAIL: No expected output"
exit 1
fi6. Security: Consider Image ScanningNo vulnerability scanning before publishing to registry Recommendation: Add Trivy scanning: - name: Scan server image for vulnerabilities
uses: aquasecurity/trivy-action@master
with:
image-ref: 'ghcr.io/typestreamio/server:latest'
format: 'sarif'
output: 'trivy-results.sarif'
severity: 'CRITICAL,HIGH'
- name: Upload Trivy results to GitHub Security
uses: github/codeql-action/upload-sarif@v3
if: always()
with:
sarif_file: 'trivy-results.sarif'7. Missing Rollback StrategyIf a bad image is published, there's no easy way to revert Recommendation: Document rollback process or add: # In publish job, save previous latest tag
- name: Backup current latest
run: |
docker pull ghcr.io/typestreamio/server:latest || true
docker tag ghcr.io/typestreamio/server:latest ghcr.io/typestreamio/server:latest-backup || true
docker push ghcr.io/typestreamio/server:latest-backup || true📋 Minor Improvements
🔒 Security Review
🧪 Test CoverageGood smoke test coverage, but consider:
📊 Priority FixesP0 (Must fix before merge):
P1 (Should fix): P2 (Nice to have): 🎯 RecommendationApprove with changes. The core workflow is solid, but the duplicate build and weak health check should be addressed before merging. The other issues can be tackled in follow-up PRs if time is tight. Great work on the smoke test approach - that's a pattern more teams should follow! 🚀 |
…ests, consistent tagging
PR Review: GitHub Actions GHCR Publishing WorkflowThanks for this comprehensive PR! The workflow structure is solid with good separation of concerns (build/test → publish). Here's my detailed feedback: ✅ Strengths
🐛 Critical Issues1. Version extraction will fail on manual dispatch (
|
… and race condition
PR Review: GitHub Actions Workflow for GHCR Image PublishingOverall AssessmentThis is a well-structured PR that adds automated Docker image publishing to GHCR. The workflow includes comprehensive smoke tests before publishing, which is excellent. However, there are a few issues that should be addressed. Critical Issues1. Race Condition in Server Smoke Test (.github/workflows/publish-images.yml:46-49)The smoke test checks if the container is running with Recommendation: # Check container health first, then logs
if ! docker inspect server-smoke-test --format='{{.State.Running}}' | grep -q true; then
echo "Server container exited unexpectedly"
docker logs server-smoke-test
exit 1
fi2. Incomplete Demo-Data Smoke Test (.github/workflows/publish-images.yml:114-122)The demo-data smoke test only verifies that the container starts but times out after 5 seconds. This doesn't verify that the connector actually works - it only checks that it fails in an expected way (Kafka connection error). A successful start that immediately exits would also pass this test. Recommendation: # Run in background and check exit code
docker run -d --name demo-test demo-data:test coinbase
sleep 2
# Should still be running (waiting for Kafka)
if ! docker inspect demo-test --format='{{.State.Running}}' | grep -q true; then
docker logs demo-test
exit 1
fi
docker rm -f demo-testCode Quality Issues3. Duplicate Version Logic (.github/workflows/publish-images.yml:193-199, 210-216, 227-233)The version tag extraction logic is duplicated three times. This violates DRY principles and makes maintenance harder. Recommendation: - name: Determine version tags
id: version
run: |
if [[ "$GITHUB_REF_TYPE" == "tag" ]]; then
VERSION="${GITHUB_REF_NAME#v}"
else
VERSION="dev-${GITHUB_SHA::7}"
fi
echo "version=${VERSION}" >> $GITHUB_OUTPUT
echo "sha=${GITHUB_SHA::7}" >> $GITHUB_OUTPUTThen use 4. Hardcoded Image NamesThe workflow uses hardcoded image names like Recommendation: env:
REGISTRY: ghcr.io
IMAGE_PREFIX: ghcr.io/typestreamioSecurity Considerations5. GITHUB_TOKEN Permissions ✅Good: The workflow correctly requests only 6. Image Artifact RetentionThe artifacts are retained for only 1 day, which is appropriate for build artifacts. However, consider whether this is sufficient for debugging failed releases. Performance Considerations7. Parallel Job Execution ✅Excellent: The build-and-test jobs run in parallel, which is efficient. The publish job correctly waits for all tests via 8. Docker Layer Caching ✅Good: Demo-data and kafka-connect builds use GitHub Actions cache ( 9. Image CompressionThe images are compressed with gzip before upload, which is good. However, consider using docker save image:tag | zstd -T0 > /tmp/image.tar.zstBest Practices10. Dockerfile Context Fix ✅The fix to 11. Missing Workflow Timeout ProtectionWhile individual jobs have timeouts (20-30 min), the overall workflow could theoretically run indefinitely if jobs keep getting queued. Recommendation: 12. No Rollback StrategyIf a bad image is published with the
Testing Gaps13. Integration Between ImagesThe workflow tests each image independently but doesn't verify they work together (e.g., kafka-connect + server). Consider adding an integration test job that runs all three containers together. 14. Missing Test: Image SizeConsider adding a check for image size to catch bloat: SIZE=$(docker image inspect image:tag --format='{{.Size}}' | awk '{print $1/1024/1024}')
if (( $(echo "$SIZE > 500" | bc -l) )); then
echo "Warning: Image larger than 500MB"
fiDocumentation15. Missing DocumentationThe PR should document:
Minor Issues16. Inconsistent QuotingSome bash variables use quotes, others don't. Be consistent for safety: docker tag demo-data:test "ghcr.io/typestreamio/demo-data:${VERSION}"17. Error Messages Could Be More HelpfulThe smoke tests print logs on failure, but could benefit from more context: echo "ERROR: Server failed to start within 30 seconds"
echo "Last 50 lines of logs:"
docker logs --tail 50 server-smoke-testSummaryStrengths:
Must Fix:
Should Fix: Nice to Have:
This is solid work overall. The smoke tests are a great addition that will catch issues before they reach production. Once the critical issues are addressed, this will be production-ready. |
1 participant
Summary
ghcr.io/typestreamio/serverv*) or manual dispatchChanges
New workflow:
.github/workflows/publish-images.ymlv*), manual dispatch only (no push to main)build-and-test-server: Builds with Jib, verifies server starts and stays runningbuild-and-test-demo-data: Builds image, verifies container can runbuild-and-test-kafka-connect: Builds image, verifies Debezium connectors installedUpdated:
server/build.gradle.ktstypestream/server(Docker Hub) toghcr.io/typestreamio/server(GHCR)latesttag alongside version tagGITHUB_ACTORandGITHUB_TOKENenv varsTest plan
latest, version, and commit SHACloses typestream-0c2