refactor(zigbee2mqtt): use alpine-jq image, drop apk add from seed-data #444
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: konairius/homelab#444
Reference in New Issue
Block a user
Applies two owner directives, and finishes removing
apk addfrom the zigbee2mqttseed-datainit container.Rename:
z2m-seed→alpine-jqCorrect — nothing in it is zigbee-specific. It is alpine plus
jq, useful to anything that needs to read JSON from a shell script. The Containerfile now describes it that way, with the zigbee2mqtt case recorded as its first consumer rather than its identity.No digest pinning
Referenced by tag instead. This collapses what was going to be two PRs into one, since the digest no longer has to exist before the Deployment can name it.
Two consequences, both improvements:
--image-name-with-digest-file/PIN THIS:apparatus is deleted. It existed only to hand a digest to a follow-up commit — and it is precisely what broke every pipeline in the repo (#443) via an unquoted": "in a YAML scalar. Less surface, same outcome.:latestmeans Kubernetes defaultsimagePullPolicytoAlways, so a rebuilt image is picked up without editing this manifest.Validation kept
A wrong
network_keymakes herdsman form a new network and drops every device, so the fail-loudly checks stay: absent key, non-hex, and length ≠ 32.jq -rprintsnullfor a missing path, so that is checked separately rather than being misreported as "non-hex" — which would send you looking at the key instead of the missing field.Verification
Ran the manifest's rendered script against the real coordinator backup and synthetic fixtures. The
alpine-jqtag does not exist until this merges and builds, so the run used the existing image of identical content (verifiedalpine 3.21.7+jq 1.7.1):network_keyline byte-identical to live (sha25627c5c77a945bcecc)network_key contains non-hex charactersnetwork_key length 4, expected 32 hex charsnetwork_key.network_key.key absent from coordinator_backup.jsonlink_keyalso in docnetwork_keyis usedThat last row is the one that matters:
"key"occurs 12 times in the real backup and the first is a devicelink_key, because"devices"sorts before"network_key".Validated with
woodpecker lint— not justyq, which is exactly what let #443 through (the unquoted form is valid YAML, it just parses as a map). Pluskustomize buildand a server-side dry run. Zero stalez2m-seedreferences remain in the tree.Cleanup needed (not doable here)
The now-orphaned
z2m-seedpackage is still in the registry and should be deleted. That needs package-delete rights this account doesn't have.🤖 Generated with Claude Code
🤖 Automated review — model
CLI default(complexity: complex), commitc3280310b5a0d0c715c04abec5a0963e2c960fdd.Summary
Renames the
z2m-seedCI image toalpine-jq(reflecting that it's a generic alpine+jq base, not zigbee-specific), switches the zigbee2mqttseed-datainit container to consume it by tag instead of digest (removing the now-unneeded digest-pinning apparatus that caused #443), and replaces the runtimeapk add --no-cache jqwith the pre-built image plus explicitnetwork_keyvalidation. I verified the rename is complete (no stalez2m-seed/"PIN THIS" references anywhere in the tree), the CI pipeline step and Containerfile path/directory line up, and the new validation logic in the init container script is correct. No blocking issues found.🟡 Suggestions
kubernetes/apps/home-assistant/zigbee2mqtt-deployment.yaml:48— usingalpine-jq:latest(vs. the dated-tag pattern already used elsewhere for other self-built, non-digest-pinned images, e.g.ci-ansible:2026-04-29inkubernetes/infrastructure/netbird-backup/cronjob.yaml:40) means a rebuild ofalpine-jqfor an unrelated consumer silently changes what this security-sensitive init container runs on its next pod restart, with no corresponding diff in this manifest to review. That's the tradeoff the PR explicitly chose (and it's a reasonable one for a thin, rarely-changing base image), just flagging it as the one place where "pick up rebuilds automatically" and "changes to this script are visible in the manifest diff" pull in different directions.🟢 Nits
None.
Tests
No automated test accompanies the new validation logic in the init container script (absent-key / non-hex / wrong-length /
link_key-before-network_keycases). The repo doesn't have a general convention of unit-testing embedded manifest shell scripts (the one script-test pipeline,.woodpecker/ci-scripts-test.yml, covers onlyscripts/ci/helper scripts, not k8s-embedded ones), so this isn't a departure from repo norms, and the PR body documents manual verification against the real coordinator backup plus synthetic fixtures for all four cases. That verification is credible but not repeatable by CI — worth keeping in mind if this script changes again.Recommendation
Looks reasonable — no blocking issues; comment above is optional.
Implemented rather than accepted as a tradeoff — you cited a precedent I had missed, and it changes the answer.
🟡
:latestvs a dated tagVerified
ci-ansible:2026-04-29inkubernetes/infrastructure/netbird-backup/cronjob.yaml, and checked the rest of the tree: for self-built images referenced by tag rather than digest, dated tags are already the convention here and:latestis used nowhere. So my "reasonable tradeoff" framing was really just an unexamined choice against existing practice.The reasoning stands on its own too. With
:latest, rebuilding this base for some unrelated consumer silently changes what theseed-datainit container runs on its next pod restart, with no diff here to review — and that container renders the Zigbee network key. "What exactly does it run" belongs in the manifest.Now
alpine-jq:2026-07-30. I did verify the tag resolves before merge rather than discovering it after:This does give up automatic pickup of rebuilds, which the previous commit had chosen the other way. For a thin, rarely-changing base feeding a security-sensitive consumer, reviewability is worth more. It also stays within the owner directive that our own images need no digest pinning — a dated tag is not a digest. The cost is remembering to bump it, which is noted at the line.
On the untested-validation point
Agreed, and worth being explicit: the four cases (absent key / non-hex / wrong length /
link_key-before-network_key) were verified by running the rendered script against the real backup and synthetic fixtures, but CI cannot reproduce that.ci-scripts-test.ymlcovers onlyscripts/ci/, so a future edit to this embedded script gets no automated safety net. Given"key"appears 12 times in that document with a device key first, that is a genuinely sharp edge to leave untested — a lint/validate gate forkubernetes/apps/**would be the real fix, and I would rather propose it as its own PR than smuggle it in here.🤖 Automated review — model
CLI default(complexity: complex), commitf66b127035cd13adfc91d89d47b829bb1437d288.Summary
Renames
z2m-seed→alpine-jq(reframed as a general-purpose alpine+jq base), drops digest pinning in favor of a plain tag, and replaces the zigbee2mqttseed-datainit container'sapk add --no-cache jqwith a build-time-installedjqplus stricternetwork_keyvalidation. The rename/digest changes are clean and the validation logic is sound, but the hardcoded image tag this PR introduces has a real self-consistency problem described below.🔴 Blocking
kubernetes/apps/home-assistant/zigbee2mqtt-deployment.yaml:53— the init container is pinned togitea.renner.uno/gitea_admin/alpine-jq:2026-07-30, but that tag does not exist anywhere yet. It will only come into existence when.woodpecker/build-ci-images.yml'sbuild-alpine-jqstep runs on push tomasterand computesTAG=$(date +%Y-%m-%d)(.woodpecker/build-ci-images.yml:155) at the moment the pipeline actually executes — not the date this string was written. If the PR merges (or the pipeline re-runs) on a different calendar day than2026-07-30in the runner's timezone — e.g. review/merge slips past midnight — the built image will be tagged with that later date, and this manifest will permanently reference a tag that is never produced, with nothing in CI to catch the mismatch.This differs from the precedent it cites (
ci-ansible:2026-04-29inkubernetes/infrastructure/netbird-backup/cronjob.yaml): that tag was pinned to a date whose image was already known to exist. Here the tag is a same-commit guess about a future build.Consequence if it does mismatch: Flux syncs
kubernetes/apps/home-assistant/on a 10-minute poll (kubernetes/clusters/homelab/home-assistant.yaml:14,interval: 10m) fully independently of the kaniko build pipeline — there is no ordering between the two.zigbee2mqttisreplicas: 1withstrategy: Recreate, so once Flux applies the new pod template it deletes the running pod before creating the replacement; if the new init container's image can't be pulled, the sole Zigbee coordinator sits inImagePullBackOff, taking down the whole Zigbee stack until someone notices and manually corrects the tag. In the common case (merge same day, build finishes before the next Flux poll) this is at most a brief, self-healing hiccup — but as written there is nothing that guarantees that.Suggested fix: either confirm the actual built tag after the image pipeline runs and update this line to match (rather than pre-guessing it in the same commit), or land the two changes as separate commits/PRs so the tag is only written once it is known to exist — consistent with how
ci-ansible:2026-04-29was actually pinned.🟡 Suggestions
kubernetes/apps/home-assistant/zigbee2mqtt-deployment.yaml:66—KEY=$(jq -r '.network_key.key' ...)runs underset -eu. If.network_keyitself is absent (rather than merely.network_key.key),jqerrors ("Cannot index null with string key") and the script aborts on that line viaset -e, surfacing jq's raw error rather than the intendedFATAL: .network_key.key absent from coordinator_backup.jsonmessage at line 73. The explicit "absent" check only catches the case where the full path resolves tonull, not where an intermediate key is missing entirely. Both paths fail loudly (which is what matters), but the messaging described in the PR's verification table ("absentnetwork_key" → the custom FATAL line) doesn't hold for every shape of "absent."🟢 Nits
None beyond the above.
Tests
No automated tests apply here (this is GitOps config, not application code with a test harness), and the PR body documents manual verification against a real coordinator backup and synthetic fixtures for the validation branches, plus
woodpecker lint,kustomize build, and a server-side dry run — reasonable diligence for this repo's conventions. The one thing that wasn't and can't be verified pre-merge is the actual point raised above: that the hardcoded tag will match what the build pipeline produces.Recommendation
Request changes — 1 blocking issue.
Addressed in
6e4729c7. One of the two findings holds and is now fixed; the blocking one rests on a premise I can disprove by measurement.🔴 "that tag does not exist anywhere yet" — it does
gitea.renner.uno/gitea_admin/alpine-jq:2026-07-30is in the registry and pullable now. Verified by actually pulling it, twice, and the latest test run below executes against that tag directly:The missing piece from the sandbox:
build-alpine-jqfires onevent: pushwith no branch restriction, so pipeline 3060 built and pushed it when this branch was pushed — well before merge. So the tag is not a guess about a future build; it is an existing artifact, which is exactly the state yourci-ansible:2026-04-29comparison asks for.That also closes the consequence you traced (Flux 10m poll, no ordering against kaniko,
replicas: 1+Recreate→ sole coordinator inImagePullBackOff). That analysis is correct and worth having on record — it just cannot trigger here, because the pull already succeeds.The residual caution is real though, so it is now documented at the line:
TAGcomes fromdateat run time, so a date must never be written here before a build has produced it; and if a later master build runs on a different day it publishes that day's tag without deleting this one, so this keeps resolving — though the image then came from a branch build rather than master, which is worth re-pointing when convenient.🟡 jq under
set -e— real gap, different triggerYou were right that a shape of "absent" bypasses the intended message, but not the shape described. Measured in the image:
{"devices":[]}"null", rc=0{"network_key":{}}"null", rc=0{"network_key":null}"null", rc=0{"network_key":"abc"}Cannot index string with string "key"{"network_key":123}Cannot index number with string "key"jq is lenient about a missing path, so
.network_keybeing absent already produced the correct FATAL line. What broke wasnetwork_keyexisting but not being an object — there.keyexits 5 andset -eaborts before any check runs. Fixed with.network_key.key?, which yields empty for those shapes and routes them into the existing absent branch.All shapes now report identically:
Thanks for pushing on this one — the non-object case would have produced a confusing raw jq error at exactly the worst moment.
🤖 Automated review — model
CLI default(complexity: complex), commit6e4729c7d04406d9b9338d6c3ad52617e5b54928.Summary
Renames the
z2m-seedCI image toalpine-jq(reflecting what it is, not who uses it), drops the digest-pinning apparatus in favor of a plain tag reference, and finishes replacing the runtimeapk add --no-cache jqin the zigbee2mqttseed-datainit container with the pre-built image plus a hardenednetwork_keyextraction. I verified the two owner directives cited in the PR body against the actual files, confirmed no stalez2m-seedreferences remain, and checked the jq/shell logic by hand against the malformed-shape cases the PR claims to handle. No blocking issues found.🟡 Suggestions
kubernetes/apps/home-assistant/zigbee2mqtt-deployment.yaml:67— the whole design rests ongitea.renner.uno/gitea_admin/alpine-jq:2026-07-30already existing in the registry (built from this same branch push, per the comment). I have no access to the registry or CI run status from this checkout, so I could not independently confirm the tag is actually pullable. Givenreplicas: 1+strategy: Recreate, an unpullable init image would leave the sole Zigbee coordinator down. Worth a final registry check (or a greenbuild-alpine-jqpipeline run for this exact SHA) immediately before merge, since Flux could reconcile this manifest as soon as it lands regardless of build timing.kubernetes/apps/home-assistant/zigbee2mqtt-deployment.yaml:87—.network_key.key?only shields the leaf.keyselector; if/backup/coordinator_backup.json's top level were ever something other than a JSON object (e.g. an array),.network_keyitself would raise an uncaught jq error before?applies, andset -euwould abort with jq's raw error instead of one of the tailoredFATAL:messages. Not a realistic shape for an Open Coordinator Backup export, and the script still fails closed either way, so this is a message-quality gap, not a correctness issue.Tests
No automated test accompanies the shell/jq logic change, but this matches the repo's existing convention for Kubernetes manifests (no CI gate renders or executes init-container scripts —
.woodpecker/ci-scripts-test.ymlonly coversscripts/ci/*, unrelated to this path). The PR body documents manual verification of all five cases (real backup, non-hex key, wrong length, absent key, and the device-link_key-precedes-network_keyordering case) against the rendered script using the real image contents (alpine 3.21.7+jq 1.7.1). I re-derived the same jq/shell behavior independently by hand-tracing each malformed-shape case through.network_key.key? // ""and the subsequentnull/empty/hex/length checks, and it holds together: every case the PR claims is rejected is in fact rejected, and the one valid case passes through unmodified.Recommendation
Looks reasonable — no blocking issues; comments above are optional.
Both suggestions were marked optional; both were cheap enough to close properly rather than acknowledge. Done in
8439a99a.🟡
?only shielded the leaf selectorYou were right, and this was the last remaining path that produced jq's raw error instead of a tailored message. Measured in the image:
?now sits on both selectors. Agreed it is not a realistic shape for an Open Coordinator Backup export and it failed closed either way — but the whole point of these guards is that whoever hits them is already having a bad day, so the message should not beCannot index array with string.Full matrix against the rendered script, run on the
alpine-jq:2026-07-30tag:🟡 Registry check before merge
Done, just now:
On your point about a green
build-alpine-jqfor this exact SHA: there deliberately is not one, and that is correct rather than a gap. The tag was published by pipeline 3060, the push build for this branch. The commits after it touch onlykubernetes/apps/**, which does not match the build step'swhen.path— so no rebuild fires, and the image content is unchanged by them. Rebuilding on every manifest tweak is exactly the waste that the per-path gating in this pipeline exists to prevent (the #336 note).Thank you for holding the line on the registry question across two rounds. The blocking version of it was based on a premise I could disprove, but the concern behind it was sound, and the pre-merge check is a reasonable thing to have insisted on given
replicas: 1+Recreateon the sole Zigbee coordinator.🤖 Automated review — model
CLI default(complexity: complex), commit8439a99a98c36c166d6a7155cb681f53b17e9fad.Summary
Renames the
z2m-seedCI image toalpine-jq(per owner directive), drops digest-pinning for the repo's own images in favor of a dated tag (per owner directive), and finishes moving the zigbee2mqttseed-datainit container offapk add --no-cache jqonto the built image, adding fail-loud validation of the extractednetwork_key. I verified the rename left no stalez2m-seedreferences, confirmed the dated-tag precedent already exists elsewhere in the repo, and independently re-ran thejqextraction against jq to check the validation logic's claimed behavior. No blocking issues found.🟡 Suggestions
kubernetes/apps/home-assistant/zigbee2mqtt-deployment.yaml:83-89— I ran the actual expression (jq 1.7.1, matching the image) against all the shapes named in the comment. The full expression(.network_key?.key? // "")returns an empty string for{},{"devices":[]},{"network_key":{}}, and{"network_key":null}— not the literal text"null"that line 84 claims. Line 89 itself says the same sub-expression "returns empty for all of them," which contradicts line 84 two lines above it. This doesn't affect correctness — theif [ "$KEY" = "null" ] || [ -z "$KEY" ]check on line 96 catches both outcomes either way — but the comment is internally inconsistent and could confuse a future reader trying to reason from it. (I confirmed separately that.network_key?.key?without the// ""fallback does print literalnullfor the null-parent cases, which is presumably where the "null" framing in the comment came from — it describes an intermediate expression, not the one actually run.)🟢 Nits
8439a99, "fix(zigbee2mqtt): guard the network_key selector at both levels") describes only the validation-hardening part of this change. The PR itself bundles in thez2m-seed→alpine-jqrename, the digest→tag switch, and the CI pipeline edits — none of which the commit message mentions. Not a functional problem, just a mismatch worth knowing ifgit logis ever used to reconstruct why the rename happened.alpine-jq:2026-07-30) trades convenience for a real operational sharp edge: if this doesn't merge same-day, or a later master build republishes under a different date, this manifest keeps resolving against a branch-built (not master-built) image until someone manually re-points it (the PR body already flags this explicitly as a caveat, so I'm not raising anything new here — just noting it's the one place in this PR where correctness depends on an out-of-repo fact, i.e., that the tag was actually pushed, which I can't verify from the checkout alone).Tests
No automated tests exist for this class of change in the repo (shell logic embedded in a Kubernetes manifest, and CI pipeline YAML) — consistent with how the rest of this repo handles infra/GitOps config. The PR body documents manual verification against a real coordinator backup plus synthetic fixtures for every branch (absent key, non-hex, wrong length, device-key collision). I independently reproduced the core claims by running the actual
jq -r '(.network_key?.key? // "")'expression against jq for all nine listed input shapes plus a valid key, and it behaves correctly in every case (see the one wording nit above). I did not have access to the actual coordinator backup or the builtalpine-jqimage/registry, so I could not verify the "byte-identical to live" claim or that thealpine-jq:2026-07-30tag actually exists in the registry — those require registry/production access outside this checkout.Recommendation
Looks reasonable — no blocking issues; comments above are optional.