Skip to content

refactor(task): drop taskParamser interface; use client.TaskBuilder directly#192

Merged
bdchatham merged 3 commits intomainfrom
refactor/drop-task-paramser
May 6, 2026
Merged

refactor(task): drop taskParamser interface; use client.TaskBuilder directly#192
bdchatham merged 3 commits intomainfrom
refactor/drop-task-paramser

Conversation

@bdchatham
Copy link
Copy Markdown
Collaborator

Resolves #161.

Summary

Removes the controller-local taskParamser interface (toRequestParams(), taskType()) and the 14 *Params shells that implemented it. The executor now constrains T to sidecar.TaskBuilder (the seictl-defined interface), so wire format, validation, and task-type strings all flow from a single source of truth in seictl.

20 files, -284 net lines, 2 files deleted.

What changed

Executor (internal/task/sidecar.go)

  • sidecarExecution[T] constraint changes from T any to T sidecar.TaskBuilder
  • Submit path: build wire request via e.params.ToTaskRequest(), then override req.Id with the deterministic plan-task UUID. Override happens at the TaskRequest boundary, not on the wrapper's TaskMeta — keeps the executor decoupled from wrapper internals (Demeter)
  • Old taskParamser interface + 14-entry assertion list deleted

Registry (internal/task/task.go)

  • All 14 sidecar registry entries swap from sidecarTask[*Params] to sidecarTask[sidecar.*Task]
  • One exception (config-apply): sidecarTask[configApplyTask] — the unexported local type below
  • Generic constraints (sidecarTask, deserializeSidecar) change to T sidecar.TaskBuilder

The one local exception: configApplyTask

Seictl's client.ConfigApplyTask wraps inputs in seiconfig.ConfigIntent rather than flat fields. Direct swap would change Params.Raw from {"mode": "X", ...} to {"Intent": {"mode": "X", ...}} — operationally fine but breaks structural compat for in-flight plans on rollover.

Solution: 14-line unexported type in internal/task/config.go that anonymously embeds seiconfig.ConfigIntent (whose existing json:"mode"/json:"overrides,omitempty"/etc. tags match today's controller shape exactly) and delegates the three TaskBuilder methods to a constructed client.ConfigApplyTask. Storage shape stable; validation and wire format still single-sourced upstream.

Planner (internal/planner/*.go)

  • 6 sites that constructed *task.ConfigApplyParams now construct *seiconfig.ConfigIntent (cleaner — same data, fewer copies)
  • All other planner construction sites use sidecar.*Task directly
  • paramsForTaskType factory updated; snapshotRestoreParams/discoverPeersParams/configureStateSyncParams renamed to *Task and return seictl wrappers

Tests (~10 sites)

Mechanical type-name swaps where tests unmarshal Params.Raw into typed structs. No behavior assertions changed.

Dependencies

  • go.mod: seictl v0.0.44 → v0.0.45 (picks up SetGenesisPeersTask from seictl#150)

Storage shape impact (operator-visible only)

PlannedTask.Params.Raw shape shifts from controller-side flat camelCase to seictl-side Go-PascalCase for 13 of 14 task types. Cosmetic change for operators running kubectl get snd -o yaml; no programmatic consumer cares — the field is internal to the controller and the wire payload to seictl is built fresh from ToTaskRequest() at submit time. Cross-review confirmed zero Params.Raw consumers in either repo outside the controller's own reconcile loop.

ConfigApply preserves shape via the embedding pattern.

Mid-rollover safety

In-flight plans persisted under v0.0.44 have Params.Raw bytes in the old camelCase shape. New controller deserializes them via Go's case-insensitive json unmarshal into the seictl client.*Task types — works cleanly for all 13 simple tasks (no acronym mismatches). ConfigApply round-trips byte-identically because the embedding pattern preserves the flat shape.

Verification

  • GOWORK=off go build ./... — clean
  • GOWORK=off go test ./... — all packages pass
  • GOWORK=off go vet ./... — clean
  • golangci-lint run — 0 issues
  • grep -rn 'taskParamser\|toRequestParams' — zero residual refs

References

🤖 Generated with Claude Code

bdchatham and others added 3 commits May 6, 2026 12:29
…irectly

Removes the controller's local taskParamser interface (toRequestParams,
taskType) and the 14 *Params shells that implemented it. The executor
now constrains T to sidecar.TaskBuilder (the seictl-defined interface),
calling builder.ToTaskRequest() to produce the wire payload and
overriding TaskRequest.Id with the deterministic plan-task UUID.

13 of 14 sidecar tasks use the seictl client.*Task wrappers directly —
single source of truth for shape, validation, and wire format. The
planner constructs them at the call site; the deserialize registry
unmarshals into them; the executor delegates to ToTaskRequest().

The 14th, config-apply, has a structural mismatch: seictl wraps the
inputs in seiconfig.ConfigIntent rather than flat fields. Solution is
a tiny unexported configApplyTask in the controller's task package
that anonymously embeds seiconfig.ConfigIntent (whose flat camelCase
json tags match today's PlannedTask.Params.Raw shape) and provides
three TaskBuilder methods that delegate to client.ConfigApplyTask.
Storage shape stable; wire format and validation still single-source.

Bumps seictl to v0.0.45 (which includes SetGenesisPeersTask).

Other 13 task types (snapshot-restore, configure-state-sync,
await-condition, config-validate, configure-genesis, discover-peers,
mark-ready, snapshot-upload, generate-identity, generate-gentx,
upload-genesis-artifacts, assemble-and-upload-genesis,
set-genesis-peers): planner constructs client.*Task directly;
PlannedTask.Params.Raw shape shifts from controller-side flat
camelCase to seictl-side Go-PascalCase. Cosmetic-only change for
operators reading raw plan bytes; runtime behavior is unchanged
because Params.Raw is internal to the controller and the wire payload
is built fresh from ToTaskRequest() at submit time.

Mid-rollover handled by Go's case-insensitive json unmarshal — old
camelCase bytes deserialize cleanly into PascalCase Go fields for the
flat-shape tasks. ConfigApply preserves shape via the embed.

Net -284 lines; 2 files deleted (bootstrap.go, snapshot_upload.go).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment-tidiness pass on the paramser refactor:

- Drop internal/task/genesis.go entirely. It contained only re-exports of
  sidecar.GenesisNodeParam and sidecar.GenesisAccountEntry, with a
  stale "saves the planner from a second seictl import" justification —
  group.go now imports sidecar directly anyway. Re-exports added a layer
  of indirection without saving any imports.
- Trim configApplyTask godoc — same load-bearing facts, fewer words.
- Trim Execute()'s Id-override comment to the WHY only (decoupling from
  TaskMeta internals); drop the WHAT-narration about UUIDs.
- Drop the registry block's WHAT-narration comment ("typed wrappers come
  from seictl/sidecar/client") — the sidecar.X type names already say
  this. The configApplyTask exception is documented at its type.
- Trim paramsForTaskType doc to point at configApplyTask for the why.

Net -13 hand-written comment lines, no behavior change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
--new-from-patch flagged the literal "node-0" on line 285 because the
patch touched it. The shared testNodeName const already exists at
group_accounts_test.go:21; using it here drops one of the 5 package-wide
occurrences without touching pre-existing tech-debt sites.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bdchatham bdchatham force-pushed the refactor/drop-task-paramser branch from 1392640 to 6f598f4 Compare May 6, 2026 20:04
@bdchatham bdchatham merged commit 238f7b1 into main May 6, 2026
2 checks passed
bdchatham added a commit that referenced this pull request May 6, 2026
…#193)

Pulls in #192 — drops the controller's local taskParamser interface and
14 *Params shells; executor now uses client.TaskBuilder (the seictl
interface) directly. ConfigApply uses an unexported configApplyTask
that anonymously embeds seiconfig.ConfigIntent for storage-shape
stability. Bumps seictl to v0.0.45.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bdchatham bdchatham mentioned this pull request May 6, 2026
3 tasks
bdchatham added a commit that referenced this pull request May 6, 2026
Picks up seictl#153 — drops the unused TaskMeta + applyMeta from
client.*Task wrappers. Side benefit: PlannedTask.Params.Raw bytes
shed the "ID":null suffix that came from json-marshaling the empty
embed.

No controller code changes — TaskMeta had zero references in this
repo (the executor sets TaskRequest.Id directly post-ToTaskRequest
since #192). Mid-rollover safe: pre-existing CRD bytes containing
"ID":null deserialize cleanly into TaskMeta-less structs (Go's
json.Unmarshal silently drops unknown keys).

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bdchatham added a commit that referenced this pull request May 6, 2026
Adds e.params.Validate() to sidecarExecution[T].Execute() before the
ToTaskRequest()/SubmitTask path. Catches malformed task params at
executor-time as a structured terminal failure instead of as an opaque
sidecar HTTP 400.

For most task types Validate() returns nil (empty struct or trivial
validation), so this is a no-op in practice. The two strict paths are:

  - sidecar.AssembleAndUploadGenesisTask.Validate() — required-field
    + bech32 checks. Already invoked at planner-time at group.go:45,
    so this catch is defense-in-depth.
  - configApplyTask.Validate() — delegates to seictl
    ConfigApplyTask.Validate(), which calls seiconfig.ValidateIntent.
    The planner doesn't currently call this; the executor now does.

Drive-by: drop a stale comment about overriding Id "rather than on
the wrapper's embedded TaskMeta" — TaskMeta no longer exists (removed
in seictl#153, consumed in #195). The comment was rot from #192.

Test fixture fix: TestExecuteGroupPlan_CompletesSuccessfully built
an AssembleAndUploadGenesisTask with only Nodes (missing required
AccountBalance and Namespace). It passed before only because Validate
wasn't invoked; now we populate the required fields, which is the
correct test intent for a happy-path submission test.

Closes the platform-engineer's deferred opportunity flagged in the
cross-review of #192.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bdchatham added a commit that referenced this pull request May 6, 2026
* refactor(task): invoke Validate() in executor before submit

Adds e.params.Validate() to sidecarExecution[T].Execute() before the
ToTaskRequest()/SubmitTask path. Catches malformed task params at
executor-time as a structured terminal failure instead of as an opaque
sidecar HTTP 400.

For most task types Validate() returns nil (empty struct or trivial
validation), so this is a no-op in practice. The two strict paths are:

  - sidecar.AssembleAndUploadGenesisTask.Validate() — required-field
    + bech32 checks. Already invoked at planner-time at group.go:45,
    so this catch is defense-in-depth.
  - configApplyTask.Validate() — delegates to seictl
    ConfigApplyTask.Validate(), which calls seiconfig.ValidateIntent.
    The planner doesn't currently call this; the executor now does.

One test fixture fix: TestExecuteGroupPlan_CompletesSuccessfully built
an AssembleAndUploadGenesisTask with only Nodes (missing required
AccountBalance and Namespace). It passed before because Validate
wasn't invoked; now we populate the required fields, which is the
correct test intent for a happy-path submission test.

Closes the platform-engineer's deferred opportunity flagged in the
cross-review of #192.

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

* refactor(task): invoke Validate() in executor before submit

Adds e.params.Validate() to sidecarExecution[T].Execute() before the
ToTaskRequest()/SubmitTask path. Catches malformed task params at
executor-time as a structured terminal failure instead of as an opaque
sidecar HTTP 400.

For most task types Validate() returns nil (empty struct or trivial
validation), so this is a no-op in practice. The two strict paths are:

  - sidecar.AssembleAndUploadGenesisTask.Validate() — required-field
    + bech32 checks. Already invoked at planner-time at group.go:45,
    so this catch is defense-in-depth.
  - configApplyTask.Validate() — delegates to seictl
    ConfigApplyTask.Validate(), which calls seiconfig.ValidateIntent.
    The planner doesn't currently call this; the executor now does.

Drive-by: drop a stale comment about overriding Id "rather than on
the wrapper's embedded TaskMeta" — TaskMeta no longer exists (removed
in seictl#153, consumed in #195). The comment was rot from #192.

Test fixture fix: TestExecuteGroupPlan_CompletesSuccessfully built
an AssembleAndUploadGenesisTask with only Nodes (missing required
AccountBalance and Namespace). It passed before only because Validate
wasn't invoked; now we populate the required fields, which is the
correct test intent for a happy-path submission test.

Closes the platform-engineer's deferred opportunity flagged in the
cross-review of #192.

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

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bdchatham bdchatham mentioned this pull request May 6, 2026
3 tasks
bdchatham added a commit that referenced this pull request May 6, 2026
Picks up seictl#156 — adds json tags to client.PeerSource. Effect on
this repo: PlannedTask.Params.Raw bytes for discover-peers tasks shift
from PascalCase (Type/Region/Tags/...) back to camelCase
(type/region/tags/...) and drop the omitempty fields, matching the
shape operators saw pre-#192.

No code changes — controller never references PeerSource fields by
struct-tag-sensitive json paths. Mid-rollover safe: pre-existing
PascalCase bytes deserialize cleanly into the now-tagged struct via
Go's case-insensitive json unmarshal.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

refactor: drop taskParamser in favor of client.TaskBuilder typed wrappers

1 participant