chore(docs): catch up container-images.md BOM + document the regen rule#878
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds contributor-facing instructions to regenerate the container image BOM and regenerates the BOM documentation. docs/contributor/data.md, AGENTS.md, and .claude/CLAUDE.md now require running Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
5bdbaad to
8f9a6c9
Compare
78288d4 to
b111fc7
Compare
Two coupled changes:
1. Pure BOM regen of docs/user/container-images.md via `make bom-docs`
to reflect current registry + chart state. No code changes, no
registry pins moved; this picks up drift accumulated since main's
last BOM commit:
- slinky-slurm-operator and slinky-slurm-operator-crds BOM rows
added (registry entries landed in NVIDIA#866 without a corresponding
BOM regen)
- kubeflow-trainer's bundled pytorch image bumped upstream
- busybox bumped upstream in some chart's rendered templates
2. Add the BOM-regen rule to contributor and agent docs, surfaced by
the same NVIDIA#872 review that flagged the drift:
- .claude/CLAUDE.md (auto-synced to AGENTS.md) — short rule in the
"Common Tasks" section: after any change to recipes/registry.yaml,
a component values file, or a chart pin, run `make bom-docs` and
commit the regenerated container-images.md in the same PR
- docs/contributor/data.md — new "Regenerating the BOM" subsection
under Component Configuration covering when to run it, how it can
surface upstream chart drift, and how to handle unrelated drift
(split into a catch-up PR vs land it together)
- Makefile — corrected the bom-check target's help text from
"CI gate, opt-in locally" to "opt-in; not wired into qualify/
lint/merge gate" to match the actual enforcement story
Surfaced as an unrelated diff in NVIDIA#872 (aws-efa v0.5.26 bump) when
`make bom-docs` was rerun against the rebased branch. Splitting this
catch-up out so NVIDIA#872's diff is scoped to aws-efa content.
Honest enforcement story: an earlier revision of these docs (and an
earlier revision of this commit message) claimed `make qualify` and
CI catch stale BOMs. That was incorrect — verified directly against
the Makefile:
- `qualify` depends on test-coverage / lint / e2e / scan / license-
check, none of which run `bom-check`
- `lint` runs `bom-pinning-check` (chart-pin verification per ADR-006),
not `bom-check` (BOM doc freshness)
- The merge gate has no PR-time BOM-staleness check
The corrected docs say so explicitly. Wiring `bom-check` into the gate
is a desirable follow-up but intentionally out of scope here — it
would change CI behavior for every PR and likely block unrelated PRs
that happen to expose accumulated upstream drift, which deserves its
own discussion.
This also explains how NVIDIA#866 merged with a stale BOM: nothing in the
existing gate would have caught it.
Test plan:
- `make qualify` passes (Go tests, golangci-lint + yamllint, agents-
sync check, chart-pin verification, 20/20 chainsaw, vulnerability
scan, license headers)
- No code changes, no chart pins moved, no behavior change
b111fc7 to
b6343b7
Compare
Summary
Two coupled changes, surfaced by Mark's review on #872:
docs/user/container-images.mdviamake bom-docsto reflect current registry + chart state.Motivation / Context
Surfaced while rebasing #872 (aws-efa v0.5.26 bump) — running
make bom-docson the rebased branch pulled in three categories of drift that have accumulated since main's last BOM commit:slinky-slurm-operator+slinky-slurm-operator-crdsBOM rows addedpytorchimage bump (kubeflow-trainer's bundled image)busyboximage bumpThe fact that this accumulated without anyone noticing is itself the signal — the existing docs don't surface the regen requirement clearly enough. So this PR both lands the catch-up and updates the guidance.
Related: #866 (slinky-slurm registry entry), #872 (parent PR that triggered the discovery)
Type of Change
Component(s) Affected
docs/user/container-images.md(auto-generated section),docs/contributor/data.md,.claude/CLAUDE.md(auto-synced toAGENTS.md)Implementation Notes
File 1:
docs/user/container-images.md— puremake bom-docsregen against current registry + chart contents. The tool preserves the prose section; only the auto-generated section changes.File 2:
.claude/CLAUDE.md(auto-synced toAGENTS.md) — one-line rule under the "Common Tasks" section: after any change torecipes/registry.yaml, a component values file, or a chart pin (registry / overlay / mixin), runmake bom-docsand commit the regenerateddocs/user/container-images.mdin the same PR. Notes that the BOM is rendered fresh from each chart's actual templates, so even an unbumped pin can pick up upstream image drift — and thatmake bom-checkexists for verifying freshness but is opt-in only (not wired intomake qualify,make lint, or the merge gate today).File 3:
docs/contributor/data.md— new "Regenerating the BOM" subsection under "Component Configuration" covering:make bom-docs(registry add/remove, chart-pin bump, values-file change)make bom-checkexists but is opt-in; neithermake qualifynormake lintruns it, and the merge gate has no PR-time BOM-staleness check — contributors must runmake bom-docsexplicitlyHonest enforcement story. An earlier revision of these docs claimed
make qualifyand CI catch stale BOMs. That was incorrect (caught by external review). Verified directly against the Makefile:qualify: test-coverage lint e2e scan license-check— does not depend onbom-checklintrunsbom-pinning-check(chart-pin verification per ADR-006), notbom-check(BOM doc freshness)The corrected docs now say so explicitly. Wiring
bom-checkintomake qualify/make lint/ the merge gate is a desirable follow-up but intentionally out of scope here — it would change CI behavior for every PR and likely block unrelated PRs that happen to expose accumulated upstream drift, which deserves its own discussion. (Also flagged:Makefile:294's comment on thebom-checktarget still calls it "CI gate, opt-in locally" — corrected in this PR.)Testing
No code changes, no chart pins moved, no behavior change.
Risk Assessment
make bom-docsagainst current main; the rule additions are non-binding text.Rollout notes: None — pure content catch-up.
Checklist
make qualify)make lint)git commit -S)