docs(decisions): add a decision record log alongside design proposals - #56
docs(decisions): add a decision record log alongside design proposals#56myasnikovdaniil wants to merge 2 commits into
Conversation
Design proposals are intent, written before the work. When implementation contradicts the design the proposal gets revised in place, so it ends up reading as though it always said the current thing -- and the reasoning that changed our minds survives only in a pull-request diff. Add decisions/ for that reasoning: one page per settled decision, never edited after merge, superseded rather than rewritten. Reviewed for accuracy by one maintainer rather than by consensus, because the decision has already been made. Wire it into the "Where do I file?" table and rewrite Proposal Drift to say what to do when the implementation diverges. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The ComputePlane delivery shape was decided three times: a tenant module with an inline cluster HelmRelease (#17), preset fields on kind: Kubernetes (#27, closed), and finally an operator-owned extra module wrapping the unchanged apps/kubernetes (#33). Only the last one is visible in the proposal today; why the preset-field model lost lives in the #33 body. Record it as the first decision, and add the Decisions section to the proposal template so later records get linked from the proposal they amend. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe change adds decision-record documentation, an ADR template, an accepted ComputePlane architecture decision, and guidance for linking decisions to design proposals. ChangesDecision records and proposal guidance
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Timofei Larkin (lllamnyp)
left a comment
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
Review: decision records
The diagnosis is right, and the evidence for it is the strongest part of this PR. Design proposals here are intent, written before the work; when implementation contradicts them we edit them in place, so they end up reading as though they always said the current thing. #42 and #44 rewrote the database-horizontal-autoscaling proposal after a spike; #53 reopens its actuation mechanism on a CloudNativePG finding; #40, #41 and #36 are the same shape. The compute-plane proposal carries a Revision (this PR): paragraph in its metadata block because that rationale had nowhere else to live. Something should exist for this, and the writing throughout — the proposal-vs-record table, the "when not to write one" list, the immutability rule — is better than most first drafts of a process.
I want one structural change before this lands, plus a set of corrections. Taking them in the order I think they matter.
1. Records should live with the proposal they amend
Concretely: design-proposals/<name>/decisions/NNNN-slug.md, with the process text folded into design-proposals/README.md and the template at design-proposals/decision-template.md, instead of a root-level decisions/ tree.
The argument that convinces me is the one your own template raises and then waves through. **Proposal:** ... — or none presupposes that a decision can exist here without a proposal. I don't think it can. An architectural decision is a decision with respect to something; this repo's front-door table already routes every candidate — cross-cutting architectural change to a design proposal here, bug or scoped feature to cozystack/cozystack, governance to an issue here. There's no residue. And a deliberation weighty enough to need a record, with no proposal to attach it to, has just demonstrated that it is a proposal.
Once that holds, the rest follows:
- Both link directions come for free. A reader browsing the proposal directory sees its decisions; a record's parent directory is its proposal. Nothing to maintain, and hand-maintained backlinks are the first thing to rot. The
Decisionssection added todesign-proposals/template.mdbecomes unnecessary rather than load-bearing. - Numbering stops colliding in practice. This repo routinely has five or so PRs open at once, so a global sequence collides constantly, and "whoever merges second renumbers on rebase" breaks any citation already written down. Per-proposal numbering collides only when two PRs touch the same proposal.
- No new top-level concept. The root README is a routing table; a
decisions/tree adds a second document class to it, and the new "a decision already made" row is slightly false — you never file a decision on its own, you file a proposal amendment that carries one. Per-proposal placement extends the process that exists and adds nothing to the front door. - Withdrawn proposals take their decisions with them instead of orphaning entries in a global log.
The case for the root log deserves recording, because parts of it are real:
- A monotonic id is short, citable and survives a directory rename. True, and the cost of giving it up is real; per-proposal files stay numbered, and citations become
compute-plane ADR 0002— more verbose, more informative. - Nygard's convention and its tooling assume one log — adr-tools defaults to a single
doc/adr. Real, but nothing in this repo would consume it: there is no CI here at all, and this template already departs from Nygard's four sections. - "What did we decide lately?" — answered in any layout by
git log -- 'design-proposals/*/decisions/*', better than by directory order. - Decisions that span proposals have no single home. This is the one that nearly carried it for me, and the answer is section 4 below.
2. This PR should introduce itself through the mechanism it introduces
As it stands, the reasoning for why decision records exist and where they live will survive only in this PR thread — which is precisely the failure the PR is written to fix. It reproduces its own diagnosis on itself.
So I'd like this to land as:
- A design proposal —
design-proposals/decision-records/README.md— stating what is being introduced and why, with the drift evidence already in your PR description. Note that a merged proposal is "a reference, not a binding spec", so the operative rules (when a record is required, the template, the immutability rule) still need to land indesign-proposals/README.mdand the template; the proposal states the change and the rationale, not the law. - A decision record under it —
design-proposals/decision-records/decisions/0001-decision-records-live-with-their-proposals.md— recording the placement decision and, in theWhy not the alternativessection, what lost: the root-level log, with the four points above stated at their real strength rather than strawmanned.
This isn't ceremony. It gives the format an exemplar written by the person who owns it, from an argument they were present for, rather than leaving 0001 — a backfill of an argument between other people — as the only pattern every future author copies. It also demonstrates per-proposal numbering immediately: two proposals, two records, both numbered 0001.
3. Corrections to the ComputePlane record
You asked me to check the Deciders line. The handle you didn't want to guess is mine, @lllamnyp — but I'd change more than that.
- The record never cites #33. It links #17, #26, #27 and #39, but not the PR where the decision was actually argued and merged, which is also where its
2026-07-18date comes from. A record whose job is to be the durable pointer to the reasoning points everywhere except at it. Add aDecided in:field to the template — that would have caught this mechanically. - The #17 attribution is wrong. The record says two reviews converged on the objection that ComputePlane should not be a distinct kind, citing #26 and my review on #17. My #17 review was an approval, and its points were: that "hidden from the tenant" was conflated with the security boundary, and the real justification for withholding access is tamper-resistance — withhold admin, not visibility; that the "1-click" framing didn't distinguish from the rejected alternative and
placementshould target a named cluster; that letting child tenants inherit an ancestor's ComputePlane re-creates the problem ComputePlanes exist to solve; and that there was no story for ComputePlane workloads reaching tenant-namespace services. The "not a distinct kind" argument is #26's alone — #26 quoted my approval summary in support of its own case. What that review did contribute is the tamper-resistance argument, which is the load-bearing rationale of this record's own Decision and Consequences sections, and it's credited to nobody. Decidersshould be@kvaps, @lllamnyp— the author of #33 and the maintainer who approved it. myasnikovdaniil's #26 forced the reopening and deserves the credit in Context, but its literal ask (no new kind at all) is not what was adopted, as the record itself says.Implemented in: not yetis false. cozystack/cozystack#3280 merged 2026-07-29.- The rejection of the preset-field model rests on the fragile argument. The record justifies the shape through
packages/extra/computeplaneand the PackageSource convention. The durable argument is structural and survives #39 deleting theextracategory: capabilities and schemas are per-kind, so two postures over one chart need two ApplicationDefinitions;ApplicationDefinitionSpechas no fixed-values facility and the conversion makes the tenant's spec become the Helm values, so operator-fixed values have nowhere to live but a chart; and a narrowopenAPISchemadoesn't substitute, because a field you can default is a field the tenant can override, while a field absent from the schema receives the chart's own unhardened default. "Hardened and unreachable" isn't expressible in a schema. - A dropped consequence. #33 was asked to co-land with #39, or land after it, because once tenants set
computeplane: truethe release-name invariant stops being a markdown disagreement and becomes a migration of live Kamaji clusters holding tenant data. #3280 has since merged and #39 is still open, so that constraint is live now. The record presents #39 as purely composing. - Minor: #27 is cited as a revision without noting it was closed unmerged, superseded by #33 the same day. In a document that is explicitly a history, that matters.
4. Process amendments
- Drop
— or nonefrom theProposal:field. Per section 1, make it required. - Add
Decided in:for the PR or issue thread where the decision was argued, and one line underWhy not the alternativesasking that each rejected option be sourced to the comment or PR it came from. Both of the substantive errors above come from writing alternatives from memory, and accuracy is checkable only if the source is linked. - Separate metadata from body in the immutability rule. "The body of an accepted record is not edited" is right, but
Status,Superseded byandImplemented inmust track reality — the lifecycle depends on it. Say that the header block is maintained and the prose below it is frozen. - Record what belongs in code instead. Where a decision's content is a fact about a type, a field or an invariant that code must respect, its primary home is a comment at that site plus a test that fails when it's violated; the record links to it rather than restating it. This project already does this well: the
computeplanerelease-name invariant is enforced atpackages/extra/computeplane/templates/check-release-name.yamland pinned bytests/release_name_test.yaml, whose suite comment carries the mechanism. That guard is a more reliable record than any prose, and it stayed more precise than this record's version of the same invariant. Without this rule the log fills with restatements of things the code already enforces. - Enforcement. There is no CI in this repository —
.github/contains only issue templates — so the rule in "Proposal Drift" is only as binding as the place a reviewer looks. Add.github/pull_request_template.mdwith one checkbox: this PR revises a merged proposal; it adds a record under that proposal'sdecisions/, or says why none is needed. That is the only enforcement available, and it costs one file. - Decide what happens to the existing in-proposal narratives. Compute-plane's
Revision (this PR):metadata line and the database-autoscaler'sWhy this changedsection plus spike appendix are three improvised solutions to this same problem, and this PR adds a fourth without retiring any. My preference is extraction, especially forRevision (this PR):, which is unmaintainable by construction — "this PR" stops resolving the moment the next revision lands. - The review bar is a governance change. "One maintainer's approval is enough" against the proposal process's "consensus from core maintainers" is probably right for accuracy-checking a settled decision, and the reasoning given for it is sound — but a lighter approval bar for a new document class should be visibly agreed rather than merged as documentation.
5. A second record, from a different subsystem
One backfilled record isn't enough to show the mechanism works, and a log that ships at 0001 and stops is the standard failure. The live candidate is the database-horizontal-autoscaling proposal: rev1's bespoke db-autoscaler operator was rejected after an implementation spike proved its enforcement premise unbuildable on the aggregated apps API. That decision is settled and merged, its rationale currently survives only in a proposal appendix that the immutability rule doesn't protect, and #53's CloudNativePG finding has since reopened the actuation mechanism downstream of it — so it would exercise Revisit if on live content rather than a hypothetical. Either land it here or open the ask on #53; I'd rather not merge the mechanism with one instance.
6. Follow-up: where design principles live
Some of what surfaced around ComputePlane isn't a decision about ComputePlane at all — that ApplicationDefinition has no operator-fixed-values facility, and that settability and defaultability are the same property in a structural schema, are standing constraints on every operator-owned module anyone builds next. A record filed under one proposal is the wrong home for them, and today they're recorded nowhere: ApplicationDefinitionSpec in api/v1alpha1/applicationdefinitions_types.go carries no such comment, and the conversion at pkg/registry/apps/application/rest.go is a bare Values: app.Spec.
Code comments plus tests cover the enforceable half, but there is genuinely no home in Cozystack for a design principle as such — something like an architecture or principles document in cozystack/cozystack. That's out of scope here, but please open an issue for it and link it from the proposal in section 2, so the gap is recorded rather than absorbed into this log by default.
To be explicit about cost: I'm asking for a restructure of a PR that's already complete, and I don't think that's free. But the move is mechanical now — one git mv, one prose fold, four link fixes — and it stops being mechanical once the log has ten entries and citations pointing at it from other repositories. Everything in section 3 is independent of the placement question and needs doing either way.
2 participants
This PR adds
decisions/for architectural decisions we already made, next to design proposals that propose them.Design proposal is intent, written before the work. When implementation contradicts design we revise proposal in place, so it reads as if it always said current thing, and reasoning that changed our minds stays only in PR diff.
It already happens here. #44 and #42 revised DHA to HPA-on-scale-subresource after spike, #53 reopened actuation because cnpg scale subresource has no selector, same shape in #40, #41, #36. Every one is implementation finding rewriting accepted proposal, with why left in PR body. compute-plane proposal even carries a
Revision (this PR):paragraph in its metadata because there was no other place to put that rationale.What is inside:
decisions/README.md- when to write a record and when not, numbering, statuses, immutability rule, review.decisions/template.md- Context / Decision / Why not the alternatives / Consequences / Revisit if.decisions/0001-computeplane-ships-as-an-operator-owned-module.md- backfilled from design-proposal: compute plane for untrusted-code workloads #17 to design-proposal(compute-plane): deliver as kind: Kubernetes presets, not a new kind (per #26 / #17 review) #27 to design-proposal(compute-plane): deliver as an operator-owned extra module wrapping apps/kubernetes (supersedes #27) #33, why ComputePlane ships as operator-owned module and not as preset fields onkind: Kubernetes.Root README gets a row in "Where do I file?", Proposal Drift now says to write a record instead of only editing proposal, and proposal template gets Decisions section.
Two rules carry this. Record is never edited after merge, it gets superseded by a new one. Review checks accuracy and not agreement, so one maintainer is enough, decision was already made and if review turns into re-litigating it then it was not settled and belongs in proposal instead.
Please check
Decidersline on 0001. I put Andrei Kvapil (@kvaps) and myself. Timofei Larkin review on #17 was half of the objection that drove the change but i did not want to guess handle, so he is named in text only.