Repository navigation
cli: pin every config env var name in a golden - #3059
momosh-ssv wants to merge 5 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
 |
d97da78 to
5455b09
Compare
iurii-ssv
left a comment
iurii-ssv
left a comment
There was a problem hiding this comment.
A small thing about the PR description: it was written before the follow-ups. It still says #3044's line needs adding, it gives the old -run Test_config_envNames_golden regen command, and it doesn't mention Test_config_envNames_matchCleanenv.
Plus couple suggestions below.
| for _, d := range globalcfg.Describe(cfg) { | ||
| if d.EnvName != "" { // skip nested-struct container rows | ||
| snapshot[d.YAMLPath] = d.Default | ||
| snapshot[d.YAMLPath] = column(d) |
There was a problem hiding this comment.
The snapshot is keyed by YAMLPath, and that key isn't unique for a field that has an env tag but no yaml tag: describeStruct gives such a field its parent's path. If two of them sit in one struct, they get the same key and the second overwrites the first, so the first one's env name is never pinned. matchCleanenv and fields_wellFormed loop over Describe directly, so neither of them notices.
Nothing hits this today (80 env-backed rows, 80 keys). To check it, I added ProbeA string `env:"PROBE_A"` and ProbeB string `env:"PROBE_B"` to KeyStore and regenerated. The golden has only "KeyStore": "PROBE_B". Retagging ProbeA as PROBE_TYPO then passes Test_config_envNames_golden, matchCleanenv and fields_wellFormed.
Failing on a repeated key here would close the gap, and it also protects the defaults golden:
require.NotContainsf(t, snapshot, d.YAMLPath, "%s is the YAML path of more than one env-backed field (missing yaml tag?)", d.YAMLPath)| // cleanenvEnvNames lists the env vars cleanenv reads for cfg. cleanenv doesn't export its field | ||
| // metadata, so the names are parsed out of its help text, where each variable is a two-space | ||
| // indented "NAME kind" line. | ||
| func cleanenvEnvNames(t *testing.T, cfg any) []string { |
There was a problem hiding this comment.
optional: after this PR the two golden test files share four functions word for word: assertDescribeGolden, cleanenvEnvNames, and the bodies of matchCleanenv and fields_wellFormed. That's about 75 lines in each file. Keeping them self-contained was cheap when it was one 20-line helper. Now any fix, like the one above or a cleanenv bump that changes the help format this function parses, has to be made twice, and the copies can drift apart.
Both packages already import cli/config. A small test-helper package next to it (e.g. cli/config/configtest) wouldn't tie operator and bootnode to each other, and each test here would shrink to a single call.
Env var names lived only in struct tags, so a typo or rename silently stopped an operator's env var from applying, and nothing failed. Snapshot yamlPath -> env name into envnames.golden.json for both operator and bootnode, through the same describer the defaults golden uses (now a column-generic assertDescribeGolden). Also assert two invariants the goldens don't: no two fields share an env var, and every env-backed field has an env-description.
…ldens with one command
5455b09 to
7fd63cf
Compare
This branch has not been deployed
2 participants
Follow-up from #3044 (comment).
Config env var names live only in struct tags. A typo or rename there makes an operator's env var silently do nothing, and a test that reads the name back from the tag reads the typo too.
testdata/envnames.golden.json(operator and bootnode) snapshotsyamlPath -> env namethrough the same describer asdefaults.golden.json, so any env name change shows up as a reviewed golden diff.assertDefaultsGoldenbecomes column-genericassertDescribeGolden.Test_config_fields_wellFormedasserts no two fields share an env var and every env-backed field has anenv-description(both hold today).Checked: misspelling
PROPOSER_DELAY_EPBSin its tag failsTest_config_envNames_golden. Regenerate intentionally withUPDATE_GOLDEN=1 go test ./cli/operator -run Test_config_envNames_golden.When #3044 merges, its
LEGACY_PROPOSER_ROUND_TIMEOUTneeds one line in the operator golden; whichever lands second regenerates it.