diff --git a/pkg/bundler/deployer/argocdhelm/argocdhelm.go b/pkg/bundler/deployer/argocdhelm/argocdhelm.go index 2e819a3a5..b1fa09890 100644 --- a/pkg/bundler/deployer/argocdhelm/argocdhelm.go +++ b/pkg/bundler/deployer/argocdhelm/argocdhelm.go @@ -90,6 +90,7 @@ import ( "github.com/NVIDIA/aicr/pkg/component" "github.com/NVIDIA/aicr/pkg/errors" "github.com/NVIDIA/aicr/pkg/recipe" + "github.com/NVIDIA/aicr/pkg/serializer" ) // yamlStringTag is the YAML resolved-tag for explicit scalar strings, used @@ -122,6 +123,16 @@ const DefaultAppName = "aicr-stack" // root values.yaml, and any documentation that references it. const rootValuesAppNameKey = "appName" +// rootValuesRepoURLKey and rootValuesTargetRevisionKey are surfaced in +// the root values.yaml with empty defaults so `helm show values` +// documents them. The parent App template's {{ required }} directive +// still enforces non-empty at render time — empty strings here only +// affect documentation surface, not runtime behavior. +const ( + rootValuesRepoURLKey = "repoURL" + rootValuesTargetRevisionKey = "targetRevision" +) + // compile-time interface check var _ deployer.Deployer = (*Generator)(nil) @@ -276,7 +287,14 @@ func (g *Generator) Generate(ctx context.Context, outputDir string) (*deployer.O dynamicOnlyValues[rootValuesAppNameKey] = g.AppName } - valuesPath, valuesSize, err := deployer.WriteValuesFile(dynamicOnlyValues, outputDir, "values.yaml") + // Surface repoURL and targetRevision in values.yaml so `helm show + // values` documents the install-time inputs. Empty defaults — the + // parent App template's {{ required }} directive still enforces + // non-empty at render time. See issue #1020. + dynamicOnlyValues[rootValuesRepoURLKey] = "" + dynamicOnlyValues[rootValuesTargetRevisionKey] = "" + + valuesPath, valuesSize, err := writeRootValuesFile(dynamicOnlyValues, outputDir) if err != nil { return nil, errors.Wrap(errors.ErrCodeInternal, "failed to write root values.yaml", err) } @@ -428,6 +446,58 @@ func (g *Generator) writeStaticValuesAndBuildStubs(outputDir string) ([]string, return files, totalSize, dynamicOnlyValues, nil } +// rootValuesInstallHeader documents the install-time inputs surfaced +// in the bundle's root values.yaml. The empty defaults below let +// `helm show values` list the keys; the parent App template's +// {{ required }} directive enforces non-empty at render time. See +// issue #1020. +const rootValuesInstallHeader = `# Generated by AICR +# +# Install-time inputs (required for argocd-helm bundles): +# +# repoURL Parent OCI namespace or Helm chart repo URL — WITHOUT +# the chart name. The parent App template appends +# .Chart.Name itself to assemble the full reference. +# Example: --set repoURL=oci://ghcr.io/nvidia +# +# targetRevision Chart version / OCI artifact tag. +# Defaults to .Chart.Version when unset at install time. +# Example: --set targetRevision=v1.0.0 +# +--- +` + +// writeRootValuesFile writes the bundle's root values.yaml with a +// documentation header that explains the required install-time inputs. +// Parallels deployer.WriteValuesFile, with a richer header tailored to +// the argocd-helm contract. +func writeRootValuesFile(values map[string]any, outputDir string) (string, int64, error) { + outputPath, err := deployer.SafeJoin(outputDir, "values.yaml") + if err != nil { + return "", 0, err + } + + var buf strings.Builder + buf.WriteString(rootValuesInstallHeader) + + if len(values) > 0 { + // Deterministic marshal so values.yaml feeding checksums.txt + // (and the bundle attestation) is byte-stable across runs. + yamlBytes, err := serializer.MarshalYAMLDeterministic(values) + if err != nil { + return "", 0, errors.PropagateOrWrap(err, errors.ErrCodeInternal, "failed to marshal values") + } + buf.Write(yamlBytes) + } + + content := buf.String() + if err := os.WriteFile(outputPath, []byte(content), 0600); err != nil { + return "", 0, errors.Wrap(errors.ErrCodeInternal, "failed to write values file", err) + } + + return outputPath, int64(len(content)), nil +} + // parentAppTemplate is the Helm template body for the parent Argo // Application. It lives inside the chart's templates/ directory so that // the user supplies repoURL and targetRevision at install time via diff --git a/pkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/values.yaml b/pkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/values.yaml index 990715949..544ea54c6 100644 --- a/pkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/values.yaml +++ b/pkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/values.yaml @@ -1,4 +1,18 @@ # Generated by AICR +# +# Install-time inputs (required for argocd-helm bundles): +# +# repoURL Parent OCI namespace or Helm chart repo URL — WITHOUT +# the chart name. The parent App template appends +# .Chart.Name itself to assemble the full reference. +# Example: --set repoURL=oci://ghcr.io/nvidia +# +# targetRevision Chart version / OCI artifact tag. +# Defaults to .Chart.Version when unset at install time. +# Example: --set targetRevision=v1.0.0 +# --- certmanager: replicaCount: 1 +repoURL: "" +targetRevision: "" diff --git a/pkg/bundler/deployer/argocdhelm/testdata/mixed_component/values.yaml b/pkg/bundler/deployer/argocdhelm/testdata/mixed_component/values.yaml index c87de7f4a..8091b5a2b 100644 --- a/pkg/bundler/deployer/argocdhelm/testdata/mixed_component/values.yaml +++ b/pkg/bundler/deployer/argocdhelm/testdata/mixed_component/values.yaml @@ -1,5 +1,19 @@ # Generated by AICR +# +# Install-time inputs (required for argocd-helm bundles): +# +# repoURL Parent OCI namespace or Helm chart repo URL — WITHOUT +# the chart name. The parent App template appends +# .Chart.Name itself to assemble the full reference. +# Example: --set repoURL=oci://ghcr.io/nvidia +# +# targetRevision Chart version / OCI artifact tag. +# Defaults to .Chart.Version when unset at install time. +# Example: --set targetRevision=v1.0.0 +# --- gpuoperator: driver: version: "580" +repoURL: "" +targetRevision: "" diff --git a/pkg/cli/bundle.go b/pkg/cli/bundle.go index aae2578d4..77bc1b477 100644 --- a/pkg/cli/bundle.go +++ b/pkg/cli/bundle.go @@ -20,6 +20,7 @@ import ( "io" "log/slog" "os" + "path" "path/filepath" "strings" @@ -711,6 +712,15 @@ func runBundleCmd(ctx context.Context, cmd *cli.Command) error { if err := pushOCIBundle(ctx, opts, out); err != nil { return err } + // argocd-helm is the only deployer that publishes a Helm chart + // artifact consumers run `helm install` against; for everyone + // else the OCI artifact is a generic AICR bundle with its own + // distribution path. Surface the canonical install line so + // users don't have to derive it from the registry URL + the + // chart's {{ required }} error. See issue #1020. + if opts.deployer == config.DeployerArgoCDHelm { + printArgoCDHelmOCIInstructions(cmd.Root().Writer, opts.ociRef) + } } return nil @@ -823,3 +833,42 @@ func printDeploymentInstructions(w io.Writer, out *result.Output) { } } } + +// printArgoCDHelmOCIInstructions emits the canonical `helm install` line +// for an argocd-helm bundle pushed to OCI. The post-#1051 contract is: +// +// - `helm install` targets the full OCI artifact reference +// (oci:///:, including the chart name). +// - `--set repoURL` carries the PARENT namespace only — the chart's +// parent App template appends .Chart.Name itself to assemble the +// final reference, so the chart name must NOT be included in +// `repoURL`. +// +// Skips silently for non-OCI references; the OCI-mode call site in +// runBundleCmd already gates on opts.ociRef != nil, this guard is +// defense-in-depth for callers that may construct ociRef differently. +func printArgoCDHelmOCIInstructions(w io.Writer, ref *oci.Reference) { + if ref == nil || !ref.IsOCI { + return + } + + chartRef := fmt.Sprintf("%s%s/%s:%s", oci.URIScheme, ref.Registry, ref.Repository, ref.Tag) + + // Parent namespace: the registry + repository path with the chart + // (last) segment stripped. path.Dir returns "." for a single-segment + // repo (e.g., "aicr-bundle"); in that case the parent is just the + // registry, with no path component. + parentPath := path.Dir(ref.Repository) + var repoURL string + if parentPath == "." || parentPath == "/" { + repoURL = oci.URIScheme + ref.Registry + } else { + repoURL = oci.URIScheme + ref.Registry + "/" + parentPath + } + + fmt.Fprintf(w, "\nargocd-helm bundle pushed: %s\n", chartRef) + fmt.Fprintln(w, "\nTo install:") + fmt.Fprintf(w, " helm install %s \\\n", chartRef) + fmt.Fprintln(w, " --namespace argocd \\") + fmt.Fprintf(w, " --set repoURL=%s\n", repoURL) +} diff --git a/pkg/cli/bundle_test.go b/pkg/cli/bundle_test.go index 00ebd3043..9771a03e7 100644 --- a/pkg/cli/bundle_test.go +++ b/pkg/cli/bundle_test.go @@ -15,6 +15,7 @@ package cli import ( + "bytes" "context" "os" "path/filepath" @@ -22,6 +23,7 @@ import ( "testing" "github.com/NVIDIA/aicr/pkg/bundler/attestation" + "github.com/NVIDIA/aicr/pkg/oci" ) // TestParseOutputTarget is now in pkg/oci/reference_test.go @@ -311,3 +313,111 @@ func TestParseBundleCmdOptions_AppName(t *testing.T) { } }) } + +// TestPrintArgoCDHelmOCIInstructions exercises the post-#1051 install-hint +// contract: `helm install` against the full OCI artifact reference, and +// `--set repoURL` carrying only the parent namespace (chart name omitted +// — the chart template appends .Chart.Name itself). See issue #1020. +func TestPrintArgoCDHelmOCIInstructions(t *testing.T) { + tests := []struct { + name string + ref *oci.Reference + wantContain []string + wantSkip bool // true means no output expected + }{ + { + name: "registry with nested namespace", + ref: &oci.Reference{ + IsOCI: true, + Registry: "ghcr.io", + Repository: "nvidia/aicr-bundle", + Tag: "v1.0.0", + }, + wantContain: []string{ + "oci://ghcr.io/nvidia/aicr-bundle:v1.0.0", + "--set repoURL=oci://ghcr.io/nvidia", + "--namespace argocd", + }, + }, + { + name: "deeply nested namespace", + ref: &oci.Reference{ + IsOCI: true, + Registry: "registry.example.com", + Repository: "team/platform/aicr-bundle", + Tag: "0.42.0", + }, + wantContain: []string{ + "oci://registry.example.com/team/platform/aicr-bundle:0.42.0", + "--set repoURL=oci://registry.example.com/team/platform", + }, + }, + { + name: "single-segment repo: parent collapses to registry-only", + ref: &oci.Reference{ + IsOCI: true, + Registry: "localhost:5000", + Repository: "aicr-bundle", + Tag: "dev", + }, + wantContain: []string{ + "oci://localhost:5000/aicr-bundle:dev", + "--set repoURL=oci://localhost:5000", + }, + }, + { + name: "nil ref skips silently", + ref: nil, + wantSkip: true, + }, + { + name: "non-OCI ref skips silently", + ref: &oci.Reference{ + IsOCI: false, + LocalPath: "./bundle", + }, + wantSkip: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf bytes.Buffer + printArgoCDHelmOCIInstructions(&buf, tt.ref) + got := buf.String() + + if tt.wantSkip { + if got != "" { + t.Errorf("expected no output, got: %q", got) + } + return + } + for _, want := range tt.wantContain { + if !strings.Contains(got, want) { + t.Errorf("output missing %q\nfull output:\n%s", want, got) + } + } + // Sanity check: the repoURL line must NOT include the chart + // name segment — the chart template appends .Chart.Name itself, + // and a chart-name-bearing repoURL would cause double-append at + // render time. + if tt.ref != nil && tt.ref.IsOCI { + chartName := tt.ref.ChartName() + if chartName != "" { + // The repoURL line is the last one; isolate it to avoid + // false positives from the chartRef line above. + var repoURLLine string + for line := range strings.SplitSeq(got, "\n") { + if strings.Contains(line, "--set repoURL=") { + repoURLLine = line + break + } + } + if strings.Contains(repoURLLine, "/"+chartName) { + t.Errorf("repoURL must not include chart name %q; got line: %q", chartName, repoURLLine) + } + } + } + }) + } +}