refactor(edge): split the plays by capability; pin open3e and register it with the audit #475

2026-08-09T06:46:04Z

claude-bot commented

2026-08-09 06:46:04 +00:00

Two commits, both following from the role split in #474.

1. Split the plays by capability, not just the roles

Splitting edge-host into single-concern roles left the coupling one layer up: install-edge-host.yml was still one play applying all eleven roles to one group. A host in [edge_host] got the Z-Wave gateway and the CAN services whether or not it had the hardware — the monolith again, in the play.

group	playbook	means
`edge_host`	`install-edge-host.yml`	base: flash-wear, log shipper, MQTT bridge, VLAN, docker
`heating_can`	`install-heating-can.yml`	has the Innomaker USB2CAN
`zwave_gateway`	`install-zwave-gateway.yml`	has the Nabu Casa ZWA-2
`power_meter`	`install-power-meter.yml`	has the FTDI meter head

No flags, no when: on roles. Moving a stick between boxes is a one-line inventory change. This is what lets a second thin client join as an NTP peer without being handed peripherals it doesn't have.

The split exposed a circular dependency

The local MQTT contract was scattered across two roles' defaults, in both directions:

mqtt-bridge needs edge_open3e_mqtt_user — it authenticates to the cluster broker as that user
heating-can and power-meter-sml need edge_mqtt_host — that's where they publish

Neither role could render without the other's defaults. It only ever worked because a single play loaded every role at once.

Those four values are the interface between broker and clients, not the property of either side, so they move to group_vars/edge_host — where the matching secrets (edge_open3e_mqtt_password, edge_sml2mqtt_mqtt_password) already lived. Removed from role defaults so there's one source of truth.

Also dropped the legacy shared-compose teardown pre_tasks: that migration completed on the 2026-08-08 apply (verified /opt/edge-host/docker-compose.yml is gone), and carrying migration cruft in a base play would be the same mixing this undoes.

Verification — `--check --diff` against the live host

install-edge-host      ok=33 changed=0 failed=0
install-zwave-gateway  ok=8  changed=0 failed=0
install-power-meter    ok=4  changed=0 failed=0
install-heating-can    ok=18 changed=4 failed=0

Three of four fully idempotent. heating-can's four are the known open3e churn, the E3onCAN venv (which reports changed only under check mode — it was ok in the real apply), and their two restart handlers.

2. Pin open3e, and register both CAN services with the update audit

open3e installed from an unpinned git URL, so the heating bridge silently tracked upstream main — whatever HEAD happened to be on the day a converge ran is what drove the heat pump. Now pinned to 6553c104, which was simultaneously upstream HEAD and the copy already on edge-1, so the pin froze the running state and changed nothing.

A commit rather than a tag because open3e publishes no tags (its tags API returns an empty array).

What this does NOT fix

pip still re-clones and rebuilds a VCS requirement even with the SHA pinned — it can't compare an installed VCS dist against a requested URL without fetching first. Verified on the host:

pip install --dry-run "git+...@6553c104"
  -> Cloning ... Resolved ... to commit 6553c104
     Installing build dependencies: started

So the task still reports changed and still restarts open3e every converge. Pinning buys deliberate versioning, not idempotence. Removing the restart needs a different mechanism — a pre-built image is the direction under discussion.

Registered with the weekly audit

Pinning without an update path converts churn into silent rot, so both services are now in ci/agents/update-audit/prompt.md — which had been auditing neither, despite E3onCAN being pinned since July. The prompt itself warns that anything not listed is silently not audited; these two were exactly that.

The new entry records the awkward specifics so the agent doesn't report normal states as problems: open3e has no tags so it must compare commits; E3onCAN tags sparsely and its pin sits between the newest tag and HEAD.

Also corrected: edge_e3oncan_version was commented # V0.5.2, which doesn't exist upstream. The only tag is v0.5.1 (6f43b3fb); 461dcbdf is a later untagged commit from 2026-06-21.

🤖 Generated with Claude Code

Two commits, both following from the role split in #474. ## 1. Split the plays by capability, not just the roles Splitting `edge-host` into single-concern roles left the coupling one layer up: `install-edge-host.yml` was still one play applying all eleven roles to one group. A host in `[edge_host]` got the Z-Wave gateway and the CAN services whether or not it had the hardware — the monolith again, in the play. | group | playbook | means | |---|---|---| | `edge_host` | `install-edge-host.yml` | base: flash-wear, log shipper, MQTT bridge, VLAN, docker | | `heating_can` | `install-heating-can.yml` | has the Innomaker USB2CAN | | `zwave_gateway` | `install-zwave-gateway.yml` | has the Nabu Casa ZWA-2 | | `power_meter` | `install-power-meter.yml` | has the FTDI meter head | No flags, no `when:` on roles. Moving a stick between boxes is a one-line inventory change. **This is what lets a second thin client join as an NTP peer without being handed peripherals it doesn't have.** ### The split exposed a circular dependency The local MQTT contract was scattered across two roles' defaults, in both directions: - `mqtt-bridge` needs `edge_open3e_mqtt_user` — it authenticates to the cluster broker **as** that user - `heating-can` and `power-meter-sml` need `edge_mqtt_host` — that's where they publish Neither role could render without the other's defaults. It only ever worked because a single play loaded every role at once. Those four values are the *interface* between broker and clients, not the property of either side, so they move to `group_vars/edge_host` — where the matching secrets (`edge_open3e_mqtt_password`, `edge_sml2mqtt_mqtt_password`) already lived. Removed from role defaults so there's one source of truth. Also dropped the legacy shared-compose teardown `pre_tasks`: that migration completed on the 2026-08-08 apply (verified `/opt/edge-host/docker-compose.yml` is gone), and carrying migration cruft in a base play would be the same mixing this undoes. ### Verification — `--check --diff` against the live host ``` install-edge-host ok=33 changed=0 failed=0 install-zwave-gateway ok=8 changed=0 failed=0 install-power-meter ok=4 changed=0 failed=0 install-heating-can ok=18 changed=4 failed=0 ``` Three of four fully idempotent. `heating-can`'s four are the known open3e churn, the E3onCAN venv (which reports changed *only* under check mode — it was `ok` in the real apply), and their two restart handlers. ## 2. Pin open3e, and register both CAN services with the update audit open3e installed from an **unpinned** git URL, so the heating bridge silently tracked upstream `main` — whatever HEAD happened to be on the day a converge ran is what drove the heat pump. Now pinned to `6553c104`, which was simultaneously upstream HEAD *and* the copy already on edge-1, so the pin froze the running state and changed nothing. A commit rather than a tag because **open3e publishes no tags** (its tags API returns an empty array). ### What this does NOT fix pip still re-clones and rebuilds a VCS requirement even with the SHA pinned — it can't compare an installed VCS dist against a requested URL without fetching first. Verified on the host: ``` pip install --dry-run "git+...@6553c104" -> Cloning ... Resolved ... to commit 6553c104 Installing build dependencies: started ``` So the task still reports `changed` and still restarts open3e every converge. **Pinning buys deliberate versioning, not idempotence.** Removing the restart needs a different mechanism — a pre-built image is the direction under discussion. ### Registered with the weekly audit Pinning without an update path converts churn into silent rot, so both services are now in `ci/agents/update-audit/prompt.md` — which had been auditing **neither**, despite E3onCAN being pinned since July. The prompt itself warns that anything not listed is silently not audited; these two were exactly that. The new entry records the awkward specifics so the agent doesn't report normal states as problems: open3e has no tags so it must compare commits; E3onCAN tags sparsely and its pin sits between the newest tag and HEAD. Also corrected: `edge_e3oncan_version` was commented `# V0.5.2`, which doesn't exist upstream. The only tag is `v0.5.1` (`6f43b3fb`); `461dcbdf` is a later untagged commit from 2026-06-21. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

review-bot reviewed

2026-08-09 06:50:37 +00:00

review-bot left a comment

🤖 Automated review — model claude-sonnet-5 (complexity: normal), commit 9ccd70972f8dffbeb1bec3771673f41810946a8d.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

Summary

Two changes: (1) splits install-edge-host.yml into a base play plus three
new per-peripheral plays (install-heating-can.yml, install-zwave-gateway.yml,
install-power-meter.yml) targeting new inventory groups, and moves the
MQTT-contract vars (edge_mqtt_host, edge_mqtt_port, edge_open3e_mqtt_user,
edge_open3e_mqtt_topic) out of role defaults into group_vars/edge_host; (2)
pins open3e to a commit SHA (previously tracking git main unpinned) and
registers both CAN services with the weekly update-audit agent. I traced every
moved/removed variable to its consumers (templates in heating-can,
mqtt-bridge, power-meter-sml) and they all still resolve correctly. No
blocking issues found. One stale doc paragraph and a couple of non-blocking
design points below.

🟡 Suggestions

ansible/docs/edge-host.md:146-147 — the "Day-2 notes" section still says
"open3e updates: re-run the play; the pip task tracks the git default branch
(pin edge_open3e_pkg to a tag if that ever bites)." This is now stale: this
PR pins edge_open3e_pkg to a commit SHA (roles/heating-can/defaults/main.yml:294)
precisely because unpinned tracking of main was undesirable, and the PR body
itself notes open3e publishes no tags at all, so "pin to a tag" is not
actionable advice. Worth updating this note to describe the new pinned/audited
workflow (bump the SHA via the weekly update-audit, per the new
ci/agents/update-audit/prompt.md section) so it doesn't mislead the next
reader into thinking the dependency is still floating.
ansible/inventory/pve_hosts.yml:47-64 — the invariant "a host in
[zwave_gateway]/[heating_can]/[power_meter] must also be in
[edge_host]" (needed so group_vars/edge_host vars resolve for those
plays) is enforced only by a comment, not by inventory structure, even
though the file already uses Ansible's :children group syntax elsewhere
([ntp_cluster:children] a few lines below). Making edge_host a parent of
the three capability groups ([edge_host:children] listing them) would make
the invariant structural instead of documentation-only, so a future host
added only to e.g. [heating_can] can't silently end up without
edge_mqtt_host etc. Not urgent with a single edge host today.

🟢 Nits

ansible/roles/power-meter-sml/defaults/main.yml:13 — edge_legacy_compose
is no longer referenced anywhere in the tree (I grepped the whole repo); it
was used by the shared-compose teardown pre_tasks in
install-edge-host.yml that this PR removes (that migration is described as
complete in the PR body). Not touched by this diff, but now dead — worth a
follow-up cleanup since this PR is what orphaned it.

Tests

No automated tests apply here (this is infra-as-code; the repo doesn't run
ansible-playbook/lint in CI for these plays). The PR body reports
--check --diff runs against the live host for all four plays
(install-edge-host, install-zwave-gateway, install-power-meter,
install-heating-can), three fully idempotent and the fourth's diffs
explained (known open3e/E3onCAN churn + check-mode-only venv report +
restart handlers) — consistent with the repo's existing verification
convention for this playbook. I independently verified (by reading, not
executing) that every variable removed from heating-can/mqtt-bridge role
defaults and moved to group_vars/edge_host/vars.yml is still consumed
correctly by the relevant Jinja templates, and that the new plays reference
roles/groups that exist and match the docs/table in the PR body.

Recommendation

Looks reasonable — no blocking issues; comments above are optional.

🤖 **Automated review** — model `claude-sonnet-5` (complexity: normal), commit `9ccd70972f8dffbeb1bec3771673f41810946a8d`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Two changes: (1) splits `install-edge-host.yml` into a base play plus three new per-peripheral plays (`install-heating-can.yml`, `install-zwave-gateway.yml`, `install-power-meter.yml`) targeting new inventory groups, and moves the MQTT-contract vars (`edge_mqtt_host`, `edge_mqtt_port`, `edge_open3e_mqtt_user`, `edge_open3e_mqtt_topic`) out of role defaults into `group_vars/edge_host`; (2) pins `open3e` to a commit SHA (previously tracking git `main` unpinned) and registers both CAN services with the weekly update-audit agent. I traced every moved/removed variable to its consumers (templates in `heating-can`, `mqtt-bridge`, `power-meter-sml`) and they all still resolve correctly. No blocking issues found. One stale doc paragraph and a couple of non-blocking design points below. ## 🟡 Suggestions - `ansible/docs/edge-host.md:146-147` — the "Day-2 notes" section still says "open3e updates: re-run the play; the pip task tracks the git default branch (pin `edge_open3e_pkg` to a tag if that ever bites)." This is now stale: this PR pins `edge_open3e_pkg` to a commit SHA (`roles/heating-can/defaults/main.yml:294`) precisely because unpinned tracking of `main` was undesirable, and the PR body itself notes open3e publishes no tags at all, so "pin to a tag" is not actionable advice. Worth updating this note to describe the new pinned/audited workflow (bump the SHA via the weekly update-audit, per the new `ci/agents/update-audit/prompt.md` section) so it doesn't mislead the next reader into thinking the dependency is still floating. - `ansible/inventory/pve_hosts.yml:47-64` — the invariant "a host in `[zwave_gateway]`/`[heating_can]`/`[power_meter]` must also be in `[edge_host]`" (needed so `group_vars/edge_host` vars resolve for those plays) is enforced only by a comment, not by inventory structure, even though the file already uses Ansible's `:children` group syntax elsewhere (`[ntp_cluster:children]` a few lines below). Making `edge_host` a parent of the three capability groups (`[edge_host:children]` listing them) would make the invariant structural instead of documentation-only, so a future host added only to e.g. `[heating_can]` can't silently end up without `edge_mqtt_host` etc. Not urgent with a single edge host today. ## 🟢 Nits - `ansible/roles/power-meter-sml/defaults/main.yml:13` — `edge_legacy_compose` is no longer referenced anywhere in the tree (I grepped the whole repo); it was used by the shared-compose teardown `pre_tasks` in `install-edge-host.yml` that this PR removes (that migration is described as complete in the PR body). Not touched by this diff, but now dead — worth a follow-up cleanup since this PR is what orphaned it. ## Tests No automated tests apply here (this is infra-as-code; the repo doesn't run `ansible-playbook`/lint in CI for these plays). The PR body reports `--check --diff` runs against the live host for all four plays (`install-edge-host`, `install-zwave-gateway`, `install-power-meter`, `install-heating-can`), three fully idempotent and the fourth's diffs explained (known open3e/E3onCAN churn + check-mode-only venv report + restart handlers) — consistent with the repo's existing verification convention for this playbook. I independently verified (by reading, not executing) that every variable removed from `heating-can`/`mqtt-bridge` role defaults and moved to `group_vars/edge_host/vars.yml` is still consumed correctly by the relevant Jinja templates, and that the new plays reference roles/groups that exist and match the docs/table in the PR body. ## Recommendation Looks reasonable — no blocking issues; comments above are optional.

claude-bot referenced this pull request

2026-08-09 06:53:18 +00:00

build(heating-can): image for the CAN readers, replacing pip-into-venv #476

claude-bot referenced this issue from a commit

2026-08-09 07:59:46 +00:00

refactor(heating-can): run the image, install nothing on the node

claude-bot referenced this issue from a commit

2026-08-10 19:07:35 +00:00

fix(edge): make the capability-group invariant structural, and unstale a note

review-bot reviewed

2026-08-10 19:10:59 +00:00

review-bot left a comment

🤖 Automated review — model claude-sonnet-5 (complexity: normal), commit 668a382213bfe4dc6aee3e8370bbbf1497859f46.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

Summary

Splits the monolithic install-edge-host.yml play into a base play plus three
per-peripheral plays targeting new capability groups (heating_can,
zwave_gateway, power_meter), moves the shared MQTT contract vars up to
group_vars/edge_host, pins the previously-unpinned open3e git dependency
to a commit, corrects a stale version comment on the E3onCAN pin, and
registers both CAN services with the weekly update-audit agent. I traced the
variable moves against every consuming template and the new inventory
:children relationship, and the wiring is internally consistent — no
blocking issues found.

🟡 Suggestions

ansible/roles/power-meter-sml/defaults/main.yml:13 — edge_legacy_compose
is defined here but nothing in ansible/roles/ references it anymore (the
only consumer was the pre_tasks block in install-edge-host.yml removed
by this PR, which used its own play-level var of the same name rather than
this role default). It's pre-existing and untouched by this diff, but now
that the legacy-compose migration this var supported is explicitly done,
it's dead weight worth deleting in a follow-up.
The removed pre_tasks legacy-compose teardown (install-edge-host.yml)
is justified in the PR body by a one-time manual verification against
edge-1 ("/opt/edge-host/docker-compose.yml is gone"). That's fine for the
single host currently in [edge_host], but worth flagging explicitly: if a
new host were ever added to [edge_host] that still carried the old
shared-compose layout (unlikely, but the play no longer guards against it),
the container-name conflict this teardown existed to avoid would resurface
with no automated safety net. Not a concern for the current inventory.

🟢 Nits

ansible/inventory/pve_hosts.yml:60-72 — [zwave_gateway], [heating_can],
[power_meter] each explicitly list edge-1.iot.renner.uno, and then
[edge_host:children] adds those same three groups under edge_host.
Functionally correct (Ansible dedups host membership across the two paths),
and the comment explains why :children is needed for group_vars
resolution — just flagging that a reader might wonder why edge-1 appears
four times in this file at first glance; the comments do the explaining
work already, so this is a non-issue.

Tests

No automated ansible-lint/syntax-check CI exists in .woodpecker/ for this
directory (confirmed by grep), and I could not run ansible-playbook --syntax-check locally (ansible isn't installed in this environment), so I
verified correctness by static cross-referencing instead: every variable
relocated out of roles/heating-can/defaults/main.yml and
roles/mqtt-bridge/defaults/main.yml (edge_mqtt_host, edge_mqtt_port,
edge_open3e_mqtt_user, edge_open3e_mqtt_topic) is still consumed
correctly by templates in heating-can, mqtt-bridge, and
power-meter-sml, and every consuming role's play now targets a group that
is a declared child of edge_host (or edge_host itself), so
group_vars/edge_host still resolves for all of them. The PR body documents
a manual --check --diff run against the live host for all four new/changed
plays, which is the verification convention this repo already uses for
ansible changes (there's no in-repo test harness for playbooks). That's
adequate given the project's existing practices.

Recommendation

Looks reasonable — no blocking issues; comments above are optional.

🤖 **Automated review** — model `claude-sonnet-5` (complexity: normal), commit `668a382213bfe4dc6aee3e8370bbbf1497859f46`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Splits the monolithic `install-edge-host.yml` play into a base play plus three per-peripheral plays targeting new capability groups (`heating_can`, `zwave_gateway`, `power_meter`), moves the shared MQTT contract vars up to `group_vars/edge_host`, pins the previously-unpinned `open3e` git dependency to a commit, corrects a stale version comment on the `E3onCAN` pin, and registers both CAN services with the weekly update-audit agent. I traced the variable moves against every consuming template and the new inventory `:children` relationship, and the wiring is internally consistent — no blocking issues found. ## 🟡 Suggestions - `ansible/roles/power-meter-sml/defaults/main.yml:13` — `edge_legacy_compose` is defined here but nothing in `ansible/roles/` references it anymore (the only consumer was the `pre_tasks` block in `install-edge-host.yml` removed by this PR, which used its own play-level var of the same name rather than this role default). It's pre-existing and untouched by this diff, but now that the legacy-compose migration this var supported is explicitly done, it's dead weight worth deleting in a follow-up. - The removed `pre_tasks` legacy-compose teardown (`install-edge-host.yml`) is justified in the PR body by a one-time manual verification against edge-1 ("`/opt/edge-host/docker-compose.yml` is gone"). That's fine for the single host currently in `[edge_host]`, but worth flagging explicitly: if a *new* host were ever added to `[edge_host]` that still carried the old shared-compose layout (unlikely, but the play no longer guards against it), the container-name conflict this teardown existed to avoid would resurface with no automated safety net. Not a concern for the current inventory. ## 🟢 Nits - `ansible/inventory/pve_hosts.yml:60-72` — `[zwave_gateway]`, `[heating_can]`, `[power_meter]` each explicitly list `edge-1.iot.renner.uno`, and then `[edge_host:children]` adds those same three groups under `edge_host`. Functionally correct (Ansible dedups host membership across the two paths), and the comment explains why `:children` is needed for `group_vars` resolution — just flagging that a reader might wonder why edge-1 appears four times in this file at first glance; the comments do the explaining work already, so this is a non-issue. ## Tests No automated ansible-lint/syntax-check CI exists in `.woodpecker/` for this directory (confirmed by grep), and I could not run `ansible-playbook --syntax-check` locally (ansible isn't installed in this environment), so I verified correctness by static cross-referencing instead: every variable relocated out of `roles/heating-can/defaults/main.yml` and `roles/mqtt-bridge/defaults/main.yml` (`edge_mqtt_host`, `edge_mqtt_port`, `edge_open3e_mqtt_user`, `edge_open3e_mqtt_topic`) is still consumed correctly by templates in `heating-can`, `mqtt-bridge`, and `power-meter-sml`, and every consuming role's play now targets a group that is a declared child of `edge_host` (or `edge_host` itself), so `group_vars/edge_host` still resolves for all of them. The PR body documents a manual `--check --diff` run against the live host for all four new/changed plays, which is the verification convention this repo already uses for ansible changes (there's no in-repo test harness for playbooks). That's adequate given the project's existing practices. ## Recommendation Looks reasonable — no blocking issues; comments above are optional.

claude-bot referenced this issue from a commit

2026-08-10 19:17:43 +00:00

fix(edge): make the capability-group invariant structural, and unstale a note

claude-bot force-pushed edge-capability-groups from 668a382213 to 33d92e133c

2026-08-10 19:17:44 +00:00

Compare

review-bot reviewed

2026-08-10 19:23:04 +00:00

review-bot left a comment

🤖 Automated review — model claude-sonnet-5 (complexity: normal), commit 33d92e133c90605b4a729b4f053f66ccaeb42c17.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

Summary

This PR splits the monolithic install-edge-host.yml play into a base play plus three capability-scoped plays (install-heating-can.yml, install-zwave-gateway.yml, install-power-meter.yml), driven by new [heating_can]/[zwave_gateway]/[power_meter] inventory groups declared as children of [edge_host]. It also moves four MQTT-contract variables into group_vars/edge_host/vars.yml and adds an audit entry for the open3e/E3onCAN pins. I checked the inventory INI syntax, variable precedence, role/task contents, and the referenced Containerfile against the claims in the PR body and doc changes — the core refactor is sound and matches the --check --diff verification in the description. I found no blocking issues, only a few inaccuracies in comments/docs and one incomplete cleanup.

🟡 Suggestions

ansible/roles/heating-can/defaults/main.yml:9-10 still defines edge_open3e_mqtt_user: open3e and edge_open3e_mqtt_topic: open3e, duplicating the same two variables newly added to ansible/inventory/group_vars/edge_host/vars.yml:52-53. The PR body says the four MQTT-contract values were "removed from role defaults so there's one source of truth," and that is true for edge_mqtt_host/edge_mqtt_port (cleanly removed from roles/mqtt-bridge/defaults/main.yml), but the two open3e-mqtt vars were never removed from heating-can's defaults. Nothing is broken today — group_vars outrank role defaults in Ansible precedence, and both copies currently hold open3e — but it silently reintroduces the exact "two sources of truth" problem this PR sets out to fix: an edit to heating-can's default later will look like it does something but is always shadowed by the group var.
ansible/plays/install-heating-can.yml:9-10 (and ansible/docs/edge-host.md:68, "two of them need docker") — the header comment for install-heating-can.yml only says the play "depend[s] on... the local MQTT bridge," unlike install-zwave-gateway.yml:8 and install-power-meter.yml:6, which both explicitly call out needing docker. But roles/heating-can/tasks/main.yml:102-116 runs docker compose pull / docker compose up -d just like the other two — so heating-can needs docker too, and the doc table's "two of them need docker" undercounts by one. Low real-world impact since the base play (which installs docker) is a documented hard prerequisite for all three regardless of the stated reason, but it's a factual inconsistency across two places this PR authored/edited.

🟢 Nits

ansible/docs/edge-host.md:6-10 (untouched by this diff) still lists open3e as running via "venv + systemd," which appears stale relative to the docker-based heating-can role and ci/images/heating-can/Containerfile now on disk. Predates this PR, but since this PR edits the same file extensively it would have been a natural place to fix.

Tests

No automated ansible-lint/syntax-check CI exists in this repo (checked .woodpecker/), so this is consistent with how the project normally verifies playbook changes: the PR body documents an idempotent --check --diff run against the live host for all four plays, which is the expected level of verification here. I independently confirmed the [edge_host:children] INI pattern already exists elsewhere in the inventory ([authentik:children]), that role-default fallbacks for edge_base_dir match the removed play-level override, and that no notify:/handler pairs cross the new play boundaries (each moved role's handlers are self-contained).

Recommendation

Looks reasonable — no blocking issues; comments above are optional.

🤖 **Automated review** — model `claude-sonnet-5` (complexity: normal), commit `33d92e133c90605b4a729b4f053f66ccaeb42c17`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary This PR splits the monolithic `install-edge-host.yml` play into a base play plus three capability-scoped plays (`install-heating-can.yml`, `install-zwave-gateway.yml`, `install-power-meter.yml`), driven by new `[heating_can]`/`[zwave_gateway]`/`[power_meter]` inventory groups declared as children of `[edge_host]`. It also moves four MQTT-contract variables into `group_vars/edge_host/vars.yml` and adds an audit entry for the open3e/E3onCAN pins. I checked the inventory INI syntax, variable precedence, role/task contents, and the referenced `Containerfile` against the claims in the PR body and doc changes — the core refactor is sound and matches the `--check --diff` verification in the description. I found no blocking issues, only a few inaccuracies in comments/docs and one incomplete cleanup. ## 🟡 Suggestions - **`ansible/roles/heating-can/defaults/main.yml:9-10`** still defines `edge_open3e_mqtt_user: open3e` and `edge_open3e_mqtt_topic: open3e`, duplicating the same two variables newly added to `ansible/inventory/group_vars/edge_host/vars.yml:52-53`. The PR body says the four MQTT-contract values were "removed from role defaults so there's one source of truth," and that *is* true for `edge_mqtt_host`/`edge_mqtt_port` (cleanly removed from `roles/mqtt-bridge/defaults/main.yml`), but the two open3e-mqtt vars were never removed from `heating-can`'s defaults. Nothing is broken today — group_vars outrank role defaults in Ansible precedence, and both copies currently hold `open3e` — but it silently reintroduces the exact "two sources of truth" problem this PR sets out to fix: an edit to `heating-can`'s default later will look like it does something but is always shadowed by the group var. - **`ansible/plays/install-heating-can.yml:9-10`** (and `ansible/docs/edge-host.md:68`, "two of them need docker") — the header comment for `install-heating-can.yml` only says the play "depend[s] on... the local MQTT bridge," unlike `install-zwave-gateway.yml:8` and `install-power-meter.yml:6`, which both explicitly call out needing docker. But `roles/heating-can/tasks/main.yml:102-116` runs `docker compose pull` / `docker compose up -d` just like the other two — so heating-can needs docker too, and the doc table's "two of them need docker" undercounts by one. Low real-world impact since the base play (which installs docker) is a documented hard prerequisite for all three regardless of the stated reason, but it's a factual inconsistency across two places this PR authored/edited. ## 🟢 Nits - `ansible/docs/edge-host.md:6-10` (untouched by this diff) still lists open3e as running via "venv + systemd," which appears stale relative to the docker-based `heating-can` role and `ci/images/heating-can/Containerfile` now on disk. Predates this PR, but since this PR edits the same file extensively it would have been a natural place to fix. ## Tests No automated ansible-lint/syntax-check CI exists in this repo (checked `.woodpecker/`), so this is consistent with how the project normally verifies playbook changes: the PR body documents an idempotent `--check --diff` run against the live host for all four plays, which is the expected level of verification here. I independently confirmed the `[edge_host:children]` INI pattern already exists elsewhere in the inventory (`[authentik:children]`), that role-default fallbacks for `edge_base_dir` match the removed play-level override, and that no `notify:`/handler pairs cross the new play boundaries (each moved role's handlers are self-contained). ## Recommendation Looks reasonable — no blocking issues; comments above are optional.

k.renner merged commit 1ee4c2b61c into master

2026-08-10 19:26:12 +00:00

k.renner referenced this issue from a commit

2026-08-10 19:26:14 +00:00

Merge pull request 'refactor(edge): split the plays by capability; pin open3e and register it with the audit' (#475) from edge-capability-groups into master

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: konairius/homelab#475

refactor(edge): split the plays by capability; pin open3e and register it with the audit #475

1. Split the plays by capability, not just the roles

The split exposed a circular dependency

Verification — --check --diff against the live host

2. Pin open3e, and register both CAN services with the update audit

What this does NOT fix

Registered with the weekly audit

Summary

🟡 Suggestions

🟢 Nits

Tests

Recommendation

Summary

🟡 Suggestions

🟢 Nits

Tests

Recommendation

Summary

🟡 Suggestions

🟢 Nits

Tests

Recommendation

Verification — `--check --diff` against the live host