fix: fail clearly when an image override has an empty tag and no digest#6462
fix: fail clearly when an image override has an empty tag and no digest#6462eamonnmoloney wants to merge 2 commits into
Conversation
A nightly install of camunda-platform-8.9 failed with a cryptic "yaml: line 49: mapping values are not allowed in this context". Root cause: an --extra-values image override redirected the orchestration image registry/repository but left image.tag empty. Helm's merge deletes the chart-default tag, and deploy-camunda's neutralizeOverriddenDigests then strips the pinned image.digest because the component is overridden — leaving the image with neither tag nor digest. The image helper then rendered "<repository>:" (unquoted, trailing colon), which is invalid YAML. deploy-camunda: detect an --extra-values image override with a present-but- empty/nil tag and no digest, and fail with an actionable error instead of stripping the digest and producing broken YAML downstream. charts (8.8/8.9/8.10): imageTagByParams now coalesces a missing tag to "" instead of the "<no value>" sentinel (global.image has no tag key), which also prevents versionLabel from emitting "<no value>" as a version label. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the image override + digest-overlay interaction so CI/deploy tooling fails with a clear, actionable error when an --extra-values image override results in an image that has neither a tag nor a digest (which can otherwise render an invalid image: YAML line). It also makes imageTagByParams return an empty string instead of Helm’s <no value> sentinel when no tag is present, avoiding sentinel leakage into other helpers (e.g., version labels).
Changes:
- Add fail-fast detection in
neutralizeOverriddenDigestsfor image overrides that have an empty/nulltag and no digest. - Add unit coverage for the empty/
nulltag override scenario. - Adjust
camundaPlatform.imageTagByParamsin 8.8/8.9/8.10 to coalesce missing tags to""rather than emitting<no value>.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| scripts/deploy-camunda/deploy/digest_overlay.go | Adds early validation for empty-tag image overrides before digest pins are stripped. |
| scripts/deploy-camunda/deploy/digest_overlay_test.go | Adds tests covering tag: null and tag: "" override cases. |
| charts/camunda-platform-8.8/templates/common/_helpers.tpl | Makes imageTagByParams default missing tags to "" instead of <no value>. |
| charts/camunda-platform-8.9/templates/common/_helpers.tpl | Makes imageTagByParams default missing tags to "" instead of <no value>. |
| charts/camunda-platform-8.10/templates/common/_helpers.tpl | Makes imageTagByParams default missing tags to "" instead of <no value>. |
| // Present-but-empty tag with no digest: stripping the overlay digest | ||
| // would render a version-less "<repository>:" (invalid YAML). Flag it. | ||
| if tag, hasTag := img["tag"]; hasTag && isBlankScalar(tag) { | ||
| emptyTagOverrides = append(emptyTagOverrides, path) | ||
| return |
hisImminence
left a comment
There was a problem hiding this comment.
Review notes
Fix is correct. Two-layer defense (Go CLI + Helm helper) is the right approach. Tests cover the trigger conditions. A few things to address or acknowledge:
Stale PR description
"An earlier iteration added a hard
failguard insideimageByParams... it was dropped"
The fail guard is present in the diff (and correct — the TestContainerSetImagePullSecretsSubChart fix via camundaHub.webModeler.image.tag: "snapshot" is what resolved the websockets conflict). Update the description before merge; it'll confuse future readers of the PR history.
Not a breaking change — but add this rationale to the PR body
The imageByParams fail guard only fires when image.tag is empty AND no digest exists. That combination previously rendered repository: — invalid YAML that Helm rejected at install time anyway. No working configuration is disrupted. Per our Breaking Changes Policy this is a Bug fix, not a breaking change. State this explicitly in the PR body so reviewers don't have to reverse-engineer it.
Findings
digest_overlay_test.go: assertion strings.Contains(err.Error(), "empty tag") is a brittle string match — any rephrasing of the error silently breaks the test. Prefer a sentinel constant or typed error.
digest_overlay_test.go (new test): multi-line // TestNeutralizeOverriddenDigests... doc comment is a narration-why block. AGENTS.md: "NEVER write reasoning/why/narration comments." One-liner or move to commit message.
_helpers.tpl (8.8/8.9/8.10): TestContainerImageWithoutTagOrDigestFails was added only for 8.10. The helper change landed on all three versions — add equivalent tests for 8.8/8.9, or confirm one version's coverage is intentional given the identical helper logic.
digest_overlay.go isBlankScalar: doc says "unset" for v == "". Empty string is set, just blank. Suggest: "nil (bare 'tag:') or empty string ('tag: \"\"')".
Otherwise good to go.
Which problem does the PR fix?
A nightly
camunda-platform-8.9install (triggered from acamunda/camundamerge-queue run) failed at the Helm install step with:The failing image line rendered as
registry.camunda.cloud/team-camunda/camunda:— an unquoted reference with a trailing colon and no tag, which is invalid YAML.What's in this PR?
Root cause. The CI passed a Helm
--extra-valuesfile that redirected the orchestration imageregistry/repositoryto an internal registry but leftimage.tagempty. Helm's merge treats the explicit empty tag as a delete, removing the chart-default tag. Independently,deploy-camunda'sneutralizeOverriddenDigestsstrips the pinnedimage.digestfrom the digest overlay whenever an--extra-valuesfile overrides that component's image. The combination left the image with neither a tag nor a digest. Becauseglobal.imagehas notagkey, the chart helpercamundaPlatform.imageTagByParamsevaluatednil | default <missing>and produced the literal sentinel<no value>, ultimately rendering an invalidimage:line.Fixes:
scripts/deploy-camunda/deploy/digest_overlay.go—neutralizeOverriddenDigestsnow detects an--extra-valuesimage override with a present-but-empty/nulltagand no digest, and fails fast with an actionable error instead of stripping the digest and emitting broken YAML downstream. AddedisBlankScalarandTestNeutralizeOverriddenDigestsEmptyTag(covers bothnulland"").charts/camunda-platform-8.8|8.9|8.10/templates/common/_helpers.tpl—imageTagByParamsnow coalesces a missing tag to""(| default "") instead of the<no value>sentinel. This is the version-scoped robustness change and also preventsversionLabelfrom emitting<no value>as a version label.Scope note. The actual successful deploy still requires the upstream workflow to supply a real
tag(ordigest) fororchestration.image; this PR converts a crypticline 49YAML crash into a clear, actionable error at the correct layer. An earlier iteration added a hardfailguard insideimageByParams, but it changed chart semantics and broke the 8.10web-modelerTestContainerSetImagePullSecretsSubCharttest (the SaaScamundaHubpath legitimately renders a tagless,| quote'd websockets image); it was dropped in favor of the minimal change above.Validation: full Go unit suites pass for 8.8/8.9/8.10 (no golden changes);
deploy-camundapackage tests +go vet+gofmtclean;helm lintclean; the original empty-tag scenario now fails with--extra-values image override for [orchestration] sets registry/repository with an empty tag and no digest; set image.tag or image.digest.Checklist
Please make sure to follow our Contributing Guide.
Before opening the PR:
make go.update-golden-only.After opening the PR: