Source: Copilot review on PR #36 (F7, v0.8.0 cycle), round 4 head 8d4e86e8, comment at internal/drivers/firewall.go:128.
Finding (Minor advisory — non-blocking):
Update validates targets using spec.Name, but other Update errors use ref.Name. If ref.Name and spec.Name ever diverge (or spec.Name is empty), the no-targets error will reference the wrong firewall. Consider using ref.Name consistently for validation/error formatting in Update (or enforce/normalize equality between them).
Repro path: caller passes a ref with one Name and a spec.Config whose name field doesn't match (or is empty). The validateFirewallTargets error string firewall %q has no targets ... would render the wrong (or empty) name in the error.
Fix scope: ~3 LoC. In FirewallDriver.Update, pass ref.Name instead of (or in addition to) spec.Name to validateFirewallTargets. Add a regression test where ref.Name != spec.Name to assert the error references the ref name.
Priority: v0.8.x follow-up. Not blocking v0.8.0 release; the divergence path is narrow and the v0.8.0 P-2 cycle was held to a "round 4 final" bound to ship the cascade-fix work in a timely manner.
Labels: good-first-issue, polish, v0.8.x
Source: Copilot review on PR #36 (F7, v0.8.0 cycle), round 4 head
8d4e86e8, comment atinternal/drivers/firewall.go:128.Finding (Minor advisory — non-blocking):
Repro path: caller passes a
refwith one Name and aspec.Configwhose name field doesn't match (or is empty). ThevalidateFirewallTargetserror stringfirewall %q has no targets ...would render the wrong (or empty) name in the error.Fix scope: ~3 LoC. In
FirewallDriver.Update, passref.Nameinstead of (or in addition to)spec.NametovalidateFirewallTargets. Add a regression test whereref.Name != spec.Nameto assert the error references the ref name.Priority: v0.8.x follow-up. Not blocking v0.8.0 release; the divergence path is narrow and the v0.8.0 P-2 cycle was held to a "round 4 final" bound to ship the cascade-fix work in a timely manner.
Labels: good-first-issue, polish, v0.8.x