refactor: dismantle the gocognit hotspots, one concern per function - #14060
Merged
Conversation
glours
force-pushed
the
refactor-wait-dependencies
branch
from
August 18, 2026 08:42
d77d68e to
accd1f4
Compare
waitDependencies (cognitive complexity 76) inlined the polling loop and the per-condition logic for all three depends_on conditions, each with its own optional-dependency degradation. The polling loop moves to waitDependency, and each condition check becomes a function reporting (done, err) — (false, nil) means keep polling. The driver drops to complexity 11 and its FIXME suppression is gone. No behavior change: same events, same log messages, same errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
RootCommand (cognitive complexity 62) buried four independent concerns inside the PersistentPreRunE closure, where every branch costs double. Each moves to a named function: runParentPreRun (cobra doesn't chain the parent's PersistentPreRunE automatically), resolveAnsiMode (--ansi vs deprecated --no-ansi vs COMPOSE_ANSI), applyDisplayMode (ANSI + NO_COLOR + progress mode), normalizeProjectOptions (--workdir deprecation, env-file paths) and resolveMaxConcurrency (COMPOSE_PARALLEL_LIMIT vs --parallel). The prerun now reads as the sequence of those steps; RootCommand drops to complexity 22 and its FIXME suppression is gone. No behavior change: same errors, same deprecation warnings, same precedence rules. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
monitor.Start (cognitive complexity 77) inlined the handling of all four container actions in the event loop. Each action moves to an onContainerX handler, with the recurring bits named: watched() (is this service's container ours to track), notify() (broadcast to listeners), initialContainers() (seed the tracking set). The loop drops to complexity 19 and reads as: seed, subscribe, dispatch until no containers remain. The tracking sets stay explicit parameters so data flow remains visible. The monitor had no unit test despite driving up/logs termination; the refactor is locked by a new suite covering the full lifecycle (created/recreated/started/restarted/exited, default-name trimming, exit codes), the restart detection through both engine states (Restarting, and Running per moby/moby#45538), the already-removed container on die, service filtering, the no-container fast path, events stream errors, context cancellation, and exit-code parse errors. No behavior change: same events, same ordering, same termination conditions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Up (cognitive complexity 79) interleaved service creation with an implicit session object: an errgroup, an error collector, exit status and termination flag, all shared between six closures. That session is now an explicit upSession type; each concern becomes a method: runEventLoop (signals/keyboard/cancellation), stopApplication and killApplication (the first/second-interrupt actions), stopOnFirstExit (--abort-on-container-exit cascade), captureExitCodeFrom (--exit-code-from), followStartedContainers + streamContainerLogs (attach to (re)started containers). Keyboard menu setup moves to setupNavigationMenu, closing the keyboard on the desktop-detection error path exactly where Up's deferred Close used to. Also fixes a latent data race: the graceful-stop and cascade-stop goroutines assigned their error to Up's outer err variable (racing with the main goroutine) before handing it to the collector; the assignment was redundant and is now local. No behavior change otherwise: same listeners in the same order, same cancellation points, same error report. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof
force-pushed
the
refactor-wait-dependencies
branch
from
August 18, 2026 09:41
accd1f4 to
cc5e408
Compare
pull (cognitive complexity 56) interleaved two scheduling loops (service images, pre_start hook images) sharing dedup state, failure slots and the must-build fallback list. That state becomes an explicit imagePuller; the loops become pullServiceImages and pullHookImages, the per-service goroutine body becomes runServicePull, and the four copies of the Skipped event literal collapse into eventSkippedPull. Also fixes a latent data race: mustBuild was appended from concurrent pull goroutines without synchronization; it is now guarded by a mutex. No behavior change otherwise: same skip events, same fail-fast rules, same error report. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
watch (cognitive complexity 52) inlined three passes over each service's triggers; they become prepareRebuildTriggers (validation + always-build marking), watchTriggerPaths (paths to monitor, bind-mount skips, initial sync) and initialSyncRequested (the DEPRECATED x-initialSync fallback). initialSyncFiles (34) delegates its WalkDir closure to initialSyncDirectory, keeping the Info() call order so error behavior on unreadable entries is unchanged. No behavior change: same validations, same warnings, same collected paths and sync mappings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Load (cognitive complexity 48) mixed the enable/offline guards with the artifact pull, the cache check and the image-index indirection. The pull path moves to pullComposeArtifact and the index-to-manifest resolution to resolveComposeManifest. Load reads as: guards, cache lookup, pull on miss. No behavior change, including the last-matching-manifest-wins loop and the silent cache hit on a non-NotExist stat error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Three command paths with the same shape — a setup phase then per- container goroutines with inlined bodies: - Ps (44): the goroutine body becomes containerSummary, with pure helpers for publishers, health/exit-code, mounts and networks. - restart (42): project resolution moves to prepareRestartProject, the per-container body to restartContainer. Also fixes a latent data race: hooks and restart assigned their error to the function's outer err from concurrent goroutines; now local. - Logs (31): container selection moves to selectLogsContainers, the streaming body to logContainer, and the follow-mode listener to followStartedContainersLogs. No behavior change: same events, same warnings, same results. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
doBuildImage (cognitive complexity 43) mixed builder-capability guards, context-type resolution (with in-scope defers), tar archiving and credential conversion. Each becomes a function; the context resolution returns a classicBuildContext carrying a cleanup so the defers run at the same point they used to. No behavior change: same errors, same context handling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
publish (cognitive complexity 34) moves its non-dry-run push path to pushComposeArtifact, with the application image index construction in pushApplicationIndex. checkForSensitiveData (34) splits per source kind: scanEnvFiles (per-service env files with the required/missing rules) and scanFiles (file-backed configs and secrets, deduplicating two identical loops). No behavior change: same events, same errors, same findings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
createEndpointSettings (cognitive complexity 31) moves address parsing to parseEndpointIPAM and the interface_name/driver_opts merge to endpointDriverOpts. buildContainerVolumes (34) gets one decision helper per mount type: bindStringForMount, volumeBindString, checkImageMountSupported. A dead branch in the bind case (source was reassigned to the same value) is simplified away — the surrounding behavior is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
LoadAdditionalResources (cognitive complexity 31) moves its per-service body to loadServiceImageResources, with the local-vs-pull inspection decision in inspectServiceImage. TestViz (35) factors the shared graph assertions into assertVizGraphNodes and assertVizDependencyEdges (t.Helper), merging the allowed/forbidden edge bookkeeping into one equivalent loop. This removes the last two of the 19 //nolint:gocognit suppressions introduced by the gocyclo->gocognit migration: the FIXME debt is fully paid, the linter now runs suppression-free. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
glours
previously approved these changes
Aug 18, 2026
ndeloof
changed the title
glours
approved these changes
Aug 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What I did
Consolidates the whole gocognit-refactoring stack (former #14061–#14071, one commit each) into a single PR based on main. Each commit dismantles one cognitive-complexity hotspot into named, single-concern functions, with no behavior change, and removes the corresponding
FIXME/nolint:gocognitsuppression:waitDependencies: one function per depends_on conditionRootCommand.PersistentPreRunEconcerns extracted (resolveAnsiMode,applyDisplayMode,normalizeProjectOptions, ...)monitor.Start: one handler per container event, with testsupmaterialized as anupSessionpullscheduling materialized as animagePullerwatchtrigger preparation and initial-sync walk flattenedociRemoteLoader.Loadpull and index resolution splitPs,restartandLogsorchestration flatteneddoBuildImagecontext preparation stagedpublishpush path and sensitive-data scanning splitcreateendpoint and volume translation decisions extractedTestViz🤖 Generated with Claude Code