refactor(config): one env reader, not three copies (C3) — ⚠ unverified C++ #55
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "c3/config-surface"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
C3 / Track 1. ⚠ Unverified C++ — no local Chromium; a t7 build will follow and this shouldn't merge until it's green.
The plan's premise was mostly already false
It called for "one typed struct tree in
capture/config/, three ordered sources, ~15 knobs made runtime-settable". Reading the tree first:WsClientConfigandIceConfigare proper structs with documented fields and their ownLoadConfigFromEnv(). That pattern is good; this change doesn't touch it.What was actually wrong
1.
EnvOrDefault()copy-pasted byte-identical into three files (cb_clipboard_relay.cc,cb_stats_relay.cc,cb_file_upload_relay.cc). Four lines each — but the semantics are subtle, so it's three places to get wrong.2. No single place to answer "what does this binary read from the environment?" You had to grep, and grep doesn't tell you the defaults or which vars are load-bearing. For someone standing up their own deployment that's the first question, and the answer lived across six files.
What this is — and deliberately isn't
capture/config/cb_env_config.{h,cc}: one shared reader, three entry points, and a header documenting the surface.Not a config framework. No registry, no validation DSL, no central struct every subsystem must register with. Those would be a bigger change than the problem justifies and would fight the per-subsystem structs, which are fine.
The subtle bit, now tested instead of folklore
An env var set but empty counts as absent. All three original copies did this, and the deployment depends on it: k8s renders an unset ConfigMap key as
"", so empty means "operator didn't configure this", not "operator wants an empty URL". A refactor "fixing" that would hand empty strings to URL parsers in every pod with an optional key omitted.That's now the load-bearing case in
cb_env_config_test.ccwith the reasoning attached, rather than behaviour that happened to be replicated three times without comment.Deliberately left alone
The
M3-R2-env-sourceTODO (should this usebase::Environmentrather thangetenv?) is a behaviour question, and answering it while consolidating would change semantics at three call sites simultaneously. Consolidate first; it can then be settled in one place instead of four.Sequencing note
The new
cb_env_config_unitteststarget is not added to the t7 lane here. #55 (c4b/wire-envelope-tests) teaches STEP 7 to derive its test list from the build targets — that must land first, or this would be a fifth test binary that builds and never runs, which is precisely the problem #55 exists to fix.Verification
make verify,cxx-include-lintclean (124 files)<cstdlib>dropped from the three relays — checked nothing else in them used it⚠ UNVERIFIED C++ — no local Chromium. A t7 build will follow; do not merge until it is green. The plan called for "one typed struct tree in capture/config/, three ordered sources, ~15 knobs made runtime-settable". Reading the tree first, most of that premise was already false: * config is ALREADY typed. WsClientConfig (cb_signaling_ws_client.h) and IceConfig (cb_ice_config.h) are proper structs with documented fields and their own LoadConfigFromEnv(). That pattern is good and this change does not touch it. * there is no sprawl of untyped knobs. The embedder reads exactly 12 env vars, and 5 of those already flow through the typed loaders above. What WAS actually wrong, and is what this change fixes: 1. EnvOrDefault() was copy-pasted BYTE-IDENTICAL into three files — cb_clipboard_relay.cc, cb_stats_relay.cc, cb_file_upload_relay.cc. Four lines each, but the semantics are subtle (see below), so it is three places to get wrong. 2. There was no single place to answer "what does this binary read from the environment?". You had to grep, and grep does not tell you the defaults or which vars are load-bearing. For someone standing up their own deployment that is the FIRST question, and the answer lived in six files. So: capture/config/cb_env_config.{h,cc} — one shared reader, three entry points (GetEnvOr / GetEnv / GetEnvBool), and a header that documents the env surface. Deliberately NOT a config framework: no registry, no validation DSL, no central struct every subsystem must register with. Those would be a bigger change than the problem justifies and would fight the per-subsystem structs, which are fine as they are. The subtle semantics, now stated once and tested: an env var that is SET BUT EMPTY counts as absent. All three original copies did this, and the deployment depends on it — k8s renders an unset ConfigMap key as "", so an empty value means "operator did not configure this", not "operator wants an empty URL". A future refactor "fixing" that would hand empty strings to URL parsers in every pod with an optional key omitted. That is now the load-bearing case in cb_env_config_test.cc with the reasoning attached, rather than folklore living in three comment-free copies. Also drops <cstdlib> from the three relays — it was there only for std::getenv and nothing else in those files used it (checked, not assumed). The M3-R2-env-source TODO in cb_signaling_ws_client.h (should this use base::Environment instead of getenv?) is deliberately NOT resolved here. It is a behaviour question, and answering it while consolidating would change semantics at three call sites at once. Consolidate first; that TODO can then be settled in ONE place instead of four. New test target cb_env_config_unittests. It is not yet in the t7 lane's CHROMELESS_BUILD_TARGETS — the branch that teaches STEP 7 to derive its test list (c4b/wire-envelope-tests) has to land first, or this would be a fifth test binary that builds and never runs, which is exactly the problem that branch exists to fix. Verified locally: make verify, cxx-include-lint clean (124 files). NOT verified: that it compiles. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>One conflicted file, capture/build-integration/BUILD.gn, in the three relay targets that C3 touches (stats_relay, clipboard_relay, file_upload_relay). Both sides edited the same `deps` lists: C3 adds //cloud-browser/capture/config:cb_env_config (the shared env reader that replaces three byte-identical EnvOrDefault copies) #57 moved //base from `deps` to `public_deps` (its headers are in these targets' PUBLIC interface, so dependents need its include dirs — without it no dep-narrow target could compile) Resolution: keep C3's cb_env_config dep, drop C3's now-redundant `//base` line from `deps` since #57 put it in `public_deps` of the same target. Listing it in both is harmless but misleading — it would suggest //base is only an implementation detail here, which is exactly the belief that cost the ten-week unbuilt-test gap. Verified per target rather than by trusting the merge: all three have public_deps //base = 1 and cb_env_config = 1, and the file still has 20 "//base" entries overall (none dropped elsewhere). `make verify` clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>