Skip to content

[wip] Try and set up a PR specific docker image flow #55

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

[wip] Try and set up a PR specific docker image flow #55

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c35532e
Try and set up a PR specific docker image flow
piyush-zlai Oct 30, 2024
a64340a
Tweak as github workflows don't yet support reuse
piyush-zlai Oct 30, 2024
b26812a
More tweaks
piyush-zlai Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion .github/workflows/build_and_push_docker.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ name: Build and Push Docker Image

on:
push:
branches: [ main ]
paths:
- '.github/image/Dockerfile'
pull_request:
Comment on lines +5 to +8
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
branches: [ main ]
paths:
- '.github/image/Dockerfile'
pull_request:
branches: [ main ]
paths:
- '.github/image/**/*'
- '**/Dockerfile'
- '**/docker-compose*.yml'
- '**/docker-compose*.yaml'
pull_request:

paths:
- '.github/image/Dockerfile'
workflow_dispatch:
Expand All @@ -27,9 +31,20 @@ jobs:
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}

# 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
Comment on lines +34 to +43
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Improve shell script robustness and move comments to documentation.

  1. The embedded comments should be moved to documentation or PR description
  2. Shell script variables should be quoted
  3. 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"
+          fi

Alternative 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)


- 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_tag.outputs.tag }}
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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)

40 changes: 32 additions & 8 deletions .github/workflows/test_scala_and_python.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,29 @@ on:
pull_request:
branches: [ main ]

# Note on the container image we're using in the various jobs below:
# 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
jobs:
determine-image:
runs-on: ubuntu-latest
outputs:
container_image: ${{ steps.set-image.outputs.container_image }}
steps:
- id: set-image
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
Comment on lines +19 to +25
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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)


python_tests:
needs: determine-image
runs-on: ubuntu-latest
container:
image: ghcr.io/${{ github.repository }}-ci:latest
image: ${{ needs.determine-image.outputs.container_image }}
credentials:
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
Expand Down Expand Up @@ -50,8 +68,9 @@ jobs:

other_spark_tests:
runs-on: ubuntu-latest
needs: determine-image
container:
image: ghcr.io/${{ github.repository }}-ci:latest
image: ${{ needs.determine-image.outputs.container_image }}
credentials:
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
Expand All @@ -69,8 +88,9 @@ jobs:

join_spark_tests:
runs-on: ubuntu-latest
needs: determine-image
container:
image: ghcr.io/${{ github.repository }}-ci:latest
image: ${{ needs.determine-image.outputs.container_image }}
credentials:
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
Expand All @@ -88,8 +108,9 @@ jobs:

mutation_spark_tests:
runs-on: ubuntu-latest
needs: determine-image
container:
image: ghcr.io/${{ github.repository }}-ci:latest
image: ${{ needs.determine-image.outputs.container_image }}
credentials:
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
Expand All @@ -107,8 +128,9 @@ jobs:

fetcher_spark_tests:
runs-on: ubuntu-latest
needs: determine-image
container:
image: ghcr.io/${{ github.repository }}-ci:latest
image: ${{ needs.determine-image.outputs.container_image }}
credentials:
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
Expand All @@ -124,10 +146,11 @@ jobs:
export SBT_OPTS="-Xmx8G -Xms2G --add-opens=java.base/sun.nio.ch=ALL-UNNAMED"
sbt "spark/testOnly ai.chronon.spark.test.FetcherTest"

scala_compile_fmt_fix :
scala_compile_fmt_fix:
runs-on: ubuntu-latest
needs: determine-image
container:
image: ghcr.io/${{ github.repository }}-ci:latest
image: ${{ needs.determine-image.outputs.container_image }}
credentials:
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
Expand All @@ -150,8 +173,9 @@ jobs:

other_scala_tests:
runs-on: ubuntu-latest
needs: determine-image
container:
image: ghcr.io/${{ github.repository }}-ci:latest
image: ${{ needs.determine-image.outputs.container_image }}
credentials:
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
Expand Down
Loading