Skip to content

ci: auto-publish model-engine helm chart to public ECR on merge (MLI-7871) - #859

Open
billyang-scale wants to merge 1 commit into
scaleapi:mainfrom
billyang-scale:billyang/_claude/publish-helm-chart-ci
Open

ci: auto-publish model-engine helm chart to public ECR on merge (MLI-7871)#859
billyang-scale wants to merge 1 commit into
scaleapi:mainfrom
billyang-scale:billyang/_claude/publish-helm-chart-ci

Conversation

@billyang-scale

@billyang-scale billyang-scale commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

What

Auto-publishes the model-engine Helm chart to the public ECR registry on merge to main, replacing the manual on-call helm push (MLI-7871, MLI-7863).

  • New publish_helm_chart CircleCI job — main-only, gated on run_unit_tests_python_client + run_unit_tests_server + build_image. Assumes the same ${CIRCLECI_ROLE_ARN} the integration_tests job already uses.
  • New .circleci/scripts/publish-helm-chart.sh — reads the version from charts/model-engine/Chart.yaml, checks whether that tag is already in public ECR, and only packages + pushes when it's new.

Workflow after this lands

Bump charts/model-engine/Chart.yaml version: in a PR → merge → the chart auto-publishes. No version bump → the job is a no-op. On-call is out of the loop.

Dependency + merge order

Requires the circleci OIDC role to have ecr-public publish permission on model-engine-helm-charts/model-engine, added in Terracode-ML https://github.com/scaleapi/Terracode-ML/pull/1530. That PR must land (and atlantis apply) before this one, or the job's push will 403.

Confirm before/at merge

The llm-engine CircleCI project env var CIRCLECI_ROLE_ARN must resolve to arn:aws:iam::692474966980:role/circleci (the role the Terracode PR grants). Every signal in Terraform + live IAM matches; the env var itself lives in CircleCI project settings.

Notes

  • Image tag cimg/aws:2023.09 — bump to a current tag if preferred.
  • Verified manually today: chart 0.2.9 published + helm pull --version 0.2.9 resolves digest sha256:8b9960…536ac8. This automates that same path.

🤖 Generated with Claude Code

Greptile Summary

This PR automates publishing the model-engine Helm chart to the public ECR registry on every merge to main, replacing the previous manual on-call helm push. The new publish_helm_chart job is gated on the three existing CI jobs and is fully idempotent — it skips the push when the chart version is already present in ECR.

  • New CircleCI job (publish_helm_chart) runs after tests and image build pass on main, uses the existing OIDC role (CIRCLECI_ROLE_ARN), and delegates all publish logic to a new shell script.
  • New publish-helm-chart.sh reads the version from Chart.yaml, checks ECR for an existing tag, and only packages + pushes when the version is new — requires the companion Terraform PR to grant ecr-public publish permission to the OIDC role before this can succeed.

Confidence Score: 4/5

Safe to merge once the companion Terraform PR (Terracode-ML #1530) is applied; the publish job will 403 until that IAM change lands.

The core logic is correct and the idempotency design is sound. The Helm installer is unpinned, diverging from the pinned pattern already used in run_unit_tests_server, and the ECR describe-images check silently treats all non-zero exits including permission errors as not yet published.

.circleci/config.yml Helm install step and the describe-images error handling in publish-helm-chart.sh

Important Files Changed

Filename Overview
.circleci/config.yml Adds publish_helm_chart job gated on tests + build_image, main-only; Helm install is unpinned unlike the existing run_unit_tests_server pattern
.circleci/scripts/publish-helm-chart.sh New idempotent publish script; version check, ECR login, package, and push are correct, but the describe-images error check silently treats all non-zero exits (including permission errors) as not published

Sequence Diagram

sequenceDiagram
    participant GH as GitHub (main merge)
    participant CCI as CircleCI
    participant ECR as ECR Public (us-east-1)

    GH->>CCI: trigger ci workflow
    par parallel gates
        CCI->>CCI: run_unit_tests_python_client
        CCI->>CCI: run_unit_tests_server
        CCI->>CCI: build_image
    end
    CCI->>CCI: publish_helm_chart (requires all three)
    CCI->>CCI: helm show chart parse VERSION from Chart.yaml
    CCI->>ECR: "ecr-public describe-images imageTag=VERSION"
    alt tag already exists
        ECR-->>CCI: exit 0 image found
        CCI->>CCI: no-op exit
    else tag not found or any error
        ECR-->>CCI: non-zero exit
        CCI->>ECR: ecr-public get-login-password
        ECR-->>CCI: token
        CCI->>CCI: helm registry login public.ecr.aws
        CCI->>CCI: helm package model-engine-VERSION.tgz
        CCI->>ECR: helm push oci to model-engine-helm-charts
        ECR-->>CCI: published
    end
Loading

Fix All in Cursor Fix All in Claude Code Fix All in Codex

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
.circleci/config.yml:39-42
The Helm install here differs from the more careful pattern already established in the `run_unit_tests_server` command (lines 322–326), which downloads the script first, pins an explicit version via `DESIRED_VERSION=v3.17.3`, and verifies with `helm version`. This job runs with live AWS OIDC credentials and is directly responsible for publishing artifacts, so using an unpinned `curl | bash` means any future change to the `main` branch of the Helm repo's installer is picked up immediately without review. Using a different Helm version than the rest of CI is also a latent inconsistency.

```suggestion
      - run:
          name: Install helm
          command: |
            curl -fsSL https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 -o get_helm.sh
            chmod 700 get_helm.sh
            DESIRED_VERSION=v3.17.3 ./get_helm.sh
            helm version
```

### Issue 2 of 2
.circleci/scripts/publish-helm-chart.sh:27-33
**Silent failure on non-404 errors in idempotency check**

`describe-images` errors are fully suppressed (`>/dev/null 2>&1`), so any failure — permission denied, transient network error, throttling — is interpreted the same way as "image not found" and causes the script to proceed to publish. If the OIDC role gains push permission but is missing `ecr-public:DescribeImages`, the check is effectively bypassed on every run; publishing would succeed on new versions but produce a redundant push attempt on unchanged versions. Consider checking the exit code separately from error output so a non-404 failure can surface rather than silently proceeding.

Reviews (1): Last reviewed commit: "ci: auto-publish model-engine helm chart..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

Adds a main-only publish_helm_chart CircleCI job (gated on unit tests + build_image) plus .circleci/scripts/publish-helm-chart.sh. The script reads the chart version from charts/model-engine/Chart.yaml and only packages + pushes when that tag is not already in public ECR, so it is safe to run on every merge - an actual publish happens only when the chart version was bumped. Replaces the manual on-call helm push. Requires the ecr-public grant on the circleci OIDC role (Terracode-ML).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment on lines +27 to +33
if aws ecr-public describe-images \
--region "${ECR_PUBLIC_REGION}" \
--repository-name "${ECR_REPOSITORY_NAME}" \
--image-ids imageTag="${VERSION}" >/dev/null 2>&1; then
echo "Chart ${VERSION} is already published to ${PUBLIC_ECR_REGISTRY}/${ECR_REPOSITORY_NAME} — nothing to do."
exit 0
fi

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Silent failure on non-404 errors in idempotency check

describe-images errors are fully suppressed (>/dev/null 2>&1), so any failure — permission denied, transient network error, throttling — is interpreted the same way as "image not found" and causes the script to proceed to publish. If the OIDC role gains push permission but is missing ecr-public:DescribeImages, the check is effectively bypassed on every run; publishing would succeed on new versions but produce a redundant push attempt on unchanged versions. Consider checking the exit code separately from error output so a non-404 failure can surface rather than silently proceeding.

Prompt To Fix With AI
This is a comment left during a code review.
Path: .circleci/scripts/publish-helm-chart.sh
Line: 27-33

Comment:
**Silent failure on non-404 errors in idempotency check**

`describe-images` errors are fully suppressed (`>/dev/null 2>&1`), so any failure — permission denied, transient network error, throttling — is interpreted the same way as "image not found" and causes the script to proceed to publish. If the OIDC role gains push permission but is missing `ecr-public:DescribeImages`, the check is effectively bypassed on every run; publishing would succeed on new versions but produce a redundant push attempt on unchanged versions. Consider checking the exit code separately from error output so a non-404 failure can surface rather than silently proceeding.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Cursor Fix in Claude Code Fix in Codex

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