Skip to content

refactor(node): read Reconcile as a step list; extract hold predicates#494

Draft
bdchatham wants to merge 1 commit into
mainfrom
refactor/seinode-reconcile-readability
Draft

refactor(node): read Reconcile as a step list; extract hold predicates#494
bdchatham wants to merge 1 commit into
mainfrom
refactor/seinode-reconcile-readability

Conversation

@bdchatham

Copy link
Copy Markdown
Collaborator

Behavior-preserving readability refactor of SeiNodeReconciler.Reconcile — it now reads as a sequence of named steps rather than prose-annotated orchestration.

What changed (structure + comment placement only)

  • Extracted the two inline hold predicates into named methods, relocating their rationale to the methods' doc comments:
    • holdInitialSTS RHS → shouldHoldInitialStatefulSet(node) (statefulset.go, next to reconcileStatefulSet)
    • holdForWorkflow RHS → adoptedWorkflowIsExecuting(node) (workflow.go, next to adoptedWorkflowParkedFailed)
  • Kept the StateSyncReady-ordering invariant at its call site (trimmed 7→4 lines) — it's a sequencing dependency not visible from call order.
  • Deliberately left inline (documented): the status-snapshot bundle (extracting it would thread a struct through reconcileWorkflow's existing signature for zero gain) and the two flush-and-return exits (distinct error-wrap strings a shared helper would blur).

Behavior

No control-flow change. The if !holdInitialSTS && !holdForWorkflow guard is byte-identical (the locals are kept as the readable step names). Existing internal/controller/node + internal/planner tests pass unchanged — the behavior-preservation proof. gofmt/vet/build clean.

🤖 Generated with Claude Code

Behavior-preserving readability refactor of SeiNodeReconciler.Reconcile so
the body reads as a sequence of named steps instead of prose-annotated
orchestration. The two inline hold predicates become named methods that
carry their rationale as doc comments:
  - holdInitialSTS RHS  -> shouldHoldInitialStatefulSet(node)  (statefulset.go)
  - holdForWorkflow RHS -> adoptedWorkflowIsExecuting(node)    (workflow.go)
The StateSyncReady-ordering invariant is kept (trimmed) at its call site;
the status-snapshot bundle and the two flush-and-return exits are left
inline by design. No control-flow change; node+planner tests pass unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@cursor

cursor Bot commented Jul 16, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
No logic changes—only comment relocation and predicate extraction; the STS apply guard is byte-identical.

Overview
Readability-only refactor of SeiNodeReconciler.Reconcile: the two StatefulSet skip conditions are now named steps instead of long inline comments.

holdInitialSTS and holdForWorkflow still gate reconcileStatefulSet the same way (if !holdInitialSTS && !holdForWorkflow), but their logic lives in shouldHoldInitialStatefulSet (statefulset.go, next to reconcileStatefulSet) and adoptedWorkflowIsExecuting (workflow.go, beside adoptedWorkflowParkedFailed). The prior rationale (state-sync gate vs data PVC, mid-wipe workflow vs parked-failed release) moves into those functions’ doc comments.

The StateSyncReady ordering note at the gate call site is shortened; no sequencing or control-flow change.

Reviewed by Cursor Bugbot for commit 3d34a68. Bugbot is set up for automated code reviews on this repo. Configure here.

@bdchatham bdchatham marked this pull request as draft July 16, 2026 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant