From 8b7ad9b4abd857225a8327827e57743e8a1da155 Mon Sep 17 00:00:00 2001 From: Andrew Hay Date: Thu, 23 Apr 2026 21:39:18 -0400 Subject: [PATCH 1/2] refactor(pihole): reduce cyclomatic complexity of ApplyChanges Split ApplyChanges into phase-per-method helpers so each function is under the cyclop threshold and the top-level reads as a linear sequence of phases: ApplyChanges - applyDeletes - buildUpdateMap (v6 target merge/dedup lives here) - applyUpdateOld (drops no-op updates, deletes changed ones) - updateIsNoOp (v6 full-target vs v5 first-target compare) - applyCreates Behavior is unchanged: v6 still merges and deduplicates targets per (DNSName, RecordType), v5 still only compares the first target when deciding whether an update is a no-op, and the create phase still runs pure creates before update-driven creates. Contributes to #5419. Signed-off-by: Andrew Hay --- provider/pihole/pihole.go | 94 ++++++++++++++++++++++++--------------- 1 file changed, 59 insertions(+), 35 deletions(-) diff --git a/provider/pihole/pihole.go b/provider/pihole/pihole.go index 76e295fffd..46990a239b 100644 --- a/provider/pihole/pihole.go +++ b/provider/pihole/pihole.go @@ -120,59 +120,84 @@ func (p *PiholeProvider) Records(ctx context.Context) ([]*endpoint.Endpoint, err // ApplyChanges implements Provider, syncing desired state with the Pi-hole server Local DNS. func (p *PiholeProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) error { - // Handle pure deletes first. - for _, ep := range changes.Delete { + if err := p.applyDeletes(ctx, changes.Delete); err != nil { + return err + } + + updateNew := p.buildUpdateMap(changes.UpdateNew) + + if err := p.applyUpdateOld(ctx, changes.UpdateOld, updateNew); err != nil { + return err + } + + return p.applyCreates(ctx, changes.Create, updateNew) +} + +// applyDeletes performs pure deletes from the plan. +func (p *PiholeProvider) applyDeletes(ctx context.Context, deletes []*endpoint.Endpoint) error { + for _, ep := range deletes { if err := p.api.deleteRecord(ctx, ep); err != nil { return err } } + return nil +} - // Handle updated state - there are no endpoints for updating in place. - updateNew := make(map[piholeEntryKey]*endpoint.Endpoint) - for _, ep := range changes.UpdateNew { +// buildUpdateMap collapses UpdateNew endpoints by (DNSName, RecordType). For +// the v6 API, endpoints that share a key have their Targets merged and +// deduplicated so a single createRecord call carries all desired targets. +func (p *PiholeProvider) buildUpdateMap(updateNew []*endpoint.Endpoint) map[piholeEntryKey]*endpoint.Endpoint { + m := make(map[piholeEntryKey]*endpoint.Endpoint, len(updateNew)) + for _, ep := range updateNew { key := piholeEntryKey{ep.DNSName, ep.RecordType} - - // If the API version is 6, we need to handle multiple targets for the same DNS name. if p.apiVersion == "6" { - if existing, ok := updateNew[key]; ok { + if existing, ok := m[key]; ok { existing.Targets = append(existing.Targets, ep.Targets...) - - // Deduplicate targets slices.Sort(existing.Targets) existing.Targets = slices.Compact(existing.Targets) - ep = existing } } - updateNew[key] = ep + m[key] = ep } + return m +} - for _, ep := range changes.UpdateOld { - // Check if this existing entry has an exact match for an updated entry and skip it if so. +// applyUpdateOld walks the old side of in-place updates. For each old entry +// whose paired new entry is unchanged, the update is dropped from updateNew +// (nothing to do). Otherwise the old record is deleted so the new record can +// be created in the subsequent phase. +func (p *PiholeProvider) applyUpdateOld(ctx context.Context, updateOld []*endpoint.Endpoint, updateNew map[piholeEntryKey]*endpoint.Endpoint) error { + for _, ep := range updateOld { key := piholeEntryKey{ep.DNSName, ep.RecordType} - if newRecord := updateNew[key]; newRecord != nil { - // If the API version is 6, we need to handle multiple targets for the same DNS name. - if p.apiVersion == "6" { - if cmp.Diff(ep.Targets, newRecord.Targets) == "" { - delete(updateNew, key) - continue - } - } else { - // For API version <= 5, we only check the first target. - if newRecord.Targets[0] == ep.Targets[0] { - delete(updateNew, key) - continue - } - } - - if err := p.api.deleteRecord(ctx, ep); err != nil { - return err - } + newRecord, ok := updateNew[key] + if !ok { + continue + } + if p.updateIsNoOp(ep, newRecord) { + delete(updateNew, key) + continue + } + if err := p.api.deleteRecord(ctx, ep); err != nil { + return err } } + return nil +} + +// updateIsNoOp reports whether an update is actually a change. For v6 all +// targets must match; for older APIs only the first target is compared, which +// matches the historical single-target semantics of the v5 local-DNS endpoint. +func (p *PiholeProvider) updateIsNoOp(oldEP, newEP *endpoint.Endpoint) bool { + if p.apiVersion == "6" { + return cmp.Diff(oldEP.Targets, newEP.Targets) == "" + } + return newEP.Targets[0] == oldEP.Targets[0] +} - // Handle pure creates before applying new updated state. - for _, ep := range changes.Create { +// applyCreates runs pure creates followed by the (possibly pruned) updates. +func (p *PiholeProvider) applyCreates(ctx context.Context, creates []*endpoint.Endpoint, updateNew map[piholeEntryKey]*endpoint.Endpoint) error { + for _, ep := range creates { if err := p.api.createRecord(ctx, ep); err != nil { return err } @@ -182,6 +207,5 @@ func (p *PiholeProvider) ApplyChanges(ctx context.Context, changes *plan.Changes return err } } - return nil } From 5aae6c60ae47c56d349a50f06cfa2b628ee04c5e Mon Sep 17 00:00:00 2001 From: Andrew Hay Date: Fri, 24 Apr 2026 07:54:06 -0400 Subject: [PATCH 2/2] address review: switch applyUpdateOld back to nil check on newRecord Per @vflaux's review feedback, use the nil-pointer check on the map lookup rather than the two-value comma-ok form. Behavior is unchanged (a missing key and a stored nil *Endpoint are both treated as 'no paired update to process') but the style matches the pre-refactor code and reads more naturally. Signed-off-by: Andrew Hay --- provider/pihole/pihole.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/provider/pihole/pihole.go b/provider/pihole/pihole.go index 46990a239b..e5557093a6 100644 --- a/provider/pihole/pihole.go +++ b/provider/pihole/pihole.go @@ -170,8 +170,8 @@ func (p *PiholeProvider) buildUpdateMap(updateNew []*endpoint.Endpoint) map[piho func (p *PiholeProvider) applyUpdateOld(ctx context.Context, updateOld []*endpoint.Endpoint, updateNew map[piholeEntryKey]*endpoint.Endpoint) error { for _, ep := range updateOld { key := piholeEntryKey{ep.DNSName, ep.RecordType} - newRecord, ok := updateNew[key] - if !ok { + newRecord := updateNew[key] + if newRecord == nil { continue } if p.updateIsNoOp(ep, newRecord) {