docs(types): make ResourceIdentifier.Key() a stated, tested contract - #273
Conversation
Key() is an identity two products must agree on — a downstream tool mirrors
the struct and joins its rows against ours — but the format lived in a three
line comment and had no test at all, so we could change it today and every
gate would stay green.
- Godoc names the format as public, spells out the two empty-segment rules
that pull in opposite directions (a cluster-scoped resource drops the
namespace segment; a core-group resource emits an empty group and leads
with "/"), and lists the four resulting shapes.
- A table-driven golden test pins the exact strings for those four shapes,
carrying the sentence that makes a red run a decision: changing a byte of
this format is a breaking change rather than a refactor. Two runnable
examples put the same strings in godoc.
- Records the version-identity decision beside both methods: Key is the
API-side identity and splits on a preferred-version bump, while the
versionless ToGitPath is the durable one, which is why such a bump moves
no file in Git. A test pins that disagreement so it cannot drift.
- Disambiguates ResourceReference.Key ("namespace/name"), the other Key()
a reader can land on first.
- One line in the pkg/manifestanalyzer package doc points at the format,
since that is where a consumer of the analyzer looks.
Implements Ask 3 of docs/design/analyzer-consumer-contract-asks.md.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…Key() Scanning for places that build a resource identity by hand turned up exactly one: sensitiveIdentityKey duplicated the format inline and always joined five segments, the naive shape the Key() godoc now warns about. It is a process-local map key so nothing was wrong today, but it is a second implementation of a format we just declared a contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Implements Ask 3 of
docs/design/analyzer-consumer-contract-asks.md:ResourceIdentifier.Key()is an identity two products must agree on, but its format lived in a three-line comment and had no test at all — we could have changed it today and every gate would have stayed green, and a downstream tool that mirrors the struct would have found out late, after a version bump it chose.What changed
The format is now stated as public. The godoc on
Key()names it a contract, spells out the two empty-segment rules that pull in opposite directions, and lists the four shapes:apps/v1/deployments/prod/apirbac.authorization.k8s.io/v1/clusterroles/admin/v1/secrets/prod/db/v1/nodes/node-1The two rules are opposites, which is what a reimplementation gets wrong: a cluster-scoped resource drops the namespace segment (always joining five parts yields
…/clusterroles//admin, which never joins), while a core-group resource emits its empty group, so the key leads with/. The fourth case applies both at once.The golden test pins the exact strings, in
internal/types/identifier_key_test.go, carrying the sentence that turns a red run from an obstacle into a decision: changing a byte of this format is a breaking change rather than a refactor. Two further tests guard the properties the format exists for — that no two distinct identifiers collide (including the cluster-scoped-name vs. namespace-name case that a dropped segment could conflate), and the version disagreement below.Two runnable examples (
ExampleResourceIdentifier_Key,ExampleResourceIdentifier_ToGitPath) put the same four strings in godoc, so a consumer reading the docs sees the output rather than a description of it, andgo testfails if the two ever disagree.The version-identity decision, recorded beside both methods. The ask asked us to decide which of the two functions is the identity, since
Key()includesVersionandToGitPath()deliberately does not:TestResourceIdentifier_Key_VersionSplitsWhereGitPathDoesNotpins that disagreement so the recorded decision cannot quietly stop being true.Ancillary:
ResourceReference.Key()("namespace/name") now says it is not the otherKey()— the ask called the ambiguity out explicitly, and it is the one a reader can land on first.One line in the
pkg/manifestanalyzerpackage doc points at the format with the four shapes inline, since that is where a consumer of the analyzer looks andinternal/typesis not importable from another module.The scan for reimplementations
Swept the tree for places that build a resource identity by hand rather than calling
Key(). Exactly one turned up:sensitiveIdentityKeyininternal/git/content_writer.goduplicated the format inline and always joined five segments — the naive shape the new godoc warns about. It is a process-local map key for the secret-encryption marker cache, so nothing was wrong today (only namespaced resources reach it in practice, and a trailing empty segment is still unique), but it was a second implementation of a format we are now declaring a contract. Replaced withid.Key().Everything else that looked adjacent is a genuinely different key and was left alone:
PlacementTypeKeyand thebyTypeplacement key are GVR type keys with no name or namespace,windowPathKeykeys on a destination path by design,manifestedit's index key is document-side (apiVersion/kind, notresource),mutationlab.ObjectKeyis a struct that also carries subresource and UID, andtypeset's sort key is type-only.Not in this PR
The ask floats one optional extra — a versionless key method beside
Key(), so a consumer joining across a storage-version bump has something to mirror instead of building their own. It is cheap, but it would add an exported method with no in-tree caller, so it is a maintainer call rather than an implementation detail: say the word and it goes in this PR. Without it, the decision above is recorded and the consumer drops the second segment themselves, which the godoc now tells them to do.Validation
task lint,task test(77.7%, within the baseline tolerance) andtask test-e2eall pass.🤖 Generated with Claude Code