Fix CI

[djeebus]

GitHub has some limitations where pull requests that come from forks cannot create packages within the origin repo. This ensures that we can still create the PR images when sent to its own repo, but otherwise the push gets skipped.

[zapier-sre-bot]

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of .github/workflows/on_pull_request.yaml

@@ -32,10 +32,12 @@ jobs:
       - uses: ./.github/actions/build-image
         id: build-image
         with:
+          push: '${{ github.event.pull_request.head.repo.full_name == github.event.pull_request.base.repo.full_name }}'
           image_tag: ${{ env.FS_TAG }}
           token: ${{ secrets.GITHUB_TOKEN }}
 
       - uses: mshick/add-pr-comment@v2
         with:
           message: |
             Temporary image available at `${{ steps.build-image.outputs.image }}`.
+        if: github.event.pull_request.head.repo.full_name == github.event.pull_request.base.repo.full_name

Feedback & Suggestions:

1. Security Consideration: The condition github.event.pull_request.head.repo.full_name == github.event.pull_request.base.repo.full_name is used to determine if the push should occur and if the comment should be added. This is a good practice to ensure actions are only performed on pull requests within the same repository. However, ensure that this logic is thoroughly tested to prevent any unintended behavior, especially if the repository structure changes.

2. Code Readability: Consider adding a comment explaining the purpose of the condition github.event.pull_request.head.repo.full_name == github.event.pull_request.base.repo.full_name. This will help future maintainers understand why this condition is necessary.

3. Consistency: Ensure that the condition is consistently applied across all relevant steps. If there are other steps that should be conditional based on the same logic, make sure they are updated accordingly.

4. Testing: After implementing these changes, test the workflow with both internal and external pull requests to ensure the conditions work as expected.

---

😼 Mergecat review of .github/workflows/on_push_to_main.yaml

@@ -23,6 +23,7 @@ jobs:
 
       - uses: ./.github/actions/build-image
         with:
+          push: 'true'
           image_tag: ${{ env.GIT_TAG }}
           token: ${{ secrets.GITHUB_TOKEN }}
 

Feedback & Suggestions:

1. Security Consideration: Ensure that the push: 'true' parameter is necessary and secure. Allowing automatic pushes can be risky if not properly controlled. Make sure that the action being used is from a trusted source and that it handles authentication securely. 🔒

2. Documentation: It would be helpful to add a comment explaining why push: 'true' is being added. This can help future maintainers understand the purpose of this change. 📝

3. Consistency: Verify that the push parameter is supported by the custom action .github/actions/build-image. If this is a new feature, ensure that the action's documentation is updated accordingly. 📚

---

😼 Mergecat review of .github/actions/build-image/action.yaml

@@ -5,6 +5,10 @@ inputs:
   image_tag:
     description: The image tag
     required: true
+  push:
+    description: True to push image after building, false otherwise
+    required: false
+    default: "false"
   tag_latest:
     description: Tag latest as well as the provided tag
     default: "false"
@@ -21,7 +25,7 @@ runs:
   using: composite
 
   steps:
-    - uses: docker/setup-qemu-action@v1
+    - uses: docker/setup-qemu-action@v3
       with:
         image: tonistiigi/binfmt:latest
         platforms: all
@@ -32,7 +36,7 @@ runs:
       with: { version: "v${{ env.EARTHLY_TOOL_VERSION }}" }
 
     - name: login to registry
-      uses: docker/login-action@v1
+      uses: docker/login-action@v3
       with:
         registry: ghcr.io
         username: ${{ github.repository_owner }}
@@ -42,8 +46,8 @@ runs:
       shell: bash
       run: >-       
         ./earthly.sh 
-        --push 
+        ${{ inputs.push == 'true' && '--push' || '' }} 
         +docker-multiarch
-        ${{ inputs.tag_latest != 'false' && format('--LATEST_IMAGE_NAME=ghcr.io/{0}:latest', github.repository) || '' }} 
+        ${{ inputs.tag_latest != 'false' && format('--LATEST_IMAGE_NAME=ghcr.io/{0}:latest', github.repository) || '' }}
         --GIT_TAG=${{ inputs.image_tag }}
         --IMAGE_NAME=ghcr.io/${{ github.repository }}:${{ inputs.image_tag }}

Feedback & Suggestions:

1. Security & Versioning:

   • The update from docker/setup-qemu-action@v1 to docker/setup-qemu-action@v3 and docker/login-action@v1 to docker/login-action@v3 is a good practice for ensuring the use of the latest features and security patches. However, ensure that these new versions are compatible with your workflow and do not introduce breaking changes. 🛡️

2. Conditional Logic:

   • The use of conditional logic for the --push flag is a good improvement for flexibility. However, consider documenting this behavior clearly in the description field of the push input to avoid confusion for users. 📜

3. Code Readability:

   • The inline conditional logic can be a bit hard to read. Consider adding comments or breaking down complex expressions into separate lines for better readability. For example, you could assign the result of the conditional logic to a variable and then use that variable in the command. 🧹

4. Default Values:

   • Ensure that the default value for push aligns with the expected behavior of your workflow. If the default is "false", make sure this is the intended behavior when the input is not provided. 🔍

---

---

Dependency Review

Click to read mergecats review!

No suggestions found