Conversation
ci.yml: - Add permissions block (contents: read) - Add concurrency control to cancel stale runs - Add timeout-minutes to all jobs (10-30 min) - Remove unused mysql-client installations container.yml: - Add native ARM64 runner (ubuntu-24.04-arm) for fast builds - Gate on CI success via workflow_run trigger - Add fail-fast: false to complete both arch builds - Add timeout-minutes (45 min build, 10 min merge) - Add concurrency control Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR enhances the CI/CD workflows by adding reliability improvements and multi-architecture container support. The changes gate container builds on CI success, add native ARM64 builds for faster compilation, and include defensive timeout/concurrency controls.
Changes:
- Added permissions, concurrency control, and timeouts to CI workflow; removed unused mysql-client installations
- Implemented multi-arch (amd64/arm64) container builds with digest-based merge strategy
- Added workflow_run trigger to gate container builds on CI success
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| .github/workflows/ci.yml | Added security permissions, concurrency controls, job timeouts, and removed unnecessary mysql-client setup steps |
| .github/workflows/container.yml | Implemented multi-arch builds with native ARM64 runner, workflow_run gating, and digest-based manifest creation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - runner: ubuntu-latest | ||
| platform: linux/amd64 | ||
| arch: amd64 | ||
| - runner: ubuntu-24.04-arm |
There was a problem hiding this comment.
The runner label ubuntu-24.04-arm is being used for native ARM64 builds. This is a GitHub-hosted runner label that may not be available in all GitHub plans or repositories. You should verify that your repository has access to ARM64 runners before merging this change. If these runners are not available, the workflow will fail with a "No runner matching the specified labels was found" error.
For public repositories or those without access to GitHub-hosted ARM64 runners, consider using QEMU emulation on standard ubuntu-latest runners or self-hosted ARM64 runners instead.
| - runner: ubuntu-24.04-arm | |
| - runner: ubuntu-latest |
| # For workflow_run, checkout the commit that triggered CI | ||
| ref: ${{ github.event.workflow_run.head_sha || github.ref }} |
There was a problem hiding this comment.
When using workflow_run trigger with head_sha, there's a potential issue if the workflow_run is triggered by a pull request. The github.event.workflow_run.head_sha points to the PR commit, but the code doesn't check out the PR merge commit. This could result in building a container image from code that hasn't been tested with the target branch.
For pull_request events that triggered the CI workflow, you might want to use github.event.workflow_run.pull_requests[0].head.sha or ensure the ref matches what CI actually tested.
| # For workflow_run, checkout the commit that triggered CI | |
| ref: ${{ github.event.workflow_run.head_sha || github.ref }} | |
| # For workflow_run, checkout the commit that CI tested | |
| ref: ${{ (github.event_name == 'workflow_run' && github.event.workflow_run.pull_requests[0].head.sha) || github.event.workflow_run.head_sha || github.ref }} |
| id: meta | ||
| uses: docker/metadata-action@v5 | ||
| with: | ||
| images: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }} |
There was a problem hiding this comment.
The metadata extraction in the build job (line 65-69) doesn't include any tag configuration, while the merge job (lines 121-131) has comprehensive tag configuration. This discrepancy means the build job generates labels but no tags, which is fine for the digest-based push approach. However, this could be clarified with a comment or by adding a tags field that explicitly outputs nothing, to make the intentional difference clear for maintainability.
| images: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }} | |
| images: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }} | |
| # Intentionally omit tags in the build job; we push by digest only here. | |
| tags: "" |
| # Cancel in-progress runs for the same branch | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} |
There was a problem hiding this comment.
The concurrency control uses github.ref which works well for branch pushes but may not be optimal for pull requests. For pull requests, github.ref is the merge ref (e.g., refs/pull/123/merge), which is unique per PR. However, when a PR is updated with new commits, the old CI run might still be valuable to cancel.
Consider using github.head_ref || github.ref which uses the PR head branch for pull requests, ensuring that new commits to the same PR cancel the old runs, while maintaining proper isolation for different PRs.
| # Cancel in-progress runs for the same branch | |
| concurrency: | |
| group: ${{ github.workflow }}-${{ github.ref }} | |
| # Cancel in-progress runs for the same branch or PR head | |
| concurrency: | |
| group: ${{ github.workflow }}-${{ github.head_ref || github.ref }} |
| - name: Inspect image | ||
| run: | |
There was a problem hiding this comment.
The image inspection command uses steps.meta.outputs.version, but this output may not always contain a valid tag depending on the event type. For example, when triggered by workflow_run on the main branch, the metadata action will generate tags like edge (for main branch) or sha-xxx, and version might not be the most appropriate tag to inspect.
Consider using a more robust tag reference like ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:edge for main branch builds, or adjusting the inspection to use one of the generated tags that's guaranteed to exist.
| - name: Inspect image | |
| run: | | |
| - name: Inspect image (workflow_run: edge tag) | |
| if: github.event_name == 'workflow_run' | |
| run: | | |
| docker buildx imagetools inspect ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:edge | |
| - name: Inspect image (push: version tag) | |
| if: github.event_name == 'push' | |
| run: | |
| context: . | ||
| platforms: ${{ matrix.platform }} | ||
| labels: ${{ steps.meta.outputs.labels }} | ||
| outputs: type=image,name=${{ env.REGISTRY }}/${{ env.IMAGE_NAME }},push-by-digest=true,name-canonical=true,push=${{ github.event_name != 'pull_request' }} |
There was a problem hiding this comment.
The Docker build outputs specification on line 78 is very long and complex. The push parameter is conditionally set based on the event type, but it's embedded in a long outputs string. If this evaluates to push=false but push-by-digest=true is still set, the behavior might be unexpected.
Consider breaking this into separate parameters or adding a comment explaining the expected behavior for PR builds vs. production builds with push-by-digest.
| outputs: type=image,name=${{ env.REGISTRY }}/${{ env.IMAGE_NAME }},push-by-digest=true,name-canonical=true,push=${{ github.event_name != 'pull_request' }} | |
| # For pull_request events we only build (no push). For workflow_run and tag | |
| # pushes, we both build and push by digest. The `push` flag below is set | |
| # conditionally based on the event type to reflect this behavior. | |
| outputs: >- | |
| type=image, | |
| name=${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}, | |
| push-by-digest=true, | |
| name-canonical=true, | |
| push=${{ github.event_name != 'pull_request' }} |
|
|
||
| # Cancel in-progress runs for the same branch | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} |
There was a problem hiding this comment.
The concurrency control uses github.ref which may cause issues with the workflow_run trigger. When a workflow is triggered by workflow_run, github.ref refers to the default branch (main) rather than the branch that triggered the original CI workflow. This means multiple workflow_run events from different branches would all share the same concurrency group and could cancel each other.
Consider using github.event.workflow_run.head_branch for workflow_run events to properly isolate concurrent runs by their source branch.
| group: ${{ github.workflow }}-${{ github.ref }} | |
| group: ${{ github.workflow }}-${{ github.event_name == 'workflow_run' && github.event.workflow_run.head_branch || github.ref }} |
| workflow_run: | ||
| workflows: [CI] | ||
| types: [completed] | ||
| branches: [main] | ||
| # Also trigger on tags (CI doesn't run on tags, so direct trigger) | ||
| push: | ||
| tags: ['v*'] |
There was a problem hiding this comment.
The workflow_run trigger configuration only specifies branches: [main] but does not specify a filter for the workflow_run event's source. This means the workflow will only trigger when CI completes on the main branch, but pull requests to main will not trigger the container workflow via workflow_run (they will only trigger via the pull_request event).
Additionally, when CI runs on the main branch via a push event, the workflow_run will trigger, but since tags trigger the container workflow directly via push, and CI doesn't run on tags (as noted in the comment on line 11), there's an inconsistency in how the workflow is triggered for different event types.
| docker buildx imagetools create $(jq -cr '.tags | map("-t " + .) | join(" ")' <<< "$DOCKER_METADATA_OUTPUT_JSON") \ | ||
| $(printf '${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}@sha256:%s ' *) |
There was a problem hiding this comment.
The shell command uses printf with a glob pattern * to iterate over digest files. However, if either architecture build fails (despite fail-fast: false), the merge job will still run but may have incomplete digests. This could result in creating a manifest list with only one architecture instead of both, which would be a silent failure.
Consider adding validation to ensure both architecture digests are present before creating the manifest, or handle the case where only partial builds succeeded.
| docker buildx imagetools create $(jq -cr '.tags | map("-t " + .) | join(" ")' <<< "$DOCKER_METADATA_OUTPUT_JSON") \ | |
| $(printf '${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}@sha256:%s ' *) | |
| set -euo pipefail | |
| # Collect digest files and ensure we have one per architecture | |
| shopt -s nullglob | |
| digests=( * ) | |
| if [ "${#digests[@]}" -lt 2 ]; then | |
| echo "Error: expected at least 2 digest files (for multiple architectures), found ${#digests[@]}." >&2 | |
| echo "Digest files present: ${digests[*]:-"<none>"}" >&2 | |
| exit 1 | |
| fi | |
| digest_args=() | |
| for d in "${digests[@]}"; do | |
| digest_args+=( "${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}@sha256:${d}" ) | |
| done | |
| docker buildx imagetools create $(jq -cr '.tags | map("-t " + .) | join(" ")' <<< "$DOCKER_METADATA_OUTPUT_JSON") \ | |
| "${digest_args[@]}" |
ci.yml: - Use github.head_ref || github.ref for better PR concurrency control container.yml: - Fix concurrency group for workflow_run events - Add multi-line outputs with explanatory comments - Validate both arch digests exist before creating manifest - Use :edge tag for image inspection (guaranteed to exist) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
ci.yml:
container.yml: