refactor(pihole): reduce cyclomatic complexity of ApplyChanges#6390
refactor(pihole): reduce cyclomatic complexity of ApplyChanges#6390AndrewCharlesHay wants to merge 2 commits intokubernetes-sigs:masterfrom
Conversation
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 kubernetes-sigs#5419.
Signed-off-by: Andrew Hay <andrew.hay@benchmarkanalytics.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/M
Coverage Report for CI Build 24888140379Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.02%) to 80.446%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions11 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
| return err | ||
| } | ||
| newRecord, ok := updateNew[key] | ||
| if !ok { |
There was a problem hiding this comment.
I’d prefer to keep checking for a nil 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 <andrew.hay@benchmarkanalytics.com>
|
Thanks @vflaux, just pushed the fix. Switched |
|
Are you using pihole? Could you share working example similar to #5111 (comment) |
5 participants
Contributes to #5419.
What
PiholeProvider.ApplyChangeswas at cyclomatic complexity 16 — a single function juggling delete / build-updates / reconcile-updates / create phases, with nestedapiVersion == "6"branches in the middle two phases.This PR decomposes it into phase-per-method helpers so each function is under the cyclop threshold and the top-level reads as a linear sequence:
After this change
ApplyChangesno longer appears in the cyclop offender list (verified locally withmax-complexity: 10).Behavior
Unchanged:
(DNSName, RecordType)before callingcreateRecord.Test plan
go test ./provider/pihole/...— all existing pihole tests pass without modificationgo build ./...ApplyChangesis below thresholdNo new tests added because every code path is still exercised by the existing
TestProvider_ApplyChanges/TestProviderV6_ApplyChangessuites — this is a pure extract-method refactor with no behavioral change.