feat(mirror): add mirror command#967
Merged
Merged
Conversation
Contributor
Author
|
some outputs of the new command: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements the Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 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: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/setup-build-tools/action.yml:
- Around line 107-126: The action currently hardcodes defaults for
hauler_version, zarf_version, and ctlptl_version which duplicates
.settings.yaml; remove those default values (set defaults to empty) and add
validation so when install_zarf or install_ctlptl (and any similar install
toggles) is true the action fails fast if the corresponding version input
(zarf_version, ctlptl_version, hauler_version) is empty - this ensures versions
must be injected by the load-versions step and preserves .settings.yaml as the
single source of truth.
In @.github/workflows/mirror-e2e.yaml:
- Around line 20-39: Update the mirror-e2e workflow's path filters: in the
mirror-e2e workflow's paths arrays (both the push and pull_request entries)
extend the existing patterns referenced under the paths key so they also include
the repo areas that affect mirror behavior such as mirror-list dependencies and
version-pin sources; modify the paths arrays under the pull_request and push
sections (the paths key in the mirror-e2e workflow) to add patterns that cover
shared rendering/loader code and version pin files so these changes will trigger
the E2E job.
In `@pkg/cli/mirror.go`:
- Around line 160-183: Validate the --format flag as soon as it's read from cmd
(via mirror.Format(cmd.String("format"))) and call isValidMirrorFormat before
calling resolveRecipeForMirror or parsing --set overrides; if invalid, return
the same errors.New(...) message referencing mirror.SupportedFormats()
immediately so you avoid running resolveRecipeForMirror(ctx, cmd, cfg) and
config.ParseValueOverrides(raw) when the format itself is wrong.
In `@pkg/helm/render.go`:
- Line 125: Add a default timeout fallback before calling exec.CommandContext so
the external helm I/O is always bounded: define a defaultTimeout (e.g. 30s),
check the incoming ctx's deadline via ctx.Deadline(); if there is no deadline,
create ctx, cancel := context.WithTimeout(ctx, defaultTimeout) and replace the
ctx passed to exec.CommandContext, if there is a parent deadline compute
remaining := time.Until(deadline) and only wrap with context.WithTimeout(ctx,
defaultTimeout) when remaining > defaultTimeout (otherwise keep the original
ctx) and ensure you call defer cancel() when you create a new cancel function;
update the site where cmd := exec.CommandContext(ctx, "helm", args...) is
invoked to use the possibly-wrapped ctx.
- Around line 62-65: Replace plain fmt.Errorf uses in RenderChart and its helper
code with pkg/errors methods: import "github.com/pkg/errors" and use
errors.Errorf(...) for standalone formatted errors (e.g., the validation check
in RenderChart when input.Chart == ""), and use errors.Wrapf(err, "...") or
errors.Wrap(err, "...") to wrap underlying errors returned from functions such
as temp-file handling, subprocess (helm) failures, or cleanup operations (the
other fmt.Errorf occurrences referenced). Ensure each error includes contextual
message text and wraps the original err where one exists.
In `@pkg/mirror/discover_test.go`:
- Around line 43-343: Add two regression test rows to TestDiscover: one that
verifies context cancellation returns an error by using a blocking
mockHelmRenderer (e.g., a renderer that waits on a channel) and calling
lister.Discover with a pre-cancelled or soon-to-be-cancelled context to assert
Discover returns an error; and another that verifies typed override semantics by
adding a component whose Overrides map sets "enabled": false (boolean) and
asserting the component is skipped (expect wantImages/wantCharts/wantComps = 0)
— use existing helpers/types like mockHelmRenderer, NewLister, WithHelmRenderer,
Discover, and recipe.ComponentRef to place these cases so they exercise
concurrent rendering and override mutation path.
In `@pkg/mirror/discover.go`:
- Around line 302-305: The overridesByKey function currently swallows errors
from recipe.GetComponentRegistry() and returns nil; change it to fail closed by
propagating the error instead of returning nil: when GetComponentRegistry()
returns a non-nil error or a nil registry, return that error (or wrap it with
context) up the call chain so callers know the registry lookup failed; update
overridesByKey’s signature and all callers as needed to accept/handle the error,
and ensure any logging includes context like "overridesByKey: failed to get
component registry" while referencing recipe.GetComponentRegistry() and
overridesByKey in the change.
- Around line 139-165: The Render error handling in the loop around
l.helmRenderer.Render should detect context cancellation/deadline errors (use
errors.Is(renderErr, context.Canceled) or errors.Is(renderErr,
context.DeadlineExceeded) or check gctx.Err()) and stop/propagate the error
instead of turning it into a warning; update the block where renderErr is
handled (around l.helmRenderer.Render, compRef, and ci.Warnings) to return the
context error immediately (or bubble it up) so discovery fails fast on
cancellation, and also add a gctx.Done() check before continuing loops (the
manifest-processing loops that call extractManifestImages) to avoid processing
after cancellation.
- Around line 266-279: The current setNestedValue always stores override values
as strings, causing booleans/numbers/null to become quoted strings; update
setNestedValue (used by applyValueOverrides) to parse the incoming string into
the correct scalar type before storing: detect "null" -> nil, try
strconv.ParseBool for booleans, then strconv.ParseInt/ParseFloat for
integers/floats, and fall back to the original string if none match, then assign
current[part] = parsedValue instead of the raw string so templates receive
proper scalar types.
In `@pkg/mirror/helm.go`:
- Around line 67-69: Replace the direct call to errors.Wrap in
pkg/mirror/helm.go with errors.PropagateOrWrap to preserve any existing
StructuredError codes: instead of returning errors.Wrap(errors.ErrCodeInternal,
err.Error(), err) return the error using errors.PropagateOrWrap(err,
errors.ErrCodeInternal, "contextual message describing the helm mirror operation
failed") so the original code is preserved when present and you avoid
duplicating err.Error(); update the return in the function that returns out and
err accordingly.
- Around line 57-63: The code calls helm.RenderChart with fields from ref
(ref.Name, ref.Chart, ref.Source, ref.Version, ref.Namespace) without checking
ref for nil; add a nil guard at the start of the Render function (or immediately
before the helm.RenderChart call) that returns a clear error if ref == nil, and
only proceed to build helm.ChartInput and call helm.RenderChart when ref is
non-nil.
In `@tools/mirror-e2e`:
- Around line 76-77: The check_tools() function currently calls has_tools with
make, kind, kubectl, crane, yq, helm, ctlptl, goreleaser but omits python3 and
docker; update check_tools() to include both python3 and docker in the has_tools
invocation so the script validates those prerequisites before proceeding (refer
to the check_tools() function and the has_tools call to locate where to add
"python3" and "docker").
- Around line 209-216: The current sed strips any `@sha256`:... suffix
unconditionally (in the public_images assignment and the similar block around
verify_registry()), corrupting digest-only refs; update the sed so it only
removes `@sha256`:... when there is a preceding tag (i.e. a ":" before the "@").
Replace the unconditional sed 's/@sha256:[a-f0-9]*//' with a regex that
preserves digest-only refs, for example use sed -E
's/\(:[^@]*\)`@sha256`:[a-f0-9]*/\1/' (apply the same fix to the other block) so
public_images remains correct and verify_registry() sees the original
digest-only refs.
🪄 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: Enterprise
Run ID: 7e70da8c-84ef-43bb-bdfa-70b8486c71f5
📒 Files selected for processing (26)
.github/actions/load-versions/action.yml.github/actions/setup-build-tools/action.yml.github/workflows/mirror-e2e.yaml.settings.yamlMakefiledocs/index.ymldocs/user/air-gap-mirror.mddocs/user/cli-reference.mdpkg/cli/mirror.gopkg/cli/mirror_test.gopkg/cli/root.gopkg/defaults/timeouts.gopkg/helm/render.gopkg/helm/render_test.gopkg/mirror/discover.gopkg/mirror/discover_test.gopkg/mirror/format_hauler.gopkg/mirror/format_hauler_test.gopkg/mirror/format_test.gopkg/mirror/format_zarf.gopkg/mirror/format_zarf_test.gopkg/mirror/helm.gopkg/mirror/mirror.gotools/bom/helm.gotools/bom/registry.gotools/mirror-e2e
💤 Files with no reviewable changes (1)
- tools/bom/registry.go
Contributor
|
🌿 Preview your docs: https://nvidia-preview-feature-mirror.docs.buildwithfern.com/aicr |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/setup-tools`:
- Around line 675-733: The optional installers for hauler and zarf can cause the
whole script to exit on failures (due to set -euo pipefail); modify the hauler
and zarf install blocks so that after verify_download_url succeeds the
subsequent steps (curl download, tar/extract, checksum verify, mv/chmod) are
executed inside guarded conditionals or a try-style sequence that catches any
error and logs a warning rather than letting the script exit. Concretely, wrap
the HAULER sequence (using HAULER_TMP, HAULER_TAR, HAULER_URL) in an if ... then
... else ... or use a subshell/command-group with || { log_warning "..."; rm -rf
"${HAULER_TMP}"; } to handle failures, and do the same for the ZARF sequence
(ZARF_TMP, ZARF_BIN, ZARF_BASE, verify_sha256) so failures trigger log_warning
and cleanup but do not abort the script.
🪄 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: Enterprise
Run ID: caab44cd-1f9c-4688-8d2e-7018b9bd908e
📒 Files selected for processing (3)
docs/user/air-gap-mirror.mdtools/check-toolstools/setup-tools
haarchri
force-pushed
the
feature/mirror
branch
2 times, most recently
from
9203cef to
d47b792
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
.github/actions/setup-build-tools/action.yml (1)
372-446:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail fast when
hauler_version,zarf_version, orctlptl_versionis empty.These install steps should explicitly validate required version inputs before URL construction; otherwise failures are noisy and non-obvious.
Suggested patch
- name: Install Hauler @@ run: | set -euo pipefail if [[ -x /usr/local/bin/hauler ]]; then echo "hauler already installed"; hauler version; exit 0; fi + if [[ -z "${HAULER_VERSION}" ]]; then + echo "::error::hauler_version is required when install_hauler=true" + exit 1 + fi VERSION="${HAULER_VERSION#v}" @@ - name: Install Zarf @@ run: | set -euo pipefail if [[ -x /usr/local/bin/zarf ]]; then echo "zarf already installed"; zarf version; exit 0; fi + if [[ -z "${ZARF_VERSION}" ]]; then + echo "::error::zarf_version is required when install_zarf=true" + exit 1 + fi VERSION="${ZARF_VERSION#v}" @@ - name: Install ctlptl @@ run: | set -euo pipefail if [[ -x /usr/local/bin/ctlptl ]]; then echo "ctlptl already installed"; ctlptl version; exit 0; fi + if [[ -z "${CTLPTL_VERSION}" ]]; then + echo "::error::ctlptl_version is required when install_ctlptl=true" + exit 1 + fi BASE="https://github.com/tilt-dev/ctlptl/releases/download/v${CTLPTL_VERSION}"As per coding guidelines,
.settings.yamlis the single source of truth for tool versions and workflow tooling should enforce that contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/setup-build-tools/action.yml around lines 372 - 446, The install steps for Hauler, Zarf and ctlptl use HAULER_VERSION, ZARF_VERSION and CTLPTL_VERSION without validating them, causing unclear failures; add an explicit non-empty check at the start of each install block (the "Install Hauler", "Install Zarf", and "Install ctlptl" steps) and emit a clear error and exit non-zero if the corresponding env var is empty (e.g., check HAULER_VERSION before computing VERSION and BASE/TAR, ZARF_VERSION before building BIN, and CTLPTL_VERSION before constructing BASE/TAR) so the workflow fails fast with a descriptive message that points to the missing input.pkg/mirror/discover.go (1)
173-182:⚠️ Potential issue | 🟠 MajorCheck
gctxinside the manifest-file loops, not just once before them.After the one-time guard on Line 174, Lines 177-182 will keep reading and parsing every manifest file even if discovery is canceled midway through the slice. Add a
gctx.Err()check in each iteration, and ideally thread the context intoextractManifestImages, so cancellation stops promptly.As per coding guidelines:
Check context in loops — Always check ctx.Done() in long-running operations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/mirror/discover.go` around lines 173 - 182, The loop currently checks gctx.Err() only once before iterating manifest slices, so cancellation won't stop mid-iteration; update both loops over compRef.ManifestFiles and compRef.PreManifestFiles to check gctx.Err() inside each iteration (e.g. if gctx.Err() != nil { return gctx.Err() }) and pass the context into extractManifestImages by adding a ctx/gctx parameter to its signature and call sites so extractManifestImages can also abort promptly on cancellation.tools/setup-tools (1)
675-733:⚠️ Potential issue | 🟠 MajorDon't make Hauler/Zarf install failures fatal.
Lines 679-699 and 708-732 still abort the whole setup flow even though this script advertises both tools as optional for mirror E2E. With
set -euo pipefail, a failed download, checksum, extract, or install here blocks general developer setup instead of lettingtools/mirror-e2eskip the corresponding test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/setup-tools` around lines 675 - 733, The Hauler and Zarf install blocks (around the HAULER_* and ZARF_* code paths) currently call exit 1 on failures and let commands like curl/tar/verify_sha256 abort under set -e, making optional tools fatal; change both blocks so failures are non-fatal by catching/handling errors: replace immediate exit 1 and fatal command behavior with guarded checks and fallback logging (use log_warning/log_error but continue), ensure temporary dirs (HAULER_TMP/ZARF_TMP) are cleaned on failure, wrap risky commands (curl, tar, verify_sha256, sudo mv) to detect failure and skip installation without exiting so tools/mirror-e2e can detect absence and skip tests, and keep verify_download_url usage but do not call exit on its failure.tools/mirror-e2e (1)
213-216:⚠️ Potential issue | 🟠 MajorVerify digest-only refs without forcing
:tagsyntax.Lines 213-216 now preserve digest-only refs, but Lines 392-395 still parse every image as
name:tag. A public image likeghcr.io/org/app@sha256:...will therefore be checked aslocalhost:5001/org/app@sha256:<digest>, which is not a valid registry reference and will make the E2E fail on digest-only discoveries.Also applies to: 392-395
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/mirror-e2e` around lines 213 - 216, The parsing logic that later coerces every image into name:tag is breaking digest-only refs (e.g., ghcr.io/org/app@sha256:...), so update the image-normalization code (the block that processes public_images and the later parsing at lines handling per-image rewrite, referenced by the public_images variable and the image-parsing code around the second block) to detect and preserve digest-only references containing "`@sha256`:" instead of forcing a :tag form; if an image contains "`@sha256`:" leave the `@sha256`:<digest> suffix intact when constructing the local target (do not append or substitute a :tag), otherwise continue to normalize tagless or tagged names as before. Ensure both the initial selection (public_images) and the later rewriting/parsing logic (the block currently treating every image as name:tag) apply the same digest-aware behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/mirror-e2e.yaml:
- Around line 20-47: The workflow’s path filters in
.github/workflows/mirror-e2e.yaml are missing the action used by the job; update
both the push and pull_request "paths" arrays to include the prep-kind-runner
action by adding the pattern '.github/actions/prep-kind-runner/**' so changes to
that action will trigger the Mirror E2E workflow; ensure the new entry is added
alongside the existing '.github/actions/load-versions/**' and
'.github/actions/setup-build-tools/**' entries in both sections.
In `@docs/user/cli-reference.md`:
- Around line 1931-1937: Add blank lines above and below the shell fenced block
containing "aicr mirror list [flags]" and also add a blank line before the Flags
table so the code fence and table are separated from surrounding text; ensure
the fenced block uses proper triple backticks and that the Flags table
(including the `--recipe` row) has a blank line before it to satisfy
MD031/MD058. Target the fenced block with text "aicr mirror list [flags]" and
the following "**Flags:**" table when making the edits.
In `@pkg/mirror/discover.go`:
- Around line 211-213: The code currently replaces any returned error from
g.Wait() (and similarly later) with errors.ErrCodeInternal, losing
structured/context error kinds; update the error handling to use
errors.PropagateOrWrap(err, errors.ErrCodeInternal, "component discovery
failed") instead of errors.Wrap so that existing structured errors (e.g., those
coming from pkg/recipe.GetComponentRegistry()) and cancellation are preserved or
propagated, and apply the same change at the second occurrence (the block around
the later g.Wait()); reference the g.Wait() call and the GetComponentRegistry()
call when making the replacement.
In `@pkg/mirror/mirror.go`:
- Around line 110-125: Render currently emits "null" for JSON/YAML when list ==
nil but other code paths treat nil as an error; update Render (function Render,
type MirrorList) to validate the input at the top (if list == nil) and return a
wrapped invalid-request error instead of proceeding (use the existing errors
package, e.g., errors.ErrCodeInvalid combined with a descriptive message) so
FormatJSON, FormatYAML and any other format branches behave consistently for nil
input.
In `@tools/mirror-e2e`:
- Around line 143-148: The recipe generation invocation using "$AICR_BIN" recipe
should be made hermetic by passing the test-safety flag --no-cluster; update the
command that calls "$AICR_BIN" recipe (the call that currently uses --service
kind --accelerator h100 --intent training --os ubuntu --output
"${WORK}/recipe.yaml") to include --no-cluster so the command does not consult
the host kubeconfig or external clusters during E2E runs.
---
Duplicate comments:
In @.github/actions/setup-build-tools/action.yml:
- Around line 372-446: The install steps for Hauler, Zarf and ctlptl use
HAULER_VERSION, ZARF_VERSION and CTLPTL_VERSION without validating them, causing
unclear failures; add an explicit non-empty check at the start of each install
block (the "Install Hauler", "Install Zarf", and "Install ctlptl" steps) and
emit a clear error and exit non-zero if the corresponding env var is empty
(e.g., check HAULER_VERSION before computing VERSION and BASE/TAR, ZARF_VERSION
before building BIN, and CTLPTL_VERSION before constructing BASE/TAR) so the
workflow fails fast with a descriptive message that points to the missing input.
In `@pkg/mirror/discover.go`:
- Around line 173-182: The loop currently checks gctx.Err() only once before
iterating manifest slices, so cancellation won't stop mid-iteration; update both
loops over compRef.ManifestFiles and compRef.PreManifestFiles to check
gctx.Err() inside each iteration (e.g. if gctx.Err() != nil { return gctx.Err()
}) and pass the context into extractManifestImages by adding a ctx/gctx
parameter to its signature and call sites so extractManifestImages can also
abort promptly on cancellation.
In `@tools/mirror-e2e`:
- Around line 213-216: The parsing logic that later coerces every image into
name:tag is breaking digest-only refs (e.g., ghcr.io/org/app@sha256:...), so
update the image-normalization code (the block that processes public_images and
the later parsing at lines handling per-image rewrite, referenced by the
public_images variable and the image-parsing code around the second block) to
detect and preserve digest-only references containing "`@sha256`:" instead of
forcing a :tag form; if an image contains "`@sha256`:" leave the `@sha256`:<digest>
suffix intact when constructing the local target (do not append or substitute a
:tag), otherwise continue to normalize tagless or tagged names as before. Ensure
both the initial selection (public_images) and the later rewriting/parsing logic
(the block currently treating every image as name:tag) apply the same
digest-aware behavior.
In `@tools/setup-tools`:
- Around line 675-733: The Hauler and Zarf install blocks (around the HAULER_*
and ZARF_* code paths) currently call exit 1 on failures and let commands like
curl/tar/verify_sha256 abort under set -e, making optional tools fatal; change
both blocks so failures are non-fatal by catching/handling errors: replace
immediate exit 1 and fatal command behavior with guarded checks and fallback
logging (use log_warning/log_error but continue), ensure temporary dirs
(HAULER_TMP/ZARF_TMP) are cleaned on failure, wrap risky commands (curl, tar,
verify_sha256, sudo mv) to detect failure and skip installation without exiting
so tools/mirror-e2e can detect absence and skip tests, and keep
verify_download_url usage but do not call exit on its failure.
🪄 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: Enterprise
Run ID: 473c72ec-85de-4ff7-bb58-f760e8819760
📒 Files selected for processing (30)
.github/actions/load-versions/action.yml.github/actions/setup-build-tools/action.yml.github/workflows/mirror-e2e.yaml.settings.yamlMakefiledocs/index.ymldocs/user/air-gap-mirror.mddocs/user/cli-reference.mdpkg/cli/mirror.gopkg/cli/mirror_test.gopkg/cli/root.gopkg/component/overrides.gopkg/component/overrides_test.gopkg/defaults/timeouts.gopkg/helm/render.gopkg/helm/render_test.gopkg/mirror/discover.gopkg/mirror/discover_test.gopkg/mirror/format_hauler.gopkg/mirror/format_hauler_test.gopkg/mirror/format_test.gopkg/mirror/format_zarf.gopkg/mirror/format_zarf_test.gopkg/mirror/helm.gopkg/mirror/mirror.gotools/bom/helm.gotools/bom/registry.gotools/check-toolstools/mirror-e2etools/setup-tools
💤 Files with no reviewable changes (1)
- tools/bom/registry.go
lockwobr
requested changes
May 20, 2026
Contributor
lockwobr
left a comment
lockwobr
left a comment
There was a problem hiding this comment.
Two themes in this review:
- Zarf output completeness —
apiVersionis omitted and the file'sv1beta1comment doesn't match upstream. Inline below. - Smaller code-level items — OCI URL drift, unbounded helm stdout, trust-model docs note, one nit. Inline below.
Separately, posted a design comment on #949 with two directions worth weighing for the --mirror-registry follow-up (containerd-level redirect vs. bundle emitting the air-gap artifacts directly).
lockwobr
reviewed
May 20, 2026
Contributor
lockwobr
left a comment
lockwobr
left a comment
There was a problem hiding this comment.
One additional refactor suggestion separated from the main review so it can be discussed independently — moving the HelmRenderer interface from pkg/mirror into pkg/helm. Details inline.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/mirror/discover_test.go (1)
17-26: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse the shared default kube-version constant in these assertions.
The hard-coded
"1.33.0"duplicates production config, so a legitimate change todefaults.MirrorDefaultKubeVersionwill fail this test even when behavior is correct.Suggested fix
import ( "context" "testing" "time" + "github.com/NVIDIA/aicr/pkg/defaults" "github.com/NVIDIA/aicr/pkg/errors" "github.com/NVIDIA/aicr/pkg/helm" "github.com/NVIDIA/aicr/pkg/helm/helmtest" "github.com/NVIDIA/aicr/pkg/recipe" ) @@ { name: "no constraints returns default", constraints: nil, - want: "1.33.0", + want: defaults.MirrorDefaultKubeVersion, }, { name: "no k8s constraint returns default", constraints: []recipe.Constraint{ {Name: "worker-os", Value: "ubuntu"}, }, - want: "1.33.0", + want: defaults.MirrorDefaultKubeVersion, },As per coding guidelines, “Use named constants from
pkg/defaultsinstead of magic literals for timeouts, limits, and configuration values”.Also applies to: 305-316
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/mirror/discover_test.go` around lines 17 - 26, The test uses a hard-coded kube version string ("1.33.0") which duplicates production config; update the assertions in discover_test.go to reference the shared constant defaults.MirrorDefaultKubeVersion instead of the literal so tests follow the coding guideline and will stay correct if the default changes; locate the assertions around the test helper/verify calls (and the additional occurrences noted later in the file) and replace the literal with defaults.MirrorDefaultKubeVersion in each assertion or expected value.
♻️ Duplicate comments (2)
pkg/mirror/discover_test.go (1)
247-296: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd non-string override cases to
TestSetNestedValue.
setNestedValuenow depends oncomponent.ConvertMapValue, but the table only asserts string leaves. Afalseandnullcase would lock in the behavior that matters for Helm conditionals and prevent regressing back to truthy strings.Suggested fix
tests := []struct { name string path string value string initial map[string]any expected any }{ { name: "simple key", path: "key", value: "val", initial: map[string]any{}, expected: "val", }, + { + name: "boolean value", + path: "enabled", + value: "false", + initial: map[string]any{}, + expected: false, + }, { name: "nested key", path: "a.b.c", value: "deep", initial: map[string]any{}, expected: "deep", }, + { + name: "null value", + path: "image.tag", + value: "null", + initial: map[string]any{}, + expected: nil, + }, { name: "override existing", path: "driver.version", value: "new", initial: map[string]any{"driver": map[string]any{"version": "old"}}, expected: "new", }, }As per coding guidelines, “Test error cases: Test error conditions and edge cases explicitly”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/mirror/discover_test.go` around lines 247 - 296, The test TestSetNestedValue only asserts string leaf values but setNestedValue now uses component.ConvertMapValue so add non-string override cases (e.g., boolean false and nil) to the tests: extend the tests slice in TestSetNestedValue with cases like path "flag" value false and path "maybe" value nil (or interface{}(nil)) with appropriate initial maps and expected types/values, then ensure the walking/assertion logic (using splitPath) checks the actual typed value (not string equality) so the test will catch regressions where ConvertMapValue returns a truthy string instead of the original boolean or nil; keep using setNestedValue and splitPath to locate the leaf.pkg/mirror/discover.go (1)
177-186:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-check cancellation inside both manifest loops.
After the pre-loop guard, discovery can still keep reading
ManifestFilesandPreManifestFilesifgctxis canceled mid-scan. That does extra I/O and can still assemble partial results for a canceled command.Suggested fix
if gctx.Err() != nil { return gctx.Err() } for _, mPath := range compRef.ManifestFiles { + if err := gctx.Err(); err != nil { + return err + } allImages = extractManifestImages(allImages, &ci, compRef.Name, mPath) } for _, mPath := range compRef.PreManifestFiles { + if err := gctx.Err(); err != nil { + return err + } allImages = extractManifestImages(allImages, &ci, compRef.Name, mPath) }As per coding guidelines, “Check context in loops — Always check
ctx.Done()in long-running operations”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/mirror/discover.go` around lines 177 - 186, The loop over compRef.ManifestFiles and compRef.PreManifestFiles can continue after gctx is canceled; update the loops in discover.go to check gctx.Err()/gctx.Done() inside each iteration and break/return early when canceled so no further I/O or extraction occurs; specifically, inside the loop that calls extractManifestImages (for both ManifestFiles and PreManifestFiles) add a context cancellation check (using gctx.Err() or select on gctx.Done()) before calling extractManifestImages and stop iterating/return the context error if canceled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/bom/main_test.go`:
- Around line 441-475: The test TestRenderHelmComponentWithValuesFile currently
only checks that output is non-empty; update it to assert the values file path
was passed into the renderer by inspecting the mock renderer's captured input
(use helmtest.MockRenderer's captured fields such as the last render request or
ValuesFiles slice) after calling renderHelmComponent; specifically assert that
the expected values file path filepath.Join(root, "recipes", "components",
"gpu-operator", "values.yaml") appears in the mock's recorded inputs (while
keeping the existing warnings and output checks around renderHelmComponent).
In `@tools/bom/main.go`:
- Around line 171-173: The current probe of values.yaml treats any os.Stat error
as “missing” by setting valuesPath = "", which hides permission/IO errors;
change the os.Stat handling so that if err != nil you check os.IsNotExist(err)
and only clear valuesPath in that case, otherwise surface the error
(log/fatal/return) so real IO/permission problems aren't suppressed — locate the
os.Stat(valuesPath) call and variable valuesPath in tools/bom/main.go and
replace the blanket suppression with an explicit os.IsNotExist check and error
propagation for non-missing errors.
- Around line 166-168: The function renderHelmComponent currently creates a new
root context with context.Background(), breaking cancellation chaining; change
its signature to accept a parent context (e.g., ctx context.Context) and derive
the timeout-bound context with context.WithTimeout(ctx, defaultHelmTimeout) and
defer cancel(), and update callers (including surveyComponent) to pass their
caller context through so cancellations propagate; ensure any internal uses of
context.Background() are removed and the same pattern is applied where
surveyComponent calls renderHelmComponent so the parent context is threaded
end-to-end.
---
Outside diff comments:
In `@pkg/mirror/discover_test.go`:
- Around line 17-26: The test uses a hard-coded kube version string ("1.33.0")
which duplicates production config; update the assertions in discover_test.go to
reference the shared constant defaults.MirrorDefaultKubeVersion instead of the
literal so tests follow the coding guideline and will stay correct if the
default changes; locate the assertions around the test helper/verify calls (and
the additional occurrences noted later in the file) and replace the literal with
defaults.MirrorDefaultKubeVersion in each assertion or expected value.
---
Duplicate comments:
In `@pkg/mirror/discover_test.go`:
- Around line 247-296: The test TestSetNestedValue only asserts string leaf
values but setNestedValue now uses component.ConvertMapValue so add non-string
override cases (e.g., boolean false and nil) to the tests: extend the tests
slice in TestSetNestedValue with cases like path "flag" value false and path
"maybe" value nil (or interface{}(nil)) with appropriate initial maps and
expected types/values, then ensure the walking/assertion logic (using splitPath)
checks the actual typed value (not string equality) so the test will catch
regressions where ConvertMapValue returns a truthy string instead of the
original boolean or nil; keep using setNestedValue and splitPath to locate the
leaf.
In `@pkg/mirror/discover.go`:
- Around line 177-186: The loop over compRef.ManifestFiles and
compRef.PreManifestFiles can continue after gctx is canceled; update the loops
in discover.go to check gctx.Err()/gctx.Done() inside each iteration and
break/return early when canceled so no further I/O or extraction occurs;
specifically, inside the loop that calls extractManifestImages (for both
ManifestFiles and PreManifestFiles) add a context cancellation check (using
gctx.Err() or select on gctx.Done()) before calling extractManifestImages and
stop iterating/return the context error if canceled.
🪄 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: Enterprise
Run ID: 0619b7f1-ce8a-4ec1-b2b0-c22384ca7d32
📒 Files selected for processing (13)
docs/user/air-gap-mirror.mdpkg/helm/helmtest/mock.gopkg/helm/render.gopkg/mirror/discover.gopkg/mirror/discover_test.gopkg/mirror/format_hauler.gopkg/mirror/format_hauler_test.gopkg/mirror/format_zarf.gopkg/mirror/format_zarf_test.gopkg/mirror/mirror.gotools/bom/helm.gotools/bom/main.gotools/bom/main_test.go
💤 Files with no reviewable changes (1)
- tools/bom/helm.go
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
… recipes Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add
aicr mirror listsubcommand for air-gapped image and chart discovery, with Hauler and Zarf output formats and end-to-end documentation.Motivation / Context
Air-gapped Kubernetes deployments need to know every container image and Helm chart a recipe references before they can mirror them into a private registry. This PR adds the discovery pipeline and integrates with the two most common air-gap tools (Hauler, Zarf).
Fixes: #949
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/)pkg/mirror/(new),pkg/helm/(new),pkg/bom/(image extraction),tools/bom/(BOM generator now sharespkg/helm/),tools/mirror-e2e(new E2E script),.github/workflows/mirror-e2e.yaml(new CI workflow)Implementation Notes
Testing
Risk Assessment
Rollout notes:
New subcommand only (
aicr mirror list). No changes to existing commands or APIs.pkg/helm/extraction is an internal refactor —tools/bom/produces identical output.Checklist
make testwith-race)make lint)git commit -S) — GPG signing info