Skip to content

network/node apply: add repeatable --config-value for spec.configValues (PLT-1246) - #253

Open
bdchatham wants to merge 3 commits into
mainfrom
devin/1789091174-seictl-config-value
Open

network/node apply: add repeatable --config-value for spec.configValues (PLT-1246)#253
bdchatham wants to merge 3 commits into
mainfrom
devin/1789091174-seictl-config-value

Conversation

@bdchatham

Copy link
Copy Markdown
Contributor

Summary

Closes PLT-1246. The controller's spec 002/003 spec.configValues (typed per-file TOML overlays) had no authoring path in seictl other than --set spec.configValues[N].*, which replaces list slots by index and can't express a bool/number without three --sets. Both network apply and node apply now take a repeatable

--config-value <file>.toml:<dotted.key>=<value>

New internal/cliutil/configvalue.go:

func ParseConfigValue(expr string) (map[string]interface{}, error)   // -> {fileName, key, value}
func ApplyConfigValues(root map[string]interface{}, exprs []string, fieldPath ...string) error
  • value is json.Unmarshaled when it parses (bool, number, array, table, "quoted string"); anything else is kept as a plain string, so sc-write-mode=async and timeout_commit="400ms" both do the obvious thing.
  • CRD constraints are enforced locally so a typo fails at render, not at Flux apply: fileName ^[A-Za-z0-9_-]+\.toml$ (≤64), key ^[A-Za-z0-9_-]+(\.[A-Za-z0-9_-]+)*$ (≤256), null/nested null refused (no TOML representation → plan-build failure), MaxItems=100.
  • Merge semantics: applied after --set/--override; an entry with the same (fileName, key) already in the list (from preset or --set) is replaced in place, new ones append in flag order. Empty flag list is a no-op (does not create spec.configValues).

Help text for both commands documents the layering and the operational caveat that editing configValues on a live SeiNetwork restarts the entire validator pool.

Tests: helper table tests (typed values, = inside value, each rejection path, merge, max-items, no-op) plus render tests in seinetwork and seinode for merge-with---set and TOML-only refusal. go build ./... && go test ./... && make lint clean.

Link to Devin session: https://app.devin.ai/sessions/967951560dfc4f51a8fe1c8697305c7b
Open in Devin Desktop: https://app.devin.ai/desktop/session/967951560dfc4f51a8fe1c8697305c7b?variant=devin
Requested by: @bdchatham

…es (PLT-1246)

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration

Copy link
Copy Markdown

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@cursor

cursor Bot commented Sep 11, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Misconfigured values still apply to the cluster and can restart validators or seid; the CLI validates shape but not semantic TOML keys.

Overview
Adds a repeatable --config-value <file>.toml:<dotted.key>=<value> flag to seictl network apply and seictl node apply, so operators can populate spec.configValues without indexing the whole list via --set.

New internal/cliutil helpers ParseConfigValue and ApplyConfigValues parse expressions into {fileName, key, value}, JSON-type values when possible (with int64 preservation), enforce CRD shape limits (TOML filenames, dotted keys, no null, max 100 entries), and merge by (fileName, key)—replacing preset/--set entries instead of duplicating them. Rendering runs --config-value after --set (and genesis/override flags where applicable). Help text documents flag layering and that changing configValues on a live SeiNetwork restarts all validators.

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

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings on the changed lines. The verdict and the summary are in this tool's comment on this pull request.

Comment thread internal/cliutil/configvalue.go Outdated

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review found nothing blocking.

@seidroid

seidroid Bot commented Sep 11, 2026

Copy link
Copy Markdown

1. Blocking

None.

2. Non-blocking

  • internal/cliutil/configvalue.go:50json.Unmarshal into interface{} renders every JSON number as float64, so an integer above 2^53 is silently rounded before it reaches the CR (codex raised this; verified against the code). Detail inline.
  • internal/cliutil/configvalue.go:105-115 — the merge loop breaks on the first (fileName, key) match, so a duplicate pair already present from the preset or from --set survives the merge; ApplyConfigValues checks the list against MaxConfigValues but never checks it for duplicates, which is the other constraint the controller enforces. Given the PR's stated goal of failing at render rather than at Flux apply, a duplicate scan over the final list would close the same gap the count check does (codex raised the residual-duplicate half of this).
  • internal/cliutil/configvalue_test.go — no table case exercises a bare numeric value; the case named nested key number asserts a bool (giga_executor.occ_enabled=false). Number is the one input type whose in-process representation changes, so it is the case most worth pinning.
  • seinetwork/apply.go:105-117 — the seinode help text explicitly contrasts --override/spec.overrides against --config-value/spec.configValues, but the seinetwork text does not do the same for spec.configOverrides, which the shipped genesis-chain preset already populates (seinetwork/presets/genesis-chain.yaml:6-7). A user can now set the same TOML key from both surfaces on one CR with no CLI-side detection and no documented precedence.
  • internal/cliutil/configvalue.go:96-98 — the NestedSlice read failure is wrapped with fmt.Errorf, not UsageError, so --set spec.configValues=<scalar> followed by --config-value surfaces as an internal error rather than as the usage error it is. Matches the existing shape in parse.go, so this is consistency-with-a-wart rather than a regression.

3. Summary

ParseConfigValue/ApplyConfigValues are correct on the paths that matter: the index arithmetic around the : and = splits is safe, the fileName regex is anchored under Go's default OneLine semantics so neither a path separator nor an embedded newline can reach the field name, containsNull recurses fully, the merge is deterministic (slice order, no map iteration), and nothing in the new code can panic through NestedSlice/SetNestedSlice since every value it produces is in apimachinery's deep-copyable set. Of the two readings carried in, I kept both but demoted both to non-blocking and corrected their line numbers (the reported 55 and 101 are diff offsets; the file lines are 50 and 110): the float64 widening is real but mirrors the established ApplyGenesisOverride pattern and only bites above 2^53, and the residual-duplicate case requires input that is already invalid, so neither breaks the change as written. No Go toolchain is available in this sandbox, so the author's go build ./... && go test ./... claim is unverified here; the new test files' strings/unstructured uses are already covered by existing imports in both render_test.go files, so nothing in the diff should fail to compile. Approving with the notes above.

seidroid review · decision approve · session c1f1919272a74bfdab7fa21438b79d07 · turn resp_claude_621508db9d0be72958097bd6e7f0306b · item 1873823d75765dfcb2a8175c8871faa9

Findings: 0 blocking | 6 non-blocking | 1 posted inline

…gOverrides overlap

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.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.

1 participant