[HYPERSHELL-111] fix(kind): build LOCAL_IMAGES from working tree by default - #150
[HYPERSHELL-111] fix(kind): build LOCAL_IMAGES from working tree by default#150rh-amarin wants to merge 2 commits into
Conversation
…efault build-images.sh built from origin/main, creating a mismatch with the branch's scripts, manifests, and seed data on feature branches. Also used `kind load docker-image` which fails with Podman. Default BUILD_SOURCE to `worktree` so images match the current branch. Add BUILD_SOURCE=baseline opt-in for origin/main builds. Use `kind load image-archive` (tarball) for Podman compatibility, matching swap-component.sh. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ransformer Stop tagging local images with registry refs and loading them under quay.io names. Instead, load the localhost/ images directly and use a kustomize images transformer at manifest-apply time to map registry refs to localhost/ equivalents. This way pods start with the correct image on the first rollout -- kubectl describe pod shows localhost/hypershell-controller:dev instead of a registry ref that could be mistaken for a remote pull. Also fix the "empty patch" error during FORCE_ROLLOUT: track which deployments were already restarted by the operational section (JWKS timing, watch-stream timing) and skip them in the FORCE_ROLLOUT loop to avoid duplicate rollout restart calls within the same second. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
c7d8109 to
1977ebd
Compare
Amber reviewStatus: Complete VerdictApprove with minor nits (COMMENT). This is a well-scoped local-development tooling change: Hi, Amber here. I reviewed the diff against What's good
Findings1. [Minor] 2. [Minor] Transformer match name is derived from Cross-PR coordinationI listed all 24 open PRs and compared goals, designs, ownership, data models, interfaces, and change order against this PR. I focused on the ones that touch the same files or the same local-dev build story: #211, #148, #206, #194, #182 (files), #212/#109 (Makefile), #214/#209 ( No material cross-PR conflict found. Details on the closest candidates:
Only #150 modifies Findings Summary (ordered by severity, highest first)
Convention Checklist
Confidence overall: High. This is dev-only tooling; the two nits are robustness/edge-case, not correctness-blocking for the default path. |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
Approve with minor nits (COMMENT). This is a well-scoped local-development tooling change: LOCAL_IMAGES=true now builds from the working tree by default (with a BUILD_SOURCE=baseline opt-in), switches to Podman-compatible kind load image-archive, and injects localhost/ refs via a kustomize images transformer at apply time. Two minor robustness issues are worth addressing but nothing blocks merge.
Hi, Amber here. I reviewed the diff against CLAUDE.md, the security and control-plane conventions, and the HyperShell review checklists. This PR touches only Kind dev tooling (Makefile, scripts/kind/build-images.sh, scripts/kind/up.sh) and the local-development spec — no Go production code, no API/proto/OpenAPI, no test files — so most Go conventions (panic, error wrapping, IsNotFound) are N/A here. The image-ref/spec consistency I did check looks correct: api_server_local=localhost/hypershell:dev, control_plane_local=localhost/hypershell-controller:dev, and web_console_local=localhost/hypershell-web-console:dev in the Makefile match the spec's "Local image names" table and the kustomize transformer output.
What's good
- The Podman fix (
kind load docker-image→save+kind load image-archive) is correct and consistent with the existingswap-component.shapproach (scripts/kind/swap-component.sh:231-232). - The apply-time kustomize
imagestransformer avoids the deploy-then-set imagerollout churn, and the spec's rationale is clearly documented. - The
FORCE_ROLLOUTdedup (_api_restarted/_cp_restarted) correctly avoids the duplicaterollout restart"empty patch" error for deployments already restarted by the JWKS/watch-stream steps. - The
BUILD_SOURCEgating cleanly preserves the oldorigin/mainworktree path behindbaseline, and the spec + scenarios were updated to match (including a new "Baseline Build" scenario).
Findings
1. [Minor] deploy/.local-images temp overlay is not cleaned up on failure and is not git-ignored — scripts/kind/up.sh:244-264
The overlay dir is created under deploy/ and removed only on the happy path (rm -rf at line 264). Because the script runs under set -euo pipefail, a failing kustomize build or kube apply exits before the cleanup, leaving deploy/.local-images/kustomization.yaml in the working tree. .gitignore ignores .local/ (line 18) but not .local-images, so a generated overlay could be accidentally committed. Prefer a trap ... EXIT cleanup (as build-images.sh already does for its worktree), a mktemp -d outside the source tree, or adding deploy/.local-images/ to .gitignore. Confidence: High.
2. [Minor] Transformer match name is derived from IMAGE_REGISTRY, but the base manifests hardcode the quay.io name — scripts/kind/up.sh:246-262
The overlay's images[].name uses _registry="${IMAGE_REGISTRY:-quay.io/...}", but deploy/kind/kustomization.yaml:183-187 hardcodes the image names as quay.io/redhat-services-prod/hcm-eng-prod-tenant/hypershell-main/hypershell-*-main regardless of IMAGE_REGISTRY. If a developer overrides IMAGE_REGISTRY (a documented variable), the transformer's name: no longer matches any image in the base, so the substitution silently no-ops and pods keep the unreachable registry refs — defeating the offline/LOCAL_IMAGES intent with no error. Match the base's actual (hardcoded) image name for the transformer's name: field, and keep newName/newTag from *_local. Confidence: Medium.
Cross-PR coordination
I listed all 24 open PRs and compared goals, designs, ownership, data models, interfaces, and change order against this PR. I focused on the ones that touch the same files or the same local-dev build story: #211, #148, #206, #194, #182 (files), #212/#109 (Makefile), #214/#209 (swap-component.sh).
No material cross-PR conflict found. Details on the closest candidates:
- #211 (
fix(kind): expose gateway metrics and fix control plane connectivity) — Also editsscripts/kind/up.sh,deploy/kind/kustomization.yaml, andMakefile. Itsup.shedits are in the summary-output section and itskustomization.yamledits add controller env/dnsConfigand a ServiceMonitor — different regions from this PR's deploy section and image-ref handling. Because #150's.local-imagesoverlay simply references../kind, it transparently inherits whatever #211 adds to the base. This is complementary, not conflicting — no design decision needed, only ordinary line-level merge if both land. - #148 (
docs(specs): add OpenShell branch build spec for kind-openshell-up) — Extends the samelocal-development.spec.md"build from source" narrative and addsOPENSHELL_*build variables. However, it builds OpenShell gateway/supervisor/sandbox images from a branch, whereas #150 builds HyperShell platform images from the working tree; different image sets and different targets. Not a duplicate solution and not an incompatible design — the two build stories are additive. Worth keeping the spec's env-var tables coherent when both merge, but no maintainer decision required. - #194 (
adopt upstream OpenShell Helm chart), #206 (hsctl login), #182 (enforce management API JWT audience) — Share only unrelated regions oflocal-development.spec.md,lib.sh, orkustomization.yaml. No overlapping goals, interfaces, or ordering constraints with this PR. - #212 / #109 (Makefile), #214 / #209 (
swap-component.sh) — Touch shared files in unrelated sections; no logical or ordering conflict with this change.
Only #150 modifies scripts/kind/build-images.sh and the LOCAL_IMAGES build-source behavior, so there is no competing implementation of this feature.
Findings Summary (ordered by severity, highest first)
- [Minor] Temp overlay
deploy/.local-imagesnot cleaned up on failure and not git-ignored — Robustness / Idempotency (up.sh L244-264) - [Minor] Transformer
name:usesIMAGE_REGISTRYwhile base manifests hardcodequay.io; silent no-op underIMAGE_REGISTRYoverride — Correctness (up.sh L246-262)
Convention Checklist
| Convention | Result |
|---|---|
| Image references consistent across manifests/spec | Pass |
| Reconcile-style idempotent tooling (converges on rerun) | Pass |
| Podman/Docker parity for image load | Pass |
| Config separate from code (env-var driven build source) | Pass |
| Conventional commit messages | Pass |
| Spec updated to match behavior change | Pass |
| No secrets in scripts/logs | Pass |
| Temp artifacts cleaned up on all paths | Fail |
Confidence overall: High. This is dev-only tooling; the two nits are robustness/edge-case, not correctness-blocking for the default path.
| newTag: ${web_console_local##*:} | ||
| EOF | ||
| kustomize build "${_kustomize_dir}" | kube apply -f - | ||
| rm -rf "${_kustomize_dir}" |
There was a problem hiding this comment.
[Minor] This rm -rf only runs on the happy path. Under set -euo pipefail, a failing kustomize build or kube apply above exits before cleanup, leaving deploy/.local-images/kustomization.yaml in the working tree. .gitignore ignores .local/ but not .local-images, so it can be committed by accident. Suggest a trap ... EXIT cleanup (like build-images.sh does for its worktree), or mktemp -d outside the source tree, or add deploy/.local-images/ to .gitignore.
| info "Applying Kind manifests with localhost image refs..." | ||
| _kustomize_dir="deploy/.local-images" | ||
| mkdir -p "${_kustomize_dir}" | ||
| _registry="${IMAGE_REGISTRY:-quay.io/redhat-services-prod/hcm-eng-prod-tenant/hypershell-main}" |
There was a problem hiding this comment.
[Minor] The transformer name: fields below are built from _registry (${IMAGE_REGISTRY:-quay.io/...}), but the base manifests hardcode the quay.io/.../hypershell-*-main names in deploy/kind/kustomization.yaml:183-187 regardless of IMAGE_REGISTRY. If a developer overrides IMAGE_REGISTRY, the images transformer no longer matches any image in the base, so substitution silently no-ops and pods keep the unreachable registry refs — defeating LOCAL_IMAGES. Match the base's actual (hardcoded) image name for name:, keeping newName/newTag from *_local.
2 participants
Summary
build-images.shnow defaults to building from the working tree instead oforigin/main, so images match the branch's scripts, manifests, and seed data on feature branchesBUILD_SOURCE=baselineopt-in to build fromorigin/mainfor baseline comparisonkind load docker-imagewithkind load image-archive(tarball) for Podman compatibility, matching the approach already used byswap-component.shMotivation
On feature branches that modify API contracts, database schemas, or seed data (e.g. the CNPG migration),
LOCAL_IMAGES=true make kind-upbuilt images fromorigin/mainbut ranup.shfrom the feature branch. This mismatch caused seed failures (API server doesn't know about new fields) and broken deployments. Thekind load docker-imagecommand also fails entirely with Podman on macOS.Test plan
LOCAL_IMAGES=true make kind-upbuilds from working tree and deploys successfullyLOCAL_IMAGES=true BUILD_SOURCE=baseline make kind-upbuilds fromorigin/mainkind load image-archive)make kind-api-server-up/make kind-control-plane-upstill work (unchanged)JIRA: https://redhat.atlassian.net/browse/HYPERSHELL-111
🤖 Generated with Claude Code