feat: add Shipwright in-cluster build support to setup-k8s.sh#148
Conversation
Adds deploy/knative/setup-shipwright-build.sh plus templated Build manifests so anyone on a Kubernetes cluster with Shipwright installed can build the harness/sandbox images in-cluster and feed the resulting refs straight into setup-k8s.sh --image/--sandbox-image, instead of needing a local Docker daemon or pushing over the internet. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Jeremy Cohn <Jeremy.Cohn@ibm.com>
… name Live-cluster testing showed that reusing the hardcoded "serverless-harness" Build object name silently overwrote an existing Build's revision/output image. Default the Build name to include --tag (serverless-harness-<tag>) so a test run with a different tag can't collide with an existing Build, add --build-name to target a specific Build on purpose, and warn before overwriting a Build whose revision/output differs from what this run would set. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Jeremy Cohn <Jeremy.Cohn@ibm.com>
--build-name combined with --with-sandbox produced identical harness/ sandbox Build names instead of acting as a prefix, contradicting its own help text. Sandbox now gets "-sandbox" appended. warn_if_build_exists_with_different_spec swallowed all kubectl errors as "no existing Build" via `2>/dev/null || return 0`, and used grep substring matching on raw JSON that produces false negatives on plausible inputs (e.g. v2.1.0 vs 2.1.0). Now distinguishes real kubectl errors from NotFound, and compares exact jsonpath-extracted fields instead of substrings. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Jeremy Cohn <Jeremy.Cohn@ibm.com>
|
Note on the failing
Remediation (a dependency bump, |
cwiklik
left a comment
cwiklik
left a comment
There was a problem hiding this comment.
Adds an in-cluster Shipwright build path for the generic setup-k8s.sh flow: a driver script + two rendered Build CR templates + README. Nicely done — set -euo pipefail, stderr-only logging (keeps stdout clean for the HARNESS_IMAGE=/SANDBOX_IMAGE= output and --dry-run YAML), a careful warn-before-clobbering-an-existing-Build guard, and tag-derived Build names to avoid collisions. Passes shellcheck/hadolint/CodeQL; no secrets, no .claude/.vscode.
Three non-blocking notes, all on the build-failure path (see inline).
CI note: trivy-scan is failing, but it's the pre-existing dependency CVE shared across this repo's PRs — this PR adds no dependency manifests (only shell/YAML/docs), so it's not attributable to #148. That check will still gate merge; it needs an org-wide fix (dependency bump or a scoped .trivyignore), separate from this review.
LGTM.
Assisted-By: Claude Code
| )" | ||
| log_info "Waiting for BuildRun/${buildrun} (timeout: ${WAIT_TIMEOUT})" | ||
| if ! "${KUBECTL[@]}" -n "$NAMESPACE" wait "buildrun/${buildrun}" \ | ||
| --for=condition=Succeeded --timeout="$WAIT_TIMEOUT"; then |
There was a problem hiding this comment.
suggestion (non-blocking): kubectl wait --for=condition=Succeeded doesn't short-circuit on failure. When a BuildRun fails, its Succeeded condition goes to False, and kubectl wait keeps blocking until --wait-timeout (default 20m) before it reports the failure. Consider also racing a --for=condition=Succeeded=false wait (kubectl ≥1.31) or polling .status.conditions, so a failed build surfaces promptly instead of hanging for the full timeout.
| if ! "${KUBECTL[@]}" -n "$NAMESPACE" wait "buildrun/${buildrun}" \ | ||
| --for=condition=Succeeded --timeout="$WAIT_TIMEOUT"; then | ||
| log_error "Build failed — logs:" | ||
| "${KUBECTL[@]}" -n "$NAMESPACE" logs "buildrun.shipwright.io/${buildrun}" --all-containers >&2 || true |
There was a problem hiding this comment.
nit: kubectl logs buildrun.shipwright.io/<name> won't return the build pod's logs — a BuildRun isn't a pod and has no logs subresource, so this best-effort (|| true) line likely prints nothing on failure. Fetching the underlying TaskRun/pod logs (or shp buildrun logs) would make the failure path actually actionable.
| sed \ | ||
| -e "s#__NAME__#${name}#g" \ | ||
| -e "s#__NAMESPACE__#${NAMESPACE}#g" \ | ||
| -e "s#__GIT_URL__#${GIT_URL}#g" \ |
There was a problem hiding this comment.
nit: render_build uses sed with # as the delimiter while substituting user-supplied values (__GIT_URL__, __IMAGE__, …). A literal # in a git URL or image ref would break the substitution. Unlikely in practice, but envsubst or a delimiter guaranteed not to appear in the input would be more robust.
Addresses review nits on rossoctl#148: - render_build used sed with '#' as the delimiter over user-supplied values; a '#' in a git URL/image ref (or '&'/'\' in a replacement) would corrupt the output. Switch to bash literal string replacement, which has no delimiter or metachar hazard. - The final `$BUILD_SANDBOX && echo ...`, as the last statement under `set -e`, short-circuited to exit 1 on a successful harness-only build (no --with-sandbox). Use an explicit if so success reports exit 0. Verified: shellcheck clean; dry-run renders a '#'-containing revision intact and exits 0. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Jeremy Cohn <Jeremy.Cohn@ibm.com>
|
Addressed the non-blocking review nits (thanks @cwiklik):
Deferred the two build-failure-path suggestions (short-circuit on BuildRun failure; fetch TaskRun/pod logs) as follow-ups — they're behavior changes worth their own change. shellcheck clean. |
2 participants
Background
The Kind and OpenShift setup paths each have a way to get images built for the
cluster —
setup-kind.shbuilds locally andkind loads them, andsetup-ocp.shcan lean on OpenShift's built-in
oc new-build. The generic Kubernetes path added in#142 (
setup-k8s.sh) consumes prebuilt image refs via--image/--sandbox-image,since vanilla Kubernetes has no standard in-cluster build system to build them for you.
This PR adds an in-cluster build option for that path, using
Shipwright: a small driver script that builds the harness
(and optionally sandbox) image straight from a git branch to a registry the cluster
can already reach — no local Docker daemon and nothing pushed over the internet — and
prints the resulting image refs ready to hand to
setup-k8s.sh. It's opt-in andchanges nothing for users who already build their images elsewhere.
What's included
deploy/knative/setup-shipwright-build.sh— driver script that creates a ShipwrightBuild+BuildRun, waits for it to complete, and printsHARNESS_IMAGE=/SANDBOX_IMAGE=lines that feed directly into
setup-k8s.sh --image/--sandbox-image.Buildmanifests underdeploy/knative/shipwright/(build-harness.yaml,build-sandbox.yaml).deploy/knative/README-k8s.mdwith usage instructions and where this fitsalongside the local-build path.
the Shipwright
Buildobject's name (serverless-harness), so re-running it couldsilently overwrite an existing Build of the same name and clobber its config. Fixed by
deriving the default Build name from
--tag(e.g.serverless-harness-<tag>), adding--build-nameto opt into targeting a specific existing Build on purpose, and warningbefore overwriting a Build whose spec differs from what the current run would set.
--build-name, when combined with--with-sandbox, produced identical harness/sandboxBuild names instead of acting as a prefix; and the guard's existence check swallowed real
kubectl errors as "no existing Build" and used unreliable substring matching. Fixed both.
Test plan
This is a draft PR — code review and live-cluster testing (including a re-verification
pass of the latest fix) are complete. Remaining before marking ready: the user's own final
review.
harness image in-cluster via
setup-shipwright-build.sh.commit above).
--build-namenow produces distinct harness/sandbox Build names, the guard warns ona genuine revision/image mismatch, does not false-positive on an idempotent re-run,
and does not misreport a not-yet-created Build as an error. Production
Build/serverless-harnessandBuild/serverless-harness-sandboxwere unchangedthroughout, and all test objects were cleaned up afterward.
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com