Skip to content

Conversation

@mihir20
Copy link

@mihir20 mihir20 commented Dec 23, 2025

What are the changes introduced in this PR?

Write a brief explainer on your code changes.

What is the related Linear task?

Resolves INT-XXX

Please explain the objectives of your changes below

Put down any required details on the broader aspect of your changes. If there are any dependent changes, mandatorily mention them here

Any changes to existing capabilities/behaviour, mention the reason & what are the changes ?

N/A

Any new dependencies introduced with this change?

N/A

Any new generic utility introduced or modified. Please explain the changes.

N/A

Any technical or performance related pointers to consider with the change?

N/A

@coderabbitai review


Developer checklist

  • My code follows the style guidelines of this project

  • No breaking changes are being introduced.

  • All related docs linked with the PR?

  • All changes manually tested?

  • Any documentation changes needed with this change?

  • Is the PR limited to 10 file changes?

  • Is the PR limited to one linear task?

  • Are relevant unit and component test-cases added in new readability format?

Reviewer checklist

  • Is the type of change in the PR title appropriate as per the changes?

  • Verified that there are no credentials or confidential data exposed with the changes.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 23, 2025

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)
Validation error: Unrecognized key(s) in object: 'auto_resolve_threads'
⚙️ Configuration instructions
  • Please see the configuration documentation for more information.
  • You can also validate your configuration using the online YAML validator.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Summary by CodeRabbit

  • Chores
    • CI now generates PR-number-based image tags for pull-request builds.
    • New CI jobs build and push Transformer and User Transformer images to ECR with multi-architecture support, integrated into dev/staging/prod deployment pipelines.

✏️ Tip: You can customize this high-level summary in your review settings.

Walkthrough

Adds a new GitHub Actions workflow to build and push multi-architecture Docker images to ECR and replaces PR tag derivation from branch-based names to PR-number-based names. Integrates the new ECR workflow across dev, staging, and prod deployment pipelines by adding ECR build jobs and updating job dependencies.

Changes

Cohort / File(s) Change Summary
Tag generation & PR artifacts
\.github/workflows/build-pr-artifacts.yml``
Tag derivation changed from branch-based to PR-number-based (pr-${{ github.event.pull_request.number }}); added PR-scoped ECR build jobs build-transformer-image-ecr and build-user-transformer-image-ecr that emit tag outputs.
New ECR build workflow
\.github/workflows/build-push-docker-image-ecr.yaml``
New workflow implementing multi-arch (amd64/arm64) builds, conditional test execution, check_actor gating (dependabot), AWS/ECR auth, buildx-based build/push, and manifest creation with build_type-driven tagging.
Dev pipeline integration
\.github/workflows/prepare-for-dev-deploy.yml``
Added build-transformer-image-ecr and build-user-transformer-image-ecr jobs calling the ECR workflow; pass tag outputs and dockerfile inputs; preserve existing gating and merge-SHA handling.
Prod DT pipeline integration
\.github/workflows/prepare-for-prod-dt-deploy.yml``
Added build-transformer-image-ecr job (uses ECR workflow) and updated create-pull-request to depend on the new ECR build job.
Prod UT pipeline integration
\.github/workflows/prepare-for-prod-ut-deploy.yml``
Added build-user-transformer-image-ecr job (uses ECR workflow) and extended create-pull-request needs to include the new ECR job.
Staging pipeline integration
\.github/workflows/prepare-for-staging-deploy.yml``
Added both build-transformer-image-ecr and build-user-transformer-image-ecr jobs and expanded create-pull-request needs to wait for the ECR builds.

Sequence Diagram

sequenceDiagram
    autonumber
    participant GH as GitHub Actions
    participant Check as check_actor
    participant SHA as get_sha
    participant Files as get_changed_files
    participant BuildA as build-transformer-image-arm64
    participant BuildB as build-transformer-image-amd64
    participant Tests as Conditional Tests
    participant ECR as Amazon ECR
    participant Manifest as create-manifest

    GH->>Check: start -> determine is_dependabot
    Check-->>GH: is_dependabot

    GH->>SHA: compute target sha
    SHA-->>GH: sha

    GH->>Files: inspect changed files -> should_execute_tests
    Files-->>GH: should_execute_tests

    par ARM64 path
        GH->>BuildA: build & optionally load
        alt should_execute_tests == true
            BuildA->>Tests: run tests
            Tests-->>BuildA: results
        end
        BuildA->>ECR: push arm64 image (if allowed)
    and AMD64 path
        GH->>BuildB: build & optionally load
        alt should_execute_tests == true
            BuildB->>Tests: run tests
            Tests-->>BuildB: results
        end
        BuildB->>ECR: push amd64 image (if allowed)
    end

    Note over Check,ECR: skip AWS auth/push for dependabot

    GH->>Manifest: create multi-arch manifest & tag variants
    Manifest->>ECR: register manifest + tags
    Manifest-->>GH: manifest created
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • sivashanmukh
  • ItsSudip
  • saikumarrs
  • krishna2020
  • maheshkutty

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description follows the template structure but contains only placeholder content without substantive implementation details, objectives, or explanations of the changes. Replace placeholder text with actual details: explain what ECR build steps were added and why, specify the Linear task ID, describe objectives, and note any dependent changes or workflow impacts.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: adding ECR build steps for transformer and user transformer images across multiple CI workflows.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch mihir/pipe-2632

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Dec 23, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.25%. Comparing base (9a0036e) to head (759b3c8).
⚠️ Report is 8 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4878   +/-   ##
========================================
  Coverage    92.25%   92.25%           
========================================
  Files          654      654           
  Lines        35358    35384   +26     
  Branches      8315     8325   +10     
========================================
+ Hits         32620    32645   +25     
- Misses        2503     2504    +1     
  Partials       235      235           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@devops-github-rudderstack
Copy link
Contributor

Allure Test reports for this run are available at:

@mihir20 mihir20 force-pushed the mihir/pipe-2632 branch 2 times, most recently from 930f32c to 8872019 Compare December 23, 2025 14:56
@devops-github-rudderstack
Copy link
Contributor

Allure Test reports for this run are available at:

@devops-github-rudderstack
Copy link
Contributor

Allure Test reports for this run are available at:

@mihir20 mihir20 marked this pull request as ready for review December 26, 2025 07:21
@mihir20 mihir20 requested a review from a team as a code owner December 26, 2025 07:21
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (3)
.github/workflows/build-pr-artifacts.yml (1)

38-48: PR-number-based tagging is a good improvement.

Using pr-${{ github.event.pull_request.number }} instead of branch-derived names avoids issues with special characters like / in branch names and provides more predictable, cleaner tags.

However, the comment on line 38 is now stale — it mentions "Replace problematic characters in branch name" but the implementation now uses PR numbers directly. Consider updating or removing this comment.

🔎 Suggested comment update
-      # Replace problematic characters in branch name (like '/') with safe characters (like '.')
+      # Use PR number for consistent, clean tag names
       - name: Generate Tag Names
.github/workflows/build-push-docker-image-ecr.yaml (2)

77-83: Shell conditional syntax is fragile.

The current syntax if ${{inputs.use_merge_sha}} == true performs string comparison in a fragile way. When use_merge_sha is true, this expands to if true == true which works, but the comparison relies on implicit shell behavior.

Consider using proper shell conditional syntax for robustness:

🔎 Suggested fix
       - name: Checkout SHA
         id: getSHA
         run: |
-          if ${{inputs.use_merge_sha}} == true; then
+          if [[ "${{ inputs.use_merge_sha }}" == "true" ]]; then
             sha=$(echo ${{github.sha}})
           else
             sha=$(echo ${{ github.event.pull_request.head.sha }})
           fi
           echo "SHA: $sha"
           echo "SHA=$sha" >> $GITHUB_OUTPUT

1-39: Well-structured reusable workflow with comprehensive inputs.

The workflow definition includes all necessary inputs with appropriate types and defaults. The workflow_url input appears unused in the current implementation — consider removing it if not needed, or documenting its intended purpose.

If workflow_url input is not being used, consider removing it to reduce confusion:

       skip_tests:
         type: boolean
         default: false
         description: if this option is true, we would skip tests while building docker image
-      workflow_url:
-        type: string
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0a4a747 and 389e689.

📒 Files selected for processing (6)
  • .github/workflows/build-pr-artifacts.yml
  • .github/workflows/build-push-docker-image-ecr.yaml
  • .github/workflows/prepare-for-dev-deploy.yml
  • .github/workflows/prepare-for-prod-dt-deploy.yml
  • .github/workflows/prepare-for-prod-ut-deploy.yml
  • .github/workflows/prepare-for-staging-deploy.yml
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/build-push-docker-image-ecr.yaml

106-106: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions

(deprecated-commands)


121-121: label "ubuntu-22" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file

(runner-label)

🔇 Additional comments (11)
.github/workflows/prepare-for-prod-dt-deploy.yml (2)

74-89: LGTM! ECR build job correctly mirrors the DockerHub build configuration.

The new build-transformer-image-ecr job appropriately replicates the parameters from the existing build-transformer-image job while targeting the ECR workflow. The conditional trigger, dependency on generate-tag-names, and build parameters (including skip_tests for hotfix branches) are all consistent.


94-94: Dependency chain correctly updated.

Adding build-transformer-image-ecr to the needs array ensures that the PR creation step waits for both DockerHub and ECR image builds to complete before proceeding with Helm chart updates.

.github/workflows/prepare-for-dev-deploy.yml (1)

93-120: ECR build jobs correctly added for dev environment.

Both build-transformer-image-ecr and build-user-transformer-image-ecr jobs appropriately mirror their DockerHub counterparts with consistent parameters. The use of use_merge_sha: true for the transformer image and its absence for the user transformer image matches the existing pattern.

Note: The restart-k8s-deployment and restart-k8s-deployment-dedicated jobs only depend on build-transformer-image (DockerHub), not on the new ECR builds. If you intend to deploy ECR images to dev K8s in the future, you may need to update these dependencies. Please confirm this is intentional for the current rollout strategy.

.github/workflows/build-pr-artifacts.yml (1)

83-110: ECR build jobs correctly added for PR artifacts.

Both ECR jobs properly mirror the DockerHub build jobs with identical skip conditions and parameters. The dependency on generate-tag-names ensures proper tag propagation.

.github/workflows/prepare-for-prod-ut-deploy.yml (2)

79-94: ECR build job for user transformer correctly configured.

The build-user-transformer-image-ecr job appropriately mirrors the existing build-user-transformer-image job with all parameters including build_type: ut, use_merge_sha: true, and the skip_tests conditional for hotfix releases.


99-99: Dependency chain correctly updated for PR creation.

The create-pull-request job now waits for both the DockerHub and ECR image builds to complete.

.github/workflows/prepare-for-staging-deploy.yml (2)

86-114: ECR build jobs for staging correctly added.

Both ECR jobs appropriately mirror their DockerHub counterparts. The use_merge_sha: true is correctly applied only to the transformer image (not the user transformer), matching the existing pattern in the DockerHub jobs.


119-126: Comprehensive dependency chain for PR creation.

The create-pull-request job now properly waits for all four image builds (DockerHub and ECR variants for both transformer types) before proceeding with Helm chart updates.

.github/workflows/build-push-docker-image-ecr.yaml (3)

166-175: Clarify test execution logic.

The condition ${{ inputs.skip_tests != true || needs.get_changed_files.outputs.should_execute_tests == 'true' }} means:

  • Run tests if skip_tests is false (expected behavior)
  • Also run tests if Docker-related files changed, even when skip_tests is true

This "override" behavior may be intentional (force tests when Dockerfiles change), but it could surprise users who explicitly set skip_tests: true for hotfix releases. Consider adding a comment to clarify this is the intended behavior.


295-309: Multi-arch manifest creation looks correct.

The manifest creation properly combines the architecture-specific images (-amd64 and -arm64 suffixes) into unified manifests. The conditional latest tag updates for dt and ut build types are appropriately scoped.


119-122: Confirm self-hosted runner labels are correctly configured.

The ubuntu-22 label is used consistently across multiple workflows (build-push-docker-image-ecr.yaml and build-push-docker-image.yml), with no alternative variants like ubuntu-22.04 in use. However, verify that your self-hosted ARM64 runners are actually tagged with ubuntu-22 in your runner configuration to ensure jobs can be scheduled successfully. This is a local infrastructure check outside the repository's workflow definitions.

Comment on lines 106 to 117
run: |
readarray -t modified_files <<<"$(jq -r '.[]' <<<'${{ steps.files.outputs.modified }}')"
echo "Modified files: $modified_files"
found=false
for modified_file in "${modified_files[@]}"; do
if [[ "$modified_file" == "Dockerfile" || "$modified_file" == "docker-compose.yml" || "$modified_file" == "Dockerfile" || "$modified_file" == "Dockerfile-ut-func" ]]; then
found=true
break
fi
done
echo "Match Found: $found"
echo "::set-output name=should_execute_tests::$found"
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix deprecated set-output workflow command.

The ::set-output command is deprecated and will eventually stop working. Use the $GITHUB_OUTPUT environment file approach instead.

Additionally, there's a duplicate check for "Dockerfile" in line 111.

🔎 Suggested fix
       - id: processing
         run: |
           readarray -t modified_files <<<"$(jq -r '.[]' <<<'${{ steps.files.outputs.modified }}')"
           echo "Modified files: $modified_files"
           found=false
           for modified_file in "${modified_files[@]}"; do
-            if [[ "$modified_file" == "Dockerfile" || "$modified_file" == "docker-compose.yml" || "$modified_file" == "Dockerfile" || "$modified_file" == "Dockerfile-ut-func" ]]; then
+            if [[ "$modified_file" == "Dockerfile" || "$modified_file" == "docker-compose.yml" || "$modified_file" == "Dockerfile-ut-func" ]]; then
               found=true
               break
             fi
           done
           echo "Match Found: $found"
-          echo "::set-output name=should_execute_tests::$found"
+          echo "should_execute_tests=$found" >> $GITHUB_OUTPUT
📝 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: |
readarray -t modified_files <<<"$(jq -r '.[]' <<<'${{ steps.files.outputs.modified }}')"
echo "Modified files: $modified_files"
found=false
for modified_file in "${modified_files[@]}"; do
if [[ "$modified_file" == "Dockerfile" || "$modified_file" == "docker-compose.yml" || "$modified_file" == "Dockerfile" || "$modified_file" == "Dockerfile-ut-func" ]]; then
found=true
break
fi
done
echo "Match Found: $found"
echo "::set-output name=should_execute_tests::$found"
- id: processing
run: |
readarray -t modified_files <<<"$(jq -r '.[]' <<<'${{ steps.files.outputs.modified }}')"
echo "Modified files: $modified_files"
found=false
for modified_file in "${modified_files[@]}"; do
if [[ "$modified_file" == "Dockerfile" || "$modified_file" == "docker-compose.yml" || "$modified_file" == "Dockerfile-ut-func" ]]; then
found=true
break
fi
done
echo "Match Found: $found"
echo "should_execute_tests=$found" >> $GITHUB_OUTPUT
🧰 Tools
🪛 actionlint (1.7.9)

106-106: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions

(deprecated-commands)

🤖 Prompt for AI Agents
.github/workflows/build-push-docker-image-ecr.yaml lines 106-117: the job uses
the deprecated ::set-output command and also repeats "Dockerfile" in the
file-check condition; replace the set-output usage by appending
"should_execute_tests=$found" to the GITHUB_OUTPUT file (e.g. echo
"should_execute_tests=$found" >> "$GITHUB_OUTPUT") and remove the duplicate
"Dockerfile" entry from the if-statement so each filename is checked only once.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
.github/workflows/build-push-docker-image-ecr.yaml (1)

103-115: Remove duplicate "Dockerfile" check.

Line 109 contains a duplicate check for "Dockerfile". This is redundant and should be removed for clarity.

🔎 Suggested fix
       - id: processing
         run: |
           readarray -t modified_files <<<"$(jq -r '.[]' <<<'${{ steps.files.outputs.modified }}')"
           echo "Modified files: $modified_files"
           found=false
           for modified_file in "${modified_files[@]}"; do
-            if [[ "$modified_file" == "Dockerfile" || "$modified_file" == "docker-compose.yml" || "$modified_file" == "Dockerfile" || "$modified_file" == "Dockerfile-ut-func" ]]; then
+            if [[ "$modified_file" == "Dockerfile" || "$modified_file" == "docker-compose.yml" || "$modified_file" == "Dockerfile-ut-func" ]]; then
               found=true
               break
             fi
           done
           echo "Match Found: $found"
           echo "should_execute_tests=$found" >> $GITHUB_OUTPUT

Note: The GITHUB_OUTPUT usage on line 115 is already correct and does not need to be changed.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 389e689 and 759b3c8.

📒 Files selected for processing (2)
  • .github/workflows/build-pr-artifacts.yml
  • .github/workflows/build-push-docker-image-ecr.yaml
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/build-push-docker-image-ecr.yaml

119-119: label "ubuntu-22" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file

(runner-label)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
  • GitHub Check: Build Transformer Docker Image(ECR) - PR / Build Transformer Docker Image AMD64
  • GitHub Check: Build User Transformer Docker Image(ECR) - PR / Build Transformer Docker Image AMD64
  • GitHub Check: Build User Transformer Docker Image(ECR) - PR / Build Transformer Docker Image ARM64
  • GitHub Check: Build Transformer Docker Image - PR / Build Transformer Docker Image ARM64
  • GitHub Check: Build Transformer Docker Image - PR / Build Transformer Docker Image AMD64
  • GitHub Check: Build User Transformer Docker Image - PR / Build Transformer Docker Image AMD64
  • GitHub Check: Build User Transformer Docker Image - PR / Build Transformer Docker Image ARM64
  • GitHub Check: Code Coverage
  • GitHub Check: UT Tests
  • GitHub Check: Check for formatting & lint errors
  • GitHub Check: test_and_publish
  • GitHub Check: Analyze (go)
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
.github/workflows/build-pr-artifacts.yml (3)

83-96: LGTM - ECR transformer build job properly configured.

The new ECR build job correctly mirrors the existing DockerHub build job with appropriate skip conditions and dependencies. Using OIDC authentication instead of secrets is a security improvement.


98-111: LGTM - User transformer ECR build job properly configured.

The user transformer ECR build job is correctly configured with the appropriate Dockerfile and tag naming. Structure is consistent with the main transformer ECR job.


38-48: Tag naming strategy improved.

Switching from branch-based to PR-number-based tags is cleaner and more predictable. PR numbers are immutable and avoid issues with special characters in branch names. All downstream consumers within the workflow correctly reference the output variables, ensuring no breaking changes. The conditional logic appropriately skips release and hotfix branches, which have their own separate tag generation in other deployment workflows.

.github/workflows/build-push-docker-image-ecr.yaml (7)

1-36: LGTM - Workflow inputs and permissions properly defined.

The workflow inputs cover all necessary parameters for flexible image building, and permissions are correctly scoped for OIDC authentication with ECR.


39-60: LGTM - Dependabot check implemented correctly.

The actor check logic is clear and uses the correct GITHUB_OUTPUT format for setting outputs.


117-123: Custom runner label is acceptable.

Line 119 uses "ubuntu-22" which triggers an actionlint warning, but this is expected for custom self-hosted runner labels. The configuration is correct.


164-173: Verify test execution logic.

The test condition on line 165 uses OR logic: tests run if skip_tests != true OR files changed. This means tests will run even when skip_tests=true if relevant files changed, which may not be the intended behavior.

If skip_tests=true is meant to always skip tests regardless of file changes, the logic should use AND instead of OR.

Please confirm the intended behavior:

  • Current behavior: Tests run if skip_tests is false OR if Dockerfile/docker-compose.yml changed
  • Alternative behavior: Tests run only if skip_tests is false (and ignore file changes)
🔎 Suggested fix (if skip_tests should take precedence)
       - name: Run Tests
-        if: ${{ inputs.skip_tests != true || needs.get_changed_files.outputs.should_execute_tests == 'true' }}
+        if: ${{ inputs.skip_tests != true && needs.get_changed_files.outputs.should_execute_tests == 'true' }}
         env:
           BUILD_TAG: ${{ steps.login-ecr.outputs.registry }}/${{ inputs.build_tag }}
         run: |

Or, if tests should always run when skip_tests is false:

       - name: Run Tests
-        if: ${{ inputs.skip_tests != true || needs.get_changed_files.outputs.should_execute_tests == 'true' }}
+        if: ${{ inputs.skip_tests != true }}
         env:
           BUILD_TAG: ${{ steps.login-ecr.outputs.registry }}/${{ inputs.build_tag }}
         run: |

175-191: LGTM - ARM64 build and push configured correctly.

The multi-platform build step properly tags with architecture suffix and is correctly gated for dependabot. Cache configuration is commented out, which may be intentional during initial rollout.


192-266: LGTM - AMD64 build job mirrors ARM64 correctly.

The AMD64 build job is properly structured and consistent with the ARM64 job. Note that the test execution condition on line 240 has the same logical consideration as mentioned for the ARM64 job.


267-307: LGTM - Multi-arch manifest creation properly implemented.

The manifest creation job correctly combines ARM64 and AMD64 images and conditionally creates latest tags based on build_type. The logic ensures proper multi-architecture image publishing.

Comment on lines +72 to +81
- name: Checkout SHA
id: getSHA
run: |
if ${{inputs.use_merge_sha}} == true; then
sha=$(echo ${{github.sha}})
else
sha=$(echo ${{ github.event.pull_request.head.sha }})
fi
echo "SHA: $sha"
echo "SHA=$sha" >> $GITHUB_OUTPUT
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix bash conditional syntax.

Line 75 uses incorrect syntax for the bash conditional. The expression if ${{inputs.use_merge_sha}} == true should use proper bash test syntax.

🔎 Suggested fix
       - name: Checkout SHA
         id: getSHA
         run: |
-          if ${{inputs.use_merge_sha}} == true; then
+          if [[ "${{inputs.use_merge_sha}}" == "true" ]]; then
             sha=$(echo ${{github.sha}})
           else
             sha=$(echo ${{ github.event.pull_request.head.sha }})
           fi
           echo "SHA: $sha"
           echo "SHA=$sha" >> $GITHUB_OUTPUT
🤖 Prompt for AI Agents
In .github/workflows/build-push-docker-image-ecr.yaml around lines 72 to 81 the
bash conditional is using incorrect syntax (if ${{inputs.use_merge_sha}} ==
true). Replace it with a proper shell test that quotes the expanded GitHub input
and uses = (or == inside [[ ]]) with correct spacing and brackets, e.g. if [
"${{ inputs.use_merge_sha }}" = "true" ]; then, so the conditional evaluates
correctly and avoids syntax errors.

@devops-github-rudderstack
Copy link
Contributor

Allure Test reports for this run are available at:

@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants