[ConfigManager] Install resolved configuration into the boot source - #3973
[ConfigManager] Install resolved configuration into the boot source#3973bdchatham wants to merge 2 commits into
Conversation
… boots with A key a section declares is written at override precedence, so the registry's answer wins over the file, the environment and any flag. Every other key is left exactly as it was. Layering rather than replacing is what makes the migration possible. A value can only be resolved ahead of the read if its name is known, and a name is known only once a section declares it. An environment cannot be enumerated for a prefix, so a value delivered that way under a key nothing declares is readable and unlistable at the same time: building a fresh source from an enumeration would drop exactly those values and replace an operator's setting with a code default. Leaving them alone cannot, because the code answering them is unchanged. Two declared keys where one names a prefix of the other are refused. A source holds a value at a path, so writing a.b turns a into a table and destroys what a held, and writing a afterwards destroys the table; no order installs both. That belongs here rather than at registration, because it is a property of how this source stores a key and not of whether the key space is coherent. The report is taken before anything is written. Installing makes every declared key enumerable, and the set that was not enumerable is what the report names, so taken afterwards it is always empty. Three things this package does not carry yet, each for want of a caller. A one-line summary of the report, and the mapping from a node mode to the mode Tendermint runs, both belong with the command that prints them. Reconciling the mode this file records against the one Tendermint runs is a diagnostic, and it arrives with the diagnostic that asks for it. 10 cases, 100% of statements, race clean, 0 lint issues. Mutation-checked: clobbering an undeclared key fails three tests, taking the report after installing fails two, and dropping the collision refusal fails two. A fold to lower case on what the source enumerates was removed rather than tested, because a source lower-cases on the way in and the fold could not fire.
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3973 +/- ##
==========================================
- Coverage 58.69% 57.61% -1.08%
==========================================
Files 2326 2229 -97
Lines 199122 187916 -11206
==========================================
- Hits 116877 108275 -8602
+ Misses 71477 69758 -1719
+ Partials 10768 9883 -885
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview Declared keys are written at override precedence so the registry wins over file, env, and flags. Undeclared keys are left untouched, including env-only values that viper can Install refuses colliding declared prefixes and any write that would silently shadow an undeclared value (including env-only ancestors). Refusals happen before the first Reviewed by Cursor Bugbot for commit 28004bf. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c80c82c. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
New config/appopts package is well-documented and thoroughly tested, but its central guarantee — "nothing undeclared is disturbed" — is only enforced between declared keys, leaving the reachable case (a declared key that prefixes, or is prefixed by, an existing undeclared key in the target) unguarded.
Findings: 1 blocking | 0 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- None at the file/PR level.
…ared
The package promises that migrating one section does not change what any other
key answers. It checked only declared keys against each other, and that pair
barely arises. The pair that does arise is a declared key against an undeclared
one, and it was unchecked in both directions. Measured:
declared giga_executor over an operator's giga_executor.enabled
before true, after false
declared a.b.c over an operator's a.b = 42
before 42, after map[c:x], and GetInt reads 0
Neither reports anything. The report went further and named the lost key as one
that still reads as it always has.
The stake is not hypothetical. giga_executor.enabled is set on two production
nodes, one of them the shadow replayer whose whole job is finding an AppHash
divergence between the two executors. With the executor silently off it compares
one implementation against itself, finds nothing, and reports a release safe.
A source holds one value per path, so the two keys cannot both occupy theirs.
That makes this a refusal rather than a report: skipping the declared key leaves
a section half migrated and a key whose answer depends on which reader a caller
asked, where refusing names both keys and stops before the first write.
The collision walk is replaced rather than repaired. It read a sorted key list
and stopped at the first key that was not a child, which assumes the children
follow directly. A hyphen sorts before a dot, so a hyphenated sibling sits
between a key and its children and ends the walk. This key space separates words
with hyphens, and grpc-web.address already sits between grpc and grpc.enable. The
paths a key nests under are now derived from the key, which assumes nothing about
order and answers the target-side question too.
The source is asked whether it holds a value, not whether it lists one, because a
value delivered through the environment is answerable and unlistable. It is asked
with IsSet rather than a nil read, because a bound flag nobody set still answers
with its default and refusing over one would be a false alarm.
Five mutations each fail a named test: restoring the sorted scan, dropping the
shadowing refusal, removing the IsSet gate, dropping the table arm that keeps
every nested section installable, and swapping IsSet for a nil read.
24 cases, 98.5% of statements, race clean, 0 lint issues.
1 participant
A node's configuration is answered today by viper, reading a file, the environment and flags. The registry declares keys and resolves a value for each. This is the piece that puts one into the other:
Installwrites every declared key into the source a booting node reads, at override precedence, and leaves every other key alone.Start with
config/appopts/appopts.go— the package doc explains why it layers rather than replaces, which is the decision the rest follows from.Properties
a.bturnsainto a table and destroys whataheld, and writingaafterwards destroys the table. No order installs both. This is a property of how the source stores a key rather than of whether the key space is coherent, which is why it is checked here and not at registration.Reportnames the three populations: the declared keys written, the keys that still read as they always have, and the declared keys the source did not carry. The second is the migration that remains, and it shrinks as sections are declared.Scope
This is the package alone. Nothing here calls it: the boot path that installs on startup is a separate change.
Three things it does not carry, each for want of a caller today. A one-line summary of the report, and the mapping from a node mode to the mode Tendermint runs, both belong with the command that prints them. Reconciling the mode
sei.tomlrecords against the oneconfig.tomlruns is a diagnostic, and it arrives with the diagnostic that asks for it.Verified
build,vet,gofmt -s,goimports,golangci-lintclean. 10 cases under-race, 100% of statements.Clobbering an undeclared key fails three tests, taking the report after installing fails two, and dropping the collision refusal fails two. A fold to lower case on what the source enumerates was removed rather than tested: a source lower-cases a key on the way in, so the fold could not fire.