build(operator): regenerate what the release actually ships - #90
Merged
Conversation
Packaging the operator was four manual steps with nothing enforcing them, and the two that matter most are silent when skipped. The chart carries its own copy of the CRD (helm/infera-operator/crds/), a mirror of config/crd/bases that `make manifests` never touched -- so editing api/v1alpha1 and running the generator left the chart on the old schema, and `helm install` would create a CRD that rejects the very fields the manager already understands. Nothing fails at package time; it fails on someone else's cluster. `docker build` has the same shape. The Dockerfile compiles the source tree as-is, so a missed `make generate` bakes a stale zz_generated.deepcopy.go into the image. Both are now dependencies rather than steps a README asks you to remember: docker-build: generate manifests helm-package: manifests manifests -> sync-chart-crd sync-chart-crd is its own target rather than a line inside manifests, so a chart copy that drifted without the types changing can be repaired without a regeneration. The two paths it copies between move into CRD_DIR and CHART_CRD_DIR, which also retires the third hard-coded spelling of config/crd/bases, in `install`. .gitignore drops the *.tgz that `helm package` leaves in the working tree. It was showing up untracked beside the sources, one `git add .` away from being committed. Also retargets the manager image and the chart at docker.io/rocm -- the image as an operator- tag of the shared rocm/infera repo, matching the engine images (sglang-v0.1.1, server-v0.1.1) -- and bumps the chart to 0.1.3. The zz_generated.deepcopy.go and config/rbac/role.yaml churn is controller-gen reordering only: both files are identical to their committed versions once sorted. Verified: `make -n docker-build` emits generate -> manifests -> sync-chart-crd -> docker build in that order, and `make helm-package` produces infera-operator-0.1.3.tgz carrying crds/infera.amd.com_inferadeployments.yaml byte-identical to config/crd/bases. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: leiwei12 <lei.wei@amd.com>
weilei0120
requested review from
JohnQinAMD,
jiejingzhangamd,
limou102 and
xiaobochen-amd
as code owners
August 5, 2026 07:31
688e608 retargeted the manager image and the chart at docker.io/rocm. Opening that line at 0.1.0 rather than carrying 0.1.3 across keeps the published numbering readable: the first release under the new name is the first number. Resets all three copies of the version together -- Chart.yaml (version and appVersion), values.yaml (image.tag), and the Makefile's IMG default. They are three spellings of one number, and moving fewer than all three is the quiet failure: the chart installs and then pulls a tag nobody pushed. Verified: `helm template` renders docker.io/rocm/infera:operator-v0.1.0 alongside app.kubernetes.io/version 0.1.0, and `make helm-package` produces infera-operator-0.1.0.tgz. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: leiwei12 <lei.wei@amd.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the operator’s shipped artifacts (Docker image + Helm chart) depend on code generation and CRD/RBAC manifest generation, reducing the risk of shipping stale generated code or an out-of-date CRD in the chart. It also retargets the image/chart registry namespace and bumps the Helm chart version.
Changes:
- Make
docker-buildandhelm-packagedepend on generator/manifests targets, and add async-chart-crdtarget to mirror CRDs into the Helm chart. - Retarget image/chart defaults to
docker.io/rocmand bump chartversion/appVersionto0.1.3. - Ignore packaged chart archives (
*.tgz) indeploy/operator/.gitignore.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| deploy/operator/Makefile | Adds generation/manifests dependencies for build/package and mirrors CRDs into the Helm chart. |
| deploy/operator/helm/infera-operator/values.yaml | Updates default operator image repository/tag to the new rocm/infera scheme. |
| deploy/operator/helm/infera-operator/Chart.yaml | Bumps chart version/appVersion for the new release. |
| deploy/operator/config/rbac/role.yaml | Regenerated RBAC manifest output (reordered rules/blocks). |
| deploy/operator/api/v1alpha1/zz_generated.deepcopy.go | Regenerated deepcopy output (reordered generated functions). |
| deploy/operator/.gitignore | Ignores Helm package tarballs (*.tgz). |
Files not reviewed (1)
- deploy/operator/api/v1alpha1/zz_generated.deepcopy.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+10
to
+11
| version: 0.1.3 | ||
| appVersion: "0.1.3" |
The release workflow builds engine images into docker.io/inferaimage/infera
and a separate process promotes them to docker.io/rocm/infera after review;
release.yml defaults to the staging repo for exactly that reason. The
operator's Makefile defaulted to the promotion target instead, so the one
address a bare `make docker-push` would reach was the one nobody working here
has rights to write -- and if they did, it would land in the public repo
without passing the review the two-repo split exists to enforce.
IMG and CHART_REGISTRY now both name inferaimage. What the artefacts *say*
is unchanged and deliberately so: values.yaml still embeds
rocm/infera:operator-v0.1.0 and the docs still install from
oci://docker.io/rocm/infera-operator, because a chart describes where it will
live, not where it is staged. Embedding the staging address would survive the
promotion and point every user at it.
Testing a staged chart end to end therefore needs one override:
helm install infera-operator oci://docker.io/inferaimage/infera-operator \
--version 0.1.0 -n infera-system --create-namespace \
--set image.repository=docker.io/inferaimage/infera
Note for whoever runs the promotion: this ships two artefacts, and the second
is a Helm chart rather than a container image, so an image-only promotion
would leave it behind.
inferaimage/infera:operator-v0.1.0 container image
inferaimage/infera-operator:0.1.0 Helm chart (OCI artefact)
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: leiwei12 <lei.wei@amd.com>
Every recipe told the reader to install from deploy/operator/helm/infera-operator,
a path that only resolves inside a clone of this repo. That made a git checkout
a prerequisite for a step whose only purpose is to get the CRD onto a cluster,
and it silently installed whatever the working tree happened to contain rather
than a released version.
The chart is published as an OCI artefact, so the six recipes now name it:
helm install infera-operator oci://docker.io/rocm/infera-operator --version 0.1.0 \
-n infera-system --create-namespace
This also matches how the same pages already refer to the engine images --
rocm/infera:sglang-v0.1.1 and friends, never the staging repo. The two
`helm upgrade --install` call sites keep that form; they are idempotent on
purpose, since the operator may already be installed.
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: leiwei12 <lei.wei@amd.com>
config/rbac/role.yaml is what external consumers -- Primus-SaFE among them -- build their RBAC from, and it was missing two grants the manager cannot start without: coordination.k8s.io/leases create,delete,get,list,patch,update,watch <core>/events create,patch Both belong to the controller-runtime manager rather than the reconciler: it takes a lease whenever --leader-elect is set, which the chart sets by default, and it records events. Nothing in internal/ mentions either, so controller-gen had nothing to emit and the generated role has been short since the beginning. It went unnoticed because the deployed RBAC does not come from this file. The chart carries a hand-written ClusterRole that already listed both, so every install from the chart worked -- while anyone generating from config/rbac got a manager that fails to acquire its lease and exits. That failure reads like a broken operator, not like a missing RBAC rule. Two markers fix it at the source. Verified by expanding both roles into (apiGroup, resource, verb) triples and comparing the sets: 12 rules, 87 permissions, identical. Before this they were 10 and 78. A sorted line comparison would not have caught the original gap, or proved this closes it, since neither distinguishes a reordering from a verb moving between groups. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: leiwei12 <lei.wei@amd.com>
Merged
4 tasks
jiejingzhangamd
added a commit
that referenced
this pull request
Aug 5, 2026
…fest Based on #90, which makes the manager a component tag of the shared repo (rocm/infera:operator-<version>) rather than its own -operator-manager repository. This follows that convention. Two things a release did not produce, both left to a human afterwards. **The operator manager image and its Helm chart.** The build already existed as make docker-build / docker-push / helm-push, but nothing ran it on a tag, so `helm install` pulled whatever manager image happened to be committed in values.yaml. The new job pins Chart.yaml's version and appVersion, and values.yaml's image repository and tag, to this release before packaging -- so a plain `helm install` gets the manager this run built. It runs on a GitHub-hosted runner: the operator Dockerfile is a distroless CGO_ENABLED=0 Go build, needing neither a GPU nor the SLURM dispatch the engine images go through. Deliberately not gated on `build` or `overlay` -- the operator ships no engine code, so waiting on a GPU queue would only delay a chart nobody was blocked on. The chart version drops the leading `v` while the image tag keeps it: Helm requires SemVer and `helm package` rejects "v0.2.3". The two differ by that character alone, which is worth knowing before it looks like a bug. **promotion-<id>.json.** Engine images publish to the private staging repo and are promoted to the public one after review; that promotion is a separate human-gated process, and the source-to-destination list was written by hand each time. It is now generated from `prepare`'s component list, so a partial dispatch produces a partial manifest instead of naming images that were never built. Uploaded as a workflow artifact and, on a tag, attached to the Release. Because #90 makes the manager a component tag of the same repo, it promotes exactly like the engines and is included -- which is what settles the question of whether it belonged in the manifest at all. Also hoists the image-id computation into `prepare`. It was open-coded identically in `build` and `overlay`; harmless while they agreed, but the overlay harvests the engine images *by tag*, so an id that diverged between jobs would silently harvest the wrong ones. Verified locally on this baseline: the manifest generator emits 7 entries (5 engines + overlay + operator) in the expected shape, and the chart-pinning steps render `docker.io/inferaimage/infera:operator-v0.2.3` and package to infera-operator-0.2.3.tgz. Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.
Packaging the operator was four manual steps with nothing enforcing them, and the two that matter most are silent when skipped.
The chart carries its own copy of the CRD (helm/infera-operator/crds/), a mirror of config/crd/bases that
make manifestsnever touched -- so editing api/v1alpha1 and running the generator left the chart on the old schema, andhelm installwould create a CRD that rejects the very fields the manager already understands. Nothing fails at package time; it fails on someone else's cluster.docker buildhas the same shape. The Dockerfile compiles the source tree as-is, so a missedmake generatebakes a stale zz_generated.deepcopy.go into the image.Both are now dependencies rather than steps a README asks you to remember:
docker-build: generate manifests
helm-package: manifests
manifests -> sync-chart-crd
sync-chart-crd is its own target rather than a line inside manifests, so a chart copy that drifted without the types changing can be repaired without a regeneration. The two paths it copies between move into CRD_DIR and CHART_CRD_DIR, which also retires the third hard-coded spelling of config/crd/bases, in
install..gitignore drops the *.tgz that
helm packageleaves in the working tree. It was showing up untracked beside the sources, onegit add .away from being committed.Also retargets the manager image and the chart at docker.io/rocm -- the image as an operator- tag of the shared rocm/infera repo, matching the engine images (sglang-v0.1.1, server-v0.1.1) -- and bumps the chart to 0.1.3.
The zz_generated.deepcopy.go and config/rbac/role.yaml churn is controller-gen reordering only: both files are identical to their committed versions once sorted.
Verified:
make -n docker-buildemits generate -> manifests -> sync-chart-crd -> docker build in that order, andmake helm-packageproduces infera-operator-0.1.3.tgz carrying crds/infera.amd.com_inferadeployments.yaml byte-identical to config/crd/bases.Description
Please include a brief summary of the changes, relevant motivation and context.
Fixes # (issue)
Type of change
Changes
Please list the changes introduced in this PR:
Checklist: