[wip] Try and set up a PR specific docker image flow #55
Conversation
WalkthroughThe changes include updates to two GitHub Actions workflow files. The Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
.github/workflows/build_and_push_docker.yaml (2)
Line range hint
44-50: Critical: Build action ignores metadata tags.The build-push-action is using a hardcoded 'latest' tag instead of the tags generated by the metadata action. This breaks the PR-specific image functionality.
Fix this by using the metadata action's tags:
- name: Build and push Docker image uses: docker/build-push-action@v6 with: context: .github/image push: true - tags: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:latest + tags: ${{ steps.docker_meta.outputs.tags }} + labels: ${{ steps.docker_meta.outputs.labels }}Also, add an
idto the metadata step:- name: Extract metadata for Docker + id: docker_meta uses: docker/metadata-action@v5
Line range hint
1-50: Consider additional architectural improvements.Since this is a WIP PR for setting up PR-specific Docker images, consider these architectural improvements:
- Add image cleanup strategy to prevent storage bloat from PR-specific images
- Configure build cache to speed up builds
- Add multi-platform build support if needed
Example improvements:
# Add cleanup workflow name: Cleanup PR Docker Images on: pull_request: types: [closed] jobs: cleanup: runs-on: ubuntu-latest steps: - name: Delete PR-specific image uses: actions/delete-package-versions@v3 with: package-name: ${{ env.IMAGE_NAME }} tag-prefix: 'pr-${{ github.event.pull_request.number }}'For the build workflow:
- name: Build and push Docker image uses: docker/build-push-action@v6 with: context: .github/image push: true tags: ${{ steps.docker_meta.outputs.tags }} + platforms: linux/amd64,linux/arm64 + cache-from: type=gha + cache-to: type=gha,mode=max
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/build_and_push_docker.yaml(2 hunks).github/workflows/test_scala_and_python.yaml(7 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/test_scala_and_python.yaml
19-19: context "env" is not allowed here. available contexts are "github", "inputs", "matrix", "needs", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
60-60: context "env" is not allowed here. available contexts are "github", "inputs", "matrix", "needs", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
79-79: context "env" is not allowed here. available contexts are "github", "inputs", "matrix", "needs", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
98-98: context "env" is not allowed here. available contexts are "github", "inputs", "matrix", "needs", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
117-117: context "env" is not allowed here. available contexts are "github", "inputs", "matrix", "needs", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
136-136: context "env" is not allowed here. available contexts are "github", "inputs", "matrix", "needs", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
160-160: context "env" is not allowed here. available contexts are "github", "inputs", "matrix", "needs", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🔇 Additional comments (1)
.github/workflows/test_scala_and_python.yaml (1)
9-13: Well-structured environment variable implementation!
The CONTAINER_IMAGE environment variable elegantly handles both PR-specific and default cases, supporting the goal of PR-specific docker images. The conditional logic and formatting are clear and maintainable.
| branches: [ main ] | ||
| paths: | ||
| - '.github/image/Dockerfile' | ||
| pull_request: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more inclusive path pattern for Docker-related changes.
The current path filter might miss other Docker-related files that should trigger the workflow. Consider using a pattern like:
paths:
- - '.github/image/Dockerfile'
+ - '.github/image/**/*'
+ - '**/Dockerfile'
+ - '**/docker-compose*.yml'
+ - '**/docker-compose*.yaml'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| branches: [ main ] | |
| paths: | |
| - '.github/image/Dockerfile' | |
| pull_request: | |
| branches: [ main ] | |
| paths: | |
| - '.github/image/**/*' | |
| - '**/Dockerfile' | |
| - '**/docker-compose*.yml' | |
| - '**/docker-compose*.yaml' | |
| pull_request: |
| # Build and push pr specific images when there's changes to Dockerfile as part of a PR | ||
| # Update the 'latest' tag when the change hits master | ||
| - name: Extract metadata for Docker | ||
| uses: docker/metadata-action@v5 | ||
| with: | ||
| images: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }} | ||
| tags: | | ||
| type=ref,event=pr | ||
| type=raw,value=latest,enable=${{ github.ref == 'refs/heads/main' }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance Docker image tagging strategy.
While the PR-specific tagging is good, consider these improvements:
- Add SHA-based tags for immutability
- Add semantic versioning tags for releases
- Move the embedded comments to the PR description or documentation
- name: Extract metadata for Docker
uses: docker/metadata-action@v5
with:
images: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}
tags: |
type=ref,event=pr
type=raw,value=latest,enable=${{ github.ref == 'refs/heads/main' }}
+ type=sha,prefix=
+ type=semver,pattern={{version}}
+ type=semver,pattern={{major}}.{{minor}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Build and push pr specific images when there's changes to Dockerfile as part of a PR | |
| # Update the 'latest' tag when the change hits master | |
| - name: Extract metadata for Docker | |
| uses: docker/metadata-action@v5 | |
| with: | |
| images: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }} | |
| tags: | | |
| type=ref,event=pr | |
| type=raw,value=latest,enable=${{ github.ref == 'refs/heads/main' }} | |
| # Build and push pr specific images when there's changes to Dockerfile as part of a PR | |
| # Update the 'latest' tag when the change hits master | |
| - name: Extract metadata for Docker | |
| uses: docker/metadata-action@v5 | |
| with: | |
| images: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }} | |
| tags: | | |
| type=ref,event=pr | |
| type=raw,value=latest,enable=${{ github.ref == 'refs/heads/main' }} | |
| type=sha,prefix= | |
| type=semver,pattern={{version}} | |
| type=semver,pattern={{major}}.{{minor}} |
| runs-on: ubuntu-latest | ||
| container: | ||
| image: ghcr.io/${{ github.repository }}-ci:latest | ||
| image: ${{ env.CONTAINER_IMAGE }} |
There was a problem hiding this comment.
Fix environment variable context usage in job containers.
The env context is not available in container definitions. This needs to be moved to the job level for proper context availability.
Apply this pattern to each job:
python_tests:
runs-on: ubuntu-latest
+ env:
+ CONTAINER_IMAGE: ${{ github.event_name == 'pull_request' && format('{0}/{1}-ci:pr-{2}', 'ghcr.io', github.repository, github.event.pull_request.number) || format('{0}/{1}-ci:latest', 'ghcr.io', github.repository) }}
container:
- image: ${{ env.CONTAINER_IMAGE }}
+ image: ${{ vars.CONTAINER_IMAGE }}
credentials:
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}Alternatively, consider using a reusable workflow or composite action to avoid duplicating the environment variable definition across jobs.
Also applies to: 60-60, 79-79, 98-98, 117-117, 136-136, 160-160
🧰 Tools
🪛 actionlint
19-19: context "env" is not allowed here. available contexts are "github", "inputs", "matrix", "needs", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
| # Define the container image we'd like to use: | ||
| # If this is in a PR we use the pr specific docker image (as there might be docker file changes) | ||
| # Else we use the 'latest' image | ||
| env: | ||
| CONTAINER_IMAGE: ${{ github.event_name == 'pull_request' && format('{0}/{1}-ci:pr-{2}', 'ghcr.io', github.repository, github.event.pull_request.number) || format('{0}/{1}-ci:latest', 'ghcr.io', github.repository) }} |
There was a problem hiding this comment.
💡 Codebase verification
Integration issue found with Docker image tagging
The Docker build workflow doesn't properly align with the test workflow's image reference pattern. While the test workflow expects PR-specific images with format ghcr.io/{repo}-ci:pr-{number}, the build workflow:
- Uses different tag format via
type=ref,event=pr - Always pushes with
:latesttag instead of PR-specific tags
This mismatch will cause the test workflow to fail when looking for PR-specific images.
Required changes in build_and_push_docker.yaml:
- Update tags configuration to use
pr-${{ github.event.pull_request.number }}format for PRs - Use the tags output from metadata-action in the build-push-action
🔗 Analysis chain
Verify integration with Docker build workflow.
Since this workflow depends on PR-specific Docker images, let's verify the build workflow configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Docker build workflow configuration
# Check if the Docker build workflow exists and contains PR-specific image building
rg -l 'build.*docker' .github/workflows/ | xargs rg 'pr-\${{ github.event.pull_request.number }}'
# Verify the image naming pattern matches between workflows
rg -l 'CONTAINER_IMAGE' .github/workflows/ | xargs rg 'ghcr.io/.*-ci:'
Length of output: 302
Script:
#!/bin/bash
# Let's try a different approach to verify Docker-related workflows
# Find Docker-related workflow files
echo "=== Docker-related workflow files ==="
fd workflow .github/workflows/ --exec grep -l "docker" {} \;
# Check contents of these files for PR-specific image building
echo -e "\n=== Checking PR-specific image building patterns ==="
rg "pr-.*github.*pull_request" .github/workflows/
# Check container image patterns
echo -e "\n=== Container image patterns ==="
rg "CONTAINER_IMAGE|ghcr.io" .github/workflows/
Length of output: 1638
Script:
#!/bin/bash
# Let's check the build_and_push_docker.yaml workflow content
echo "=== Docker build workflow content ==="
cat .github/workflows/build_and_push_docker.yaml
Length of output: 1443
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
.github/workflows/build_and_push_docker.yaml (1)
44-50: Consider enhancing Docker build configuration.The current build configuration could benefit from:
- Enabling layer caching to speed up builds
- Supporting multi-stage builds
- Handling build arguments
Example enhancement:
- name: Build and push Docker image uses: docker/build-push-action@v6 with: context: .github/image push: true tags: ${{ steps.docker_tag.outputs.tag }} + cache-from: type=gha + cache-to: type=gha,mode=max + build-args: | + BUILDKIT_INLINE_CACHE=1🧰 Tools
🪛 yamllint
[error] 50-50: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/test_scala_and_python.yaml (1)
Line range hint
27-182: Consider using reusable workflow to reduce configuration duplication.The container configuration is duplicated across all test jobs. Consider extracting it into a reusable workflow to improve maintainability.
Create a new file
.github/workflows/reusable-container-job.yml:name: Reusable Container Job on: workflow_call: inputs: container_image: required: true type: string working_directory: required: true type: string secrets: GITHUB_TOKEN: required: true jobs: container_job: runs-on: ubuntu-latest container: image: ${{ inputs.container_image }} credentials: username: ${{ github.actor }} password: ${{ secrets.GITHUB_TOKEN }} defaults: run: working-directory: ${{ inputs.working_directory }}Then simplify the test jobs:
python_tests: needs: determine-image uses: ./.github/workflows/reusable-container-job.yml with: container_image: ${{ needs.determine-image.outputs.container_image }} working_directory: ${{ github.workspace }} secrets: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} steps: - uses: actions/checkout@v4 # ... rest of the steps
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/build_and_push_docker.yaml(2 hunks).github/workflows/test_scala_and_python.yaml(7 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/build_and_push_docker.yaml
38-38: shellcheck reported issue in this script: SC2086:info:2:86: Double quote to prevent globbing and word splitting
(shellcheck)
38-38: shellcheck reported issue in this script: SC2086:info:4:50: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/test_scala_and_python.yaml
19-19: shellcheck reported issue in this script: SC2086:info:3:99: Double quote to prevent globbing and word splitting
(shellcheck)
19-19: shellcheck reported issue in this script: SC2086:info:5:63: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
.github/workflows/build_and_push_docker.yaml
[error] 50-50: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (2)
.github/workflows/build_and_push_docker.yaml (1)
5-8: Path pattern improvement already suggested.
A previous review has already suggested improving the path patterns for Docker-related changes.
.github/workflows/test_scala_and_python.yaml (1)
13-25: LGTM! Well-structured job for dynamic image selection.
The job correctly implements the logic for selecting PR-specific or latest Docker images, with clear output variable definition and proper GitHub context usage.
🧰 Tools
🪛 actionlint
19-19: shellcheck reported issue in this script: SC2086:info:3:99: Double quote to prevent globbing and word splitting
(shellcheck)
19-19: shellcheck reported issue in this script: SC2086:info:5:63: Double quote to prevent globbing and word splitting
(shellcheck)
| # Build and push pr specific images when there's changes to Dockerfile as part of a PR | ||
| # Update the 'latest' tag when the change hits master | ||
| - name: Set Docker tag | ||
| id: docker_tag | ||
| run: | | ||
| if [[ "${{ github.event_name }}" == "pull_request" ]]; then | ||
| echo "tag=${REGISTRY}/${IMAGE_NAME}:pr-${{ github.event.pull_request.number }}" >> $GITHUB_OUTPUT | ||
| else | ||
| echo "tag=${REGISTRY}/${IMAGE_NAME}:latest" >> $GITHUB_OUTPUT | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve shell script robustness and move comments to documentation.
- The embedded comments should be moved to documentation or PR description
- Shell script variables should be quoted
- Consider using GitHub Actions expressions instead of shell script
Apply this diff to fix the shell script issues and use Actions expressions:
- # Build and push pr specific images when there's changes to Dockerfile as part of a PR
- # Update the 'latest' tag when the change hits master
- - name: Set Docker tag
- id: docker_tag
- run: |
- if [[ "${{ github.event_name }}" == "pull_request" ]]; then
- echo "tag=${REGISTRY}/${IMAGE_NAME}:pr-${{ github.event.pull_request.number }}" >> $GITHUB_OUTPUT
- else
- echo "tag=${REGISTRY}/${IMAGE_NAME}:latest" >> $GITHUB_OUTPUT
- fi
+ - name: Set Docker tag
+ id: docker_tag
+ run: |
+ if [[ "${{ github.event_name }}" == "pull_request" ]]; then
+ echo "tag=${REGISTRY}/${IMAGE_NAME}:pr-${{ github.event.pull_request.number }}" >> "$GITHUB_OUTPUT"
+ else
+ echo "tag=${REGISTRY}/${IMAGE_NAME}:latest" >> "$GITHUB_OUTPUT"
+ fiAlternative implementation using GitHub Actions expressions:
- name: Set Docker tag
id: docker_tag
run: echo "tag=${{ format('{0}/{1}:{2}', env.REGISTRY, env.IMAGE_NAME,
github.event_name == 'pull_request' && format('pr-{0}', github.event.pull_request.number) || 'latest') }}" >> "$GITHUB_OUTPUT"🧰 Tools
🪛 actionlint
38-38: shellcheck reported issue in this script: SC2086:info:2:86: Double quote to prevent globbing and word splitting
(shellcheck)
38-38: shellcheck reported issue in this script: SC2086:info:4:50: Double quote to prevent globbing and word splitting
(shellcheck)
There was a problem hiding this comment.
Add newline at end of file.
Add a newline character at the end of the file to comply with POSIX standards.
- tags: ${{ steps.docker_tag.outputs.tag }}
+ tags: ${{ steps.docker_tag.outputs.tag }}
+📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tags: ${{ steps.docker_tag.outputs.tag }} | |
| tags: ${{ steps.docker_tag.outputs.tag }} | |
🧰 Tools
🪛 yamllint
[error] 50-50: no new line character at the end of file
(new-line-at-end-of-file)
| run: | | ||
| REPOSITORY="${{ github.repository }}" | ||
| if [[ "${{ github.event_name }}" == "pull_request" ]]; then | ||
| echo "container_image=ghcr.io/${REPOSITORY,,}-ci:pr-${{ github.event.pull_request.number }}" >> $GITHUB_OUTPUT | ||
| else | ||
| echo "container_image=ghcr.io/${REPOSITORY,,}-ci:latest" >> $GITHUB_OUTPUT | ||
| fi |
There was a problem hiding this comment.
Add quotes around variables in shell script.
The shell script should have proper quoting to prevent word splitting and globbing issues.
Apply this diff:
run: |
- REPOSITORY="${{ github.repository }}"
+ REPOSITORY="${{ github.repository }}"
if [[ "${{ github.event_name }}" == "pull_request" ]]; then
- echo "container_image=ghcr.io/${REPOSITORY,,}-ci:pr-${{ github.event.pull_request.number }}" >> $GITHUB_OUTPUT
+ echo "container_image=ghcr.io/${REPOSITORY,,}-ci:pr-${{ github.event.pull_request.number }}" >> "$GITHUB_OUTPUT"
else
- echo "container_image=ghcr.io/${REPOSITORY,,}-ci:latest" >> $GITHUB_OUTPUT
+ echo "container_image=ghcr.io/${REPOSITORY,,}-ci:latest" >> "$GITHUB_OUTPUT"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: | | |
| REPOSITORY="${{ github.repository }}" | |
| if [[ "${{ github.event_name }}" == "pull_request" ]]; then | |
| echo "container_image=ghcr.io/${REPOSITORY,,}-ci:pr-${{ github.event.pull_request.number }}" >> $GITHUB_OUTPUT | |
| else | |
| echo "container_image=ghcr.io/${REPOSITORY,,}-ci:latest" >> $GITHUB_OUTPUT | |
| fi | |
| run: | | |
| REPOSITORY="${{ github.repository }}" | |
| if [[ "${{ github.event_name }}" == "pull_request" ]]; then | |
| echo "container_image=ghcr.io/${REPOSITORY,,}-ci:pr-${{ github.event.pull_request.number }}" >> "$GITHUB_OUTPUT" | |
| else | |
| echo "container_image=ghcr.io/${REPOSITORY,,}-ci:latest" >> "$GITHUB_OUTPUT" | |
| fi |
🧰 Tools
🪛 actionlint
19-19: shellcheck reported issue in this script: SC2086:info:3:99: Double quote to prevent globbing and word splitting
(shellcheck)
19-19: shellcheck reported issue in this script: SC2086:info:5:63: Double quote to prevent globbing and word splitting
(shellcheck)
|
Closing as the docker image update scenario is not frequent and the more reliable way here is publishing a image per commit which slows down our CI / dev process a bunch |
Summary
Checklist
Summary by CodeRabbit
New Features
Bug Fixes