fix(config)!: address a database section's errors to that section - #1115
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 37 minutes Limit details: You’ve used the included review currently available. Your 105 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughDatabase section normalization now qualifies ChangesDatabase error qualification
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This PR qualifies non-root database configuration error fields while preserving root behavior and documenting the consumer migration. No actionable merge-blocking risk remains after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
a9f6e5a to
58607a0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Root, databases.<name>, and multitenant.tenants.<id>.database share one normalization module and therefore one set of error constructors, all spelling their fields against the root. The startup door attached the section path by WRAPPING, so the message read "databases.reporting: config_missing: database.database required" while the *ConfigError a consumer reaches through errors.As still said Field="database.database". A consumer switching on Field could not tell which section failed, and the spelling disagreed with the path-qualified keys ADR-051's delivered-empty check already emits for the same sections. normalizeDatabaseSection now rewrites the error it returns: a key under the root section swaps its "database" head for the section path (databases.reporting.host), and a tenant keeps its own trailing ".database" (multitenant.tenants.acme.database.host) -- byte-identical to the delivered-empty spelling, cross-checked by a test over every identity key rather than by two hand-typed tables agreeing. A field that is not key-shaped is prefixed instead, so the Oracle connection- identifier check keeps its name. The rewrite works on a copy, because the constructors are shared with the connect door, which resolves a config with no section path to speak of; that door keeps the root spelling and is pinned by its own test. The wrapping message is gone, so the path is printed once. Two adjacent defects were found while writing this and are filed, not fixed: the runtime door still reports the root spelling for a dynamically-resolved tenant, alongside three other Field spellings in the tenant tree (#1113), and Action still names the root env var for a named section, which the removed wrapper used to frame as a template (#1114). ADR-076 records both deferrals and their cost. Decision recorded in ADR-076; migration atom C60.16. Closes #1025
CodeRabbit (mirror#6). The migration advice told consumers to replace field == "database.host" with strings.HasSuffix(field, ".host"), but ConfigError.Field is not a database-only namespace: cache.redis.host and messaging.broker.host both end in .host, so a consumer following the runbook would route an unrelated config error as a database-host error. C60.16 now carries a predicate scoped to the three database field families -- exact match on database.<key>, and a suffix only behind the databases. or multitenant.tenants. prefix -- and the ADR, the ADR index summary and the CLAUDE.md line say the same thing rather than "match by suffix".
58607a0 to
f749c7c
Compare
|
1 participant
What
All three database sections share one normalization module, so each raised errors spelling their fields against the root. The startup door attached the path by wrapping the message, leaving the
*ConfigErrorbehinderrors.AssayingField = "database.database", so a consumer switching onFieldcould not tell which section failed. The path now lives inFieldand only there (databases.reporting.host,multitenant.tenants.acme.database.host), byte-identical to what ADR-051's delivered-empty check already emits.Impact
Breaking (ADR-076, atom C60.16).
field == "database.host"matchers break for non-root sections —errors.Asplus a suffix match survives — and the message loses itsdatabases.reporting:prefix. Root and the connect door are unchanged. Filed not fixed: #1113 (the runtime door and three other tenant-tree spellings), #1114 (Actionstill names the root env var).Verification
A test cross-checks the qualified spelling against the delivered-empty producer over every identity key, rather than trusting two hand-typed tables to agree.
Closes #1025
Summary by CodeRabbit
Bug Fixes
Documentation