ci: auto-publish model-engine helm chart to public ECR on merge (MLI-7871) - #859
Open
billyang-scale wants to merge 1 commit into
Open
Conversation
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 |
There was a problem hiding this 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.
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.
diazagasatya
approved these changes
Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Auto-publishes the
model-engineHelm chart to the public ECR registry on merge tomain, replacing the manual on-callhelm push(MLI-7871, MLI-7863).publish_helm_chartCircleCI job —main-only, gated onrun_unit_tests_python_client+run_unit_tests_server+build_image. Assumes the same${CIRCLECI_ROLE_ARN}theintegration_testsjob already uses..circleci/scripts/publish-helm-chart.sh— reads the version fromcharts/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.yamlversion: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
circleciOIDC role to haveecr-publicpublish permission onmodel-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_ARNmust resolve toarn: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
cimg/aws:2023.09— bump to a current tag if preferred.0.2.9published +helm pull --version 0.2.9resolves digestsha256:8b9960…536ac8. This automates that same path.🤖 Generated with Claude Code
Greptile Summary
This PR automates publishing the
model-engineHelm chart to the public ECR registry on every merge tomain, replacing the previous manual on-callhelm push. The newpublish_helm_chartjob 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.publish_helm_chart) runs after tests and image build pass onmain, uses the existing OIDC role (CIRCLECI_ROLE_ARN), and delegates all publish logic to a new shell script.publish-helm-chart.shreads the version fromChart.yaml, checks ECR for an existing tag, and only packages + pushes when the version is new — requires the companion Terraform PR to grantecr-publicpublish 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
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 endPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "ci: auto-publish model-engine helm chart..." | Re-trigger Greptile