Skip to content

fix(health): exempt manifest-only Helm components from chart_pinned#1303

Open
njhensley wants to merge 1 commit into
NVIDIA:mainfrom
njhensley:health/chart-pinned-manifest-only
Open

fix(health): exempt manifest-only Helm components from chart_pinned#1303
njhensley wants to merge 1 commit into
NVIDIA:mainfrom
njhensley:health/chart-pinned-manifest-only

Conversation

@njhensley

Copy link
Copy Markdown
Member

Summary

chart_pinned flagged manifest-only "Helm" components (typed Helm but shipping local manifestFiles with no external chart) as unpinned. This exempts them, since there is no chart version to pin.

Motivation / Context

Running the health generator against current main reported 17 of 32 recipes as fail — every one for the same reason:

chart_pinned: fail — unpinned Helm chart version for: nodewright-customizations

nodewright-customizations is a manifest-only component: overlays declare it type: Helm but it ships local manifestFiles with empty repository/chart/version. The merged classifyChartPinned (#1226) flags any enabled Helm ref with an empty Version as fail and did not exempt components with no external chart to pin — so this was a false-positive class, not a real pinning gap.

This matters because ADR-009 is emphatic that the public matrix must never mislead ("a stale public 'health' surface is worse than none"). Publishing a doc that says half the catalog fails — when it is really one known false-positive class — would do exactly that.

Fixes: N/A
Related: #1226, #1293

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Component(s) Affected

  • Other: recipe health signal (pkg/health)

Implementation Notes

  • Added isManifestOnlyHelm(ref) — a Helm-typed ref with empty Chart and Source but non-empty ManifestFiles/PreManifestFiles. This mirrors the bundler's canonical manifest-only detection (pkg/bundler/deployer/flux/flux.go: ref.Chart == "" && ref.Source == "" && hasManifests), keeping the health signal aligned with what is actually bundled and deployed.
  • classifyChartPinned now skips manifest-only refs the same way it skips disabled components: not counted toward helmCount, never added to the unpinned list.
  • Edge case: a recipe whose only enabled Helm component is manifest-only now reports the existing vacuous-pass "not applicable" detail (consistent with the disabled-only precedent). No current recipe hits this — the affected 17 all also carry real pinned charts.

Testing

go test ./pkg/health/       # ok
go vet ./pkg/health/...     # clean
gofmt -l pkg/health/*.go    # clean
  • Added three table cases to TestClassifyChartPinned: manifest-only alongside a pinned chart → pass; manifest-only not appearing in the unpinned list; manifest-only-only → vacuous "not applicable" pass. They fail before the change and pass after.
  • Verified real-catalog impact with a temporary probe against the embedded catalog (removed afterward): chart_pinned went from 17/32 fail → 0/32 fail.
  • -race could not run in the dev sandbox (no cgo/gcc) and golangci-lint is not on PATH locally; CI covers both.

Risk Assessment

  • Low — Isolated change to one scoring function, table-test covered, easy to revert.

Rollout notes: N/A — read-only health signal; no API, schema, or deployment behavior changes.

Checklist

  • Tests pass locally (make test with -race) — -race unavailable in sandbox (no cgo); go test ./pkg/health/ passes, CI runs -race
  • Linter passes (make lint) — golangci-lint not on PATH locally; go vet + gofmt clean, CI runs the pinned linter
  • I did not skip/disable tests to make CI green
  • I added/updated tests for new functionality
  • I updated docs if user-facing behavior changed (N/A — internal health signal)
  • Changes follow existing patterns in the codebase
  • Commits are cryptographically signed (git commit -S)

classifyChartPinned flagged any enabled Helm-typed component with an
empty Version as fail, with no exemption for components that have no
external chart to pin. nodewright-customizations is typed Helm but
ships local manifestFiles with empty chart/source/version, so it was
reported as an unpinned chart on all 17 recipes that include it — a
false positive against a non-existent pin.

Skip manifest-only Helm components (empty Chart and Source, manifests
present) the same way disabled components are skipped: not counted
toward helmCount, never added to the unpinned list. Detection mirrors
the bundler's manifest-only check, keeping the health signal aligned
with what is actually bundled and deployed.

Restores an honest public matrix per ADR-009: chart_pinned now reports
0/32 recipes failing (was 17/32).
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

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: bad87f61-b710-48a8-a2eb-600bb8cc3be6

📥 Commits

Reviewing files that changed from the base of the PR and between 6c14530 and d0a6eac.

📒 Files selected for processing (2)
  • pkg/health/health.go
  • pkg/health/health_test.go

📝 Walkthrough

Walkthrough

This PR extends the chart_pinned health check in pkg/health/health.go to recognize and skip Helm-typed components that are "manifest-only"—those with local manifest files but no external chart or source to pin. A new isManifestOnlyHelm helper encapsulates the detection logic, and classifyChartPinned uses it to avoid false failures for components that cannot have an external chart version. Documentation and three new test cases verify the behavior.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • NVIDIA/aicr#1293: Introduces the graded chart_pinned signal and its classification logic, which this PR extends with a manifest-only component skip guard.

Suggested labels

area/recipes, theme/recipes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: exempting manifest-only Helm components from the chart_pinned check. It is concise, clear, and directly related to the primary fix in the changeset.
Description check ✅ Passed The description comprehensively explains the issue (false-positive flagging), motivation (ADR-009 compliance), implementation details (isManifestOnlyHelm helper), testing approach, and risk assessment. It is directly related to the changeset and provides meaningful context.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant