docs: add ADR for deployment validation#630
Conversation
📝 WalkthroughWalkthroughThis pull request adds a new Architecture Decision Record (ADR-006) documenting the design for registry-driven deployment validation. The ADR specifies how the Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/006-deployment-validation.md`:
- Line 208: Replace the awkward phrasing "requires coordinated Go and registry
changes" at the sentence starting with "**Fixed special-case surface**" with a
clearer, concise alternative such as "requires coordinating Go and registry
changes" (or an equivalent like "requires coordinated changes to Go and the
registry") so the intent reads smoothly.
- Around line 87-120: Reword the sections that currently assert readiness
metadata is "hydrated onto each resolved ComponentRef" and list
`customChecks`/`crds` as runtime contract fields to clearly separate implemented
behavior from intended design: state that the YAML snippet in
`recipes/registry.yaml` and the described `readiness`/`customChecks`/`crds` are
the intended design/target contract (future intent), and explicitly note that
the current runtime schema (see `pkg/recipe/metadata.go` and the `Validation`
model) does not yet include readiness fields on `ComponentRef` and that
`RecipeResult` hydration is a planned capability; alternatively, if those fields
are already implemented elsewhere, update the references to point to the actual
implementation location. Apply the same clarification to the later block (lines
referenced as 223-237).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0dddf8b6-e6ce-4b5f-b9a1-9410a8b49801
📒 Files selected for processing (2)
docs/contributor/validator.mddocs/design/006-deployment-validation.md
| Deployment-phase deep checks are declared in `recipes/registry.yaml`, hydrated | ||
| onto each resolved `ComponentRef`, and interpreted by the deployment validator | ||
| using Kubernetes API state only. | ||
|
|
||
| ### Contract | ||
|
|
||
| The component readiness contract is: | ||
|
|
||
| ```yaml | ||
| components: | ||
| - name: kube-prometheus-stack | ||
| readiness: | ||
| namespace: monitoring | ||
| selector: | ||
| app.kubernetes.io/name: kube-prometheus-stack | ||
| workloads: | ||
| - kind: Deployment | ||
| name: kube-prometheus-operator | ||
| customChecks: [] | ||
| crds: [] | ||
| ``` | ||
|
|
||
| The supported coverage shapes are: | ||
|
|
||
| - `readiness` | ||
| - required `namespace` | ||
| - optional deployer-neutral `selector` | ||
| - optional exact `workloads` using `{kind, name}` | ||
| - sibling `customChecks` | ||
| - top-level `crds` | ||
|
|
||
| These fields are hydrated onto each resolved `ComponentRef` in `RecipeResult`. | ||
| The deployment validator reads only mounted `recipe.yaml`; it does not read | ||
| `registry.yaml` directly at runtime. |
There was a problem hiding this comment.
Clarify implemented vs target-state contract to avoid documenting unsupported schema as current behavior.
This ADR currently states as present fact that readiness metadata is “hydrated onto each resolved ComponentRef” and lists customChecks/crds contract fields as if already in the runtime schema, but the referenced recipe model shows Validation at recipe level and no readiness fields on ComponentRef (see pkg/recipe/metadata.go snippets). Please reword these sections to clearly separate accepted design intent from currently shipped structures/behavior (or update references if the schema exists elsewhere).
As per coding guidelines: “Documentation must not invent features, guarantees, timelines, or roadmap commitments. Clearly distinguish current behavior from future intent.”
Also applies to: 223-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/006-deployment-validation.md` around lines 87 - 120, Reword the
sections that currently assert readiness metadata is "hydrated onto each
resolved ComponentRef" and list `customChecks`/`crds` as runtime contract fields
to clearly separate implemented behavior from intended design: state that the
YAML snippet in `recipes/registry.yaml` and the described
`readiness`/`customChecks`/`crds` are the intended design/target contract
(future intent), and explicitly note that the current runtime schema (see
`pkg/recipe/metadata.go` and the `Validation` model) does not yet include
readiness fields on `ComponentRef` and that `RecipeResult` hydration is a
planned capability; alternatively, if those fields are already implemented
elsewhere, update the references to point to the actual implementation location.
Apply the same clarification to the later block (lines referenced as 223-237).
| full Chainsaw expressiveness or pod-phase parity. | ||
| - **Potential semantic overlap**: some repo-side Chainsaw health checks and the | ||
| deployment contract may cover similar ground separately. | ||
| - **Fixed special-case surface**: new bespoke behavior requires coordinated Go |
There was a problem hiding this comment.
Polish wording at Line 208 for clarity.
“requires coordinated Go and registry changes” is awkward; use “requires coordinating Go and registry changes” (or equivalent concise phrasing).
🧰 Tools
🪛 LanguageTool
[style] ~208-~208: The double modal “requires coordinated” is nonstandard (only accepted in certain dialects). Consider “to be coordinated”.
Context: ...urface**: new bespoke behavior requires coordinated Go and registry changes rather than a...
(NEEDS_FIXED)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/006-deployment-validation.md` at line 208, Replace the awkward
phrasing "requires coordinated Go and registry changes" at the sentence starting
with "**Fixed special-case surface**" with a clearer, concise alternative such
as "requires coordinating Go and registry changes" (or an equivalent like
"requires coordinated changes to Go and the registry") so the intent reads
smoothly.
|
Still reading this, but wanted to make a Note; when Chainsaw was added to the project before it was able to compile with go 1.26. This issue has been fixed, so we could import chainsaw and use it assert engine in our code base if that makes sense, vs building and maintaining that logic. |
|
Superseded by #631, opened from the yuanchen8911 fork branch as required. |
2 participants
Summary
Add ADR-006 to document the registry-driven deployment validation design and record why AICR standardizes on the Kubernetes-native pure-Go approach instead of reusing Chainsaw at validator runtime.
Motivation / Context
Issue
#622became large enough to warrant a durable architecture record. This ADR captures the finalized decision, the Go-based versus Chainsaw-based comparison, and the rationale for keepingaicr validatealigned with the existing pure-Go validator model used by conformance.Fixes: N/A
Related: #622
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/api,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)Implementation Notes
docs/design/006-deployment-validation.mdin the existing ADR format.aicr validateconsistent with the conformance phase and easier to extend toward broader CNCF AI Conformance validation.docs/contributor/validator.mdso the ADR is discoverable from the validator architecture docs.Testing
Local result: passed.
Risk Assessment
Rollout notes: N/A
Checklist
make testwith-race)make lint)git commit -S) — GPG signing info