Skip to content

fix(bundler): v0.12 rc1 smoke-test follow-ups#677

Merged
mchmarny merged 1 commit into
mainfrom
fix/minor-testing-finds-0.12.rc1
Apr 24, 2026
Merged

fix(bundler): v0.12 rc1 smoke-test follow-ups#677
mchmarny merged 1 commit into
mainfrom
fix/minor-testing-finds-0.12.rc1

Conversation

@lockwobr
Copy link
Copy Markdown
Contributor

Summary

Fix three minor issues found during 0.12.rc1 testing: wrong Skyhook taint
domain in docs/tests, missing cluster-values.yaml in generated helm
command snippets, and silent fallthrough when a --dynamic path is absent
from component values.

Motivation / Context

Shaking out the 0.12.rc1 bundle output surfaced three small but user-visible
defects:

  1. Wrong Skyhook taint domain. The docs and tests still referenced the
    legacy skyhook.io/runtime-required key. The NVIDIA Skyhook operator
    uses skyhook.nvidia.com/runtime-required, so copy-pasting the
    documented --workload-gate value produced a taint no Skyhook install
    would ever match.
  2. Generated helm commands omitted cluster-values.yaml. The Helm
    deployer writes dynamic paths into a separate cluster-values.yaml, but
    the rendered README templates only referenced values.yaml. Users who
    declared --dynamic paths and then ran the command from the README
    would silently lose those install-time overrides.
  3. Silent placeholder injection. When --dynamic <component>:<path>
    names a path that isn't present in the component's values, both the
    helm and argocd-helm deployers fall back to writing an empty string
    stub. That fallback is load-bearing (empty declarations are legitimate),
    but producing it without any operator-visible signal made typos in
    dynamic paths impossible to diagnose from the bundle alone.

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/api, 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)
  • Docs/examples (docs/, examples/)
  • Other: ____________

Implementation Notes

Testing

# Commands run (prefer `make qualify` for non-trivial changes)
make qualify

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:

Checklist

  • Tests pass locally (make test with -race)
  • Linter passes (make lint)
  • 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
  • Changes follow existing patterns in the codebase
  • Commits are cryptographically signed (git commit -S) — GPG signing info

@mchmarny mchmarny enabled auto-merge (squash) April 24, 2026 22:30
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 24, 2026

Coverage Report ✅

Metric Value
Coverage 74.7%
Threshold 70%
Status Pass
Coverage Badge
![Coverage](https://img.shields.io/badge/coverage-74.7%25-green)

Merging this branch will increase overall coverage

Impacted Packages Coverage Δ 🤖
github.com/NVIDIA/aicr/pkg/bundler/deployer/argocdhelm 72.27% (+0.12%) 👍
github.com/NVIDIA/aicr/pkg/bundler/deployer/helm 87.63% (+0.07%) 👍

Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/NVIDIA/aicr/pkg/bundler/deployer/argocdhelm/argocdhelm.go 72.27% (+0.12%) 238 (+1) 172 (+1) 66 👍
github.com/NVIDIA/aicr/pkg/bundler/deployer/helm/helm.go 87.63% (+0.07%) 186 (+1) 163 (+1) 23 👍

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

@coderabbitai

This comment was marked as resolved.

@lockwobr lockwobr changed the title ix(bundler): v0.12 rc1 smoke-test follow-ups fix(bundler): v0.12 rc1 smoke-test follow-ups Apr 24, 2026
@lockwobr lockwobr force-pushed the fix/minor-testing-finds-0.12.rc1 branch from f1c84f8 to 33792a1 Compare April 24, 2026 22:34
@lockwobr lockwobr force-pushed the fix/minor-testing-finds-0.12.rc1 branch from 33792a1 to 3315615 Compare April 24, 2026 22:34
@mchmarny mchmarny merged commit f032582 into main Apr 24, 2026
46 checks passed
@mchmarny mchmarny deleted the fix/minor-testing-finds-0.12.rc1 branch April 24, 2026 22:44
lockwobr added a commit that referenced this pull request Apr 28, 2026
Co-authored-by: Mark Chmarny <mchmarny@users.noreply.github.com>
lockwobr added a commit that referenced this pull request Apr 28, 2026
Co-authored-by: Mark Chmarny <mchmarny@users.noreply.github.com>
lockwobr added a commit that referenced this pull request Apr 28, 2026
Co-authored-by: Mark Chmarny <mchmarny@users.noreply.github.com>
lockwobr added a commit that referenced this pull request Apr 28, 2026
Co-authored-by: Mark Chmarny <mchmarny@users.noreply.github.com>
lockwobr added a commit that referenced this pull request Apr 28, 2026
Co-authored-by: Mark Chmarny <mchmarny@users.noreply.github.com>
lockwobr added a commit that referenced this pull request Apr 29, 2026
Co-authored-by: Mark Chmarny <mchmarny@users.noreply.github.com>
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.

2 participants