docs(defaults): correct MirrorDefaultKubeVersion rationale comment

[yuanchen8911]

Summary

Corrects the rationale comment on defaults.MirrorDefaultKubeVersion, which claimed the value tracks the project's minimum supported Kubernetes version. It does not, and must not. Comment-only; the value is unchanged and there is no behavior change.

Motivation / Context

The comment says:

The value tracks the project's minimum supported Kubernetes version declared in recipes/overlays/base.yaml.

recipes/overlays/base.yaml declares K8s.server.version: ">= 1.25", while the constant is 1.33.0. Read literally, the comment invites a maintainer to "fix" the mismatch by lowering the constant to 1.25 — which would break the exact thing the constant exists to prevent.

Two sentences earlier, the same comment already explains why: without --kube-version, Helm falls back to its compiled-in v1.27.0, which is too old for charts declaring kubeVersion: >=1.32.0-0. So 1.33.0 is correct and the rationale is the part that is wrong.

The constant is a render-safe fallback for helm template --kube-version, read at three sites:

• pkg/helm/render.go:117 — when RenderInput.KubeVersion is empty
• pkg/mirror/discover.go:80 — when WithKubeVersion is unset
• pkg/mirror/discover.go:546 — KubeVersionFromConstraints, when the recipe declares no K8s.server.version constraint

That last one matters: a recipe that does declare the constraint never reaches this default — its own value is used. So the constant is not a floor for anything; it is what mirror/BOM discovery renders with when the recipe is silent.

Found while reviewing #2335. Pre-existing and untouched by that PR, so it is split out here rather than widening a docs-only diff.

Relationship to #2335. That PR unpins a stale "Kubernetes 1.33+" claim from the AGENTS.md / .claude/CLAUDE.md Tech Stack line, which matched nothing in the tree. This comment is the likely origin of that claim — the same 1.33 mis-described as a support floor. #2335 removes the bad statement from the docs; this PR stops the code comment from regenerating it. The two are independent: they share no files and can merge in either order.

Fixes: N/A
Related: #2335

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Refactoring (no functional changes)
• Build/CI/tooling

Component(s) Affected

• CLI (cmd/aicr, pkg/cli)
• API server (cmd/aicrd, pkg/server)
• Recipe engine / data (pkg/recipe)
• Bundlers (pkg/bundler, pkg/component/*)
• Collectors / snapshotter (pkg/collector, pkg/snapshotter)
• Validator (pkg/validator)
• Core libraries (pkg/errors, pkg/k8s) — pkg/defaults
• Docs/examples (docs/, examples/)
• Other: ____________

Implementation Notes

The replacement comment does three things the old one did not:

1. Names what the value actually is — a render-safe fallback, not a support floor — and states the real invariant: it must stay at or above the strictest kubeVersion any bundled chart declares.
2. Explicitly warns against lowering it to base.yaml's ">= 1.25". This is the load-bearing sentence; without it the next person to notice the mismatch makes the same wrong inference the old comment invited.
3. Points at mirror.KubeVersionFromConstraints so the reader can see that a constraint-carrying recipe bypasses the default entirely.

The constant's value is deliberately left at 1.33.0.

Testing

golangci-lint run -c .golangci.yaml ./pkg/defaults/... ./pkg/mirror/... ./pkg/helm/...
# 0 issues.

go build ./pkg/defaults/... ./pkg/mirror/... ./pkg/helm/...
# ok

go test -race ./pkg/defaults/... ./pkg/mirror/... ./pkg/helm/...
# ok  github.com/NVIDIA/aicr/pkg/defaults  1.874s
# ok  github.com/NVIDIA/aicr/pkg/mirror    1.852s
# ok  github.com/NVIDIA/aicr/pkg/helm      1.294s

Full make qualify was not run: the diff is a single Go comment with no token change to any expression, so tests, e2e, and coverage cannot regress from it. The mandatory Go lint gate was run on the changed package and both consumers, and both packages' tests were run with -race as a sanity check.

Coverage gate: not applicable — no exported symbol added or changed, and no statement is added or removed.

Risk Assessment

• Low — Isolated change, well-tested, easy to revert
• Medium — Touches multiple components or has broader impact
• High — Breaking change, affects critical paths, or complex rollout

Rollout notes: N/A — comment text only; the compiled binary is unchanged.

Checklist

• Tests pass locally (make test with -race) — run scoped to the changed package and its two consumers
• Linter passes (make lint) — golangci-lint on ./pkg/defaults/..., ./pkg/mirror/..., ./pkg/helm/...: 0 issues
• I did not skip/disable tests to make CI green
• I added/updated tests for new functionality — N/A, comment-only
• I updated docs if user-facing behavior changed — N/A, no behavior change
• Changes follow existing patterns in the codebase
• Commits are cryptographically signed (git commit -S)

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 9653df2e-8f73-4bbd-9664-7257e5bc5f2e

📥 Commits

Reviewing files that changed from the base of the PR and between 735a311 and aed801d.

📒 Files selected for processing (1)

• pkg/defaults/timeouts.go

Included review availability: Your plan provides up to 12 included reviews per hour; 8 remain after this review.

---

📝 Walkthrough

Walkthrough

Updated the MirrorDefaultKubeVersion documentation. It now defines a render-safe fallback that must satisfy the strictest bundled chart kubeVersion constraint. The constant value is unchanged.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to aed80

This is a localized documentation-only correction with no compiled behavior change or actionable merge-blocking risk remaining.

Suggested reviewers: njhensley, mchmarny

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the documentation change and the corrected rationale for MirrorDefaultKubeVersion.
Description check	✅ Passed	The description accurately explains the comment-only change, its motivation, scope, testing, and unchanged behavior.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[njhensley]

📋 Multi-Persona Review — Approve

Method: 2 independent persona reviewers (Correctness + Domain, Docs) → adversarial senior meta-review, each finding re-derived from the resolved code.

Legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick

Overall assessment

Approve. This is a +7/−2 comment-only change to pkg/defaults/timeouts.go — no token of any expression changes and the constant stays "1.33.0". It replaces two actively-misleading lines ("tracks the project's minimum supported Kubernetes version") with an accurate rationale ("render-safe fallback, not a support floor"). Every literal claim in the new comment was verified against the code:

• render.go:117 — falls back to the default when input.KubeVersion == "" ✓
• discover.go:80 (NewLister) — falls back when l.kubeVersion == "" ✓
• discover.go:546 — KubeVersionFromConstraints returns the default only when no K8s.server.version constraint is present ✓
• recipes/overlays/base.yaml:24-25 declares >= 1.25; the constant is 1.33.0 ✓
• Referenced mirror.KubeVersionFromConstraints exists and is exported (discover.go:540) ✓

The old comment genuinely invited the wrong fix — lowering the constant to 1.25, which would break charts declaring kubeVersion: >=1.32.0-0. This PR closes that trap. Nothing survives above nitpick tier; the four inline 🔵 notes are all discretionary (three are explicit "leave as-is").

Confirmed non-issues

• The v1.27.0 in Helm 3.x Helm-default line — pre-existing, outside this diff.
• Godoc form — the comment remains well-formed and starts with the identifier name, matching sibling constants in the block.
• Line length — all lines ≤ 73 chars.

Summary

🔴 Blocker	🟠 Major	🟡 Minor	🔵 Nitpick	Recommendation
0	0	0	4	Approve — comment-only; strictly improves accuracy

[yuanchen8911]

Addressed the review and rebased onto main (the branch went BEHIND once #2335 merged, and the gate requires up-to-date branches).

Force-push: f010725d2 → faf815870. The rebase was gate-required, so inline anchors from the previous head may show as outdated — apologies for the churn.

L1258 (invariant under-scoped) — fixed, and thank you, the finding is correct. I reproduced it: extractVersion(">= 1.25") returns "1.25", so a recipe carrying only base.yaml's floor renders with --kube-version 1.25, below what the sentence asserted. The invariant is now scoped explicitly to the constant, plus an added note that the constraint path is unbounded below, so the comment cannot be misread as a guarantee about rendering in general.

L1259 (all-caps "Do NOT"), L1262 (mild restatement), L1263 (rename-coupling) — left as-is per your recommendation on each.

The unbounded-below extraction behaviour itself is untouched here, as you noted it is out of scope for a comment PR. Flagging it for a separate issue rather than folding it in.

[yuanchen8911]

@njhensley nits addressed — please take another look when you get a chance.

Your approval was dismissed automatically by the push (dismiss_stale_reviews_on_push), so this needs a fresh one. Summary of what changed since f010725d2:

• L1258 — fixed. Your finding was right and I reproduced it: extractVersion(">= 1.25") returns "1.25", so a base-only recipe renders --kube-version 1.25, below what the sentence claimed. The invariant is now scoped explicitly to the constant, plus a note that the constraint path is unbounded below.
• L1259 / L1262 / L1263 — left as-is per your recommendation on each.
• Rebased onto main after docs: doc sweep for stale references, casing, and index gaps #2335 merged (the gate requires up-to-date branches). Force-push f010725d2 → faf815870; two of your inline anchors went outdated as a result, so I replied on each thread individually rather than only here.

All four threads have inline replies and are resolved. The unbounded-below extractVersion behaviour is untouched, per your out-of-scope call — I will file it separately.

[yuanchen8911]

Rebased onto main (the branch had fallen behind 06d2cbbad; the gate requires up-to-date branches).

Force-push: faf815870 → 3bf7702b5. Catch-up rebase only — I verified git diff origin/main...HEAD is byte-identical before and after, so no content changed. Inline anchors from the previous head may show as outdated as a result.

[mchmarny]

Approve: no findings against db1ada7. Two exact-head checks are still running; completed checks are passing.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.