Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/bundler/bundler.go
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,7 @@ func (b *DefaultBundler) buildDeployer(ctx context.Context, recipeResult *recipe
VendorCharts: b.Config.VendorCharts(),
ChartName: b.Config.BundleChartName(),
AppName: b.Config.AppName(),
OCIParentNamespace: b.Config.OCIParentNamespace(),
}, nil

case config.DeployerArgoCD:
Expand Down
21 changes: 21 additions & 0 deletions pkg/bundler/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,12 @@ type Config struct {
// targetRevision` triple resolves against the real artifact. See #1019.
bundleChartName string

// ociParentNamespace is the OCI registry + repository path with the chart-name
// segment stripped (e.g. "oci://ghcr.io/nvidia" for "oci://ghcr.io/nvidia/my-bundle:v1").
// Baked into the argocd-helm bundle's root values.yaml as the default repoURL.
// Empty when --output targets a local directory. See #1342.
ociParentNamespace string

// appName overrides the parent Argo Application's `metadata.name` for
// the argocd-helm and argocd deployers. Empty means each deployer
// applies its own default ("aicr-stack" / "nvidia-stack"). When two
Expand Down Expand Up @@ -459,6 +465,12 @@ func (c *Config) BundleChartName() string {
return c.bundleChartName
}

// OCIParentNamespace returns the OCI parent namespace baked into the
// argocd-helm bundle's root values.yaml as the default repoURL. See #1342.
func (c *Config) OCIParentNamespace() string {
return c.ociParentNamespace
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// AppName returns the parent Application name override for the argocd-helm
// and argocd deployers. Empty means "use the deployer's default". See #1011.
func (c *Config) AppName() string {
Expand Down Expand Up @@ -727,6 +739,15 @@ func WithBundleChartName(name string) Option {
}
}

// WithOCIParentNamespace sets the OCI parent namespace (registry + repo path
// without the chart segment) baked into the argocd-helm bundle's values.yaml
// as the default repoURL. Empty keeps repoURL as "" (local output). See #1342.
func WithOCIParentNamespace(ns string) Option {
return func(c *Config) {
c.ociParentNamespace = ns
}
}

// WithAppName sets the parent Argo Application's `metadata.name` for the
// argocd-helm and argocd deployers. Empty leaves the deployer's default
// in place. Required by operators deploying multiple non-overlapping
Expand Down
17 changes: 12 additions & 5 deletions pkg/bundler/deployer/argocdhelm/argocdhelm.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,12 @@ type Generator struct {
// is the multi-bundle collision fix — see issue #1011.
AppName string

// OCIParentNamespace is the OCI registry + repository path with the
// chart-name segment stripped. When set, written as the default repoURL
// in the bundle's root values.yaml so deploying from the push-target
// registry requires no --set flags. Empty for local-directory output. See #1342.
OCIParentNamespace string

Comment thread
coderabbitai[bot] marked this conversation as resolved.
// DynamicValues maps component names to their dynamic value paths.
DynamicValues map[string][]string

Expand Down Expand Up @@ -297,11 +303,12 @@ func (g *Generator) Generate(ctx context.Context, outputDir string) (*deployer.O
dynamicOnlyValues[rootValuesAppNameKey] = g.AppName
}

// 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] = ""
// Bake the OCI parent namespace as the repoURL default when the bundle was
// pushed to a registry — `helm show values` and a plain `helm install` both
// work without --set flags. For local output, OCIParentNamespace is "" and
// the {{ required }} safety-net is unchanged. See #1342.
repoURLDefault := g.OCIParentNamespace
dynamicOnlyValues[rootValuesRepoURLKey] = repoURLDefault

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocker — Golden tests fail: the targetRevision default was silently dropped

This block replaced two lines with one — it kept the repoURL write but deleted dynamicOnlyValues[rootValuesTargetRevisionKey] = "". That was the only writer of targetRevision into the root values.yaml, so the key now vanishes from generated output. The golden fixtures testdata/helm_and_manifest_only/values.yaml and testdata/mixed_component/values.yaml still contain the trailing targetRevision: "" line.

Blast radius: Reproduced: go test ./pkg/bundler/deployer/argocdhelm/... → FAIL: TestBundleGolden_HelmAndManifestOnly, FAIL: TestBundleGolden_MixedComponent (byte-diff = the missing targetRevision: "" line). make test / make qualify go red; the PR cannot merge. The PR description's "tests pass" claim does not hold.

Fix: Restore the line rather than regenerate goldens. Dropping targetRevision looks unintentional and regresses the explicit intent of #1020 ("surface both keys so helm show values documents them"). Re-adding dynamicOnlyValues[rootValuesTargetRevisionKey] = "" makes the goldens pass unchanged and keeps the values.yaml header (which still documents targetRevision) consistent. If the drop IS intentional, regenerate goldens with -update AND also fix the stale header (line 470-484) and the now-dead rootValuesTargetRevisionKey constant (line 126-134).

dynamicOnlyValues[rootValuesTargetRevisionKey] = ""

valuesPath, valuesSize, err := writeRootValuesFile(dynamicOnlyValues, outputDir)
Expand Down
47 changes: 47 additions & 0 deletions pkg/bundler/deployer/argocdhelm/argocdhelm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -987,6 +987,53 @@ func TestBundleGolden_MixedComponent(t *testing.T) {
}
}

// TestBundleGolden_OCI_BakesRepoURL verifies that when OCIParentNamespace is
// set the bundle's root values.yaml contains the parent namespace as the
// repoURL default, not an empty string. See #1342.
func TestBundleGolden_OCI_BakesRepoURL(t *testing.T) {
ctx := context.Background()
outputDir := t.TempDir()

rr := newRecipeResult("v1.0.0", []recipe.ComponentRef{
{
Name: "cert-manager",
Namespace: "cert-manager",
Chart: "cert-manager",
Version: "v1.20.2",
Type: recipe.ComponentTypeHelm,
Source: "https://charts.jetstack.io",
},
})
rr.DeploymentOrder = []string{"cert-manager"}

const wantNamespace = "oci://ghcr.io/myorg"

g := &Generator{
RecipeResult: rr,
ComponentValues: map[string]map[string]any{"cert-manager": {"replicaCount": 1}},
Version: "v0.0.0-golden",
TargetRevision: "v1.0.0",
OCIParentNamespace: wantNamespace,
}

if _, err := g.Generate(ctx, outputDir); err != nil {
t.Fatalf("Generate() error = %v", err)
}

valuesBytes, err := os.ReadFile(filepath.Join(outputDir, "values.yaml"))
if err != nil {
t.Fatalf("read values.yaml: %v", err)
}
values := string(valuesBytes)

if !strings.Contains(values, `repoURL: `+wantNamespace) {
t.Errorf("values.yaml missing baked repoURL; got:\n%s", values)
}
if strings.Contains(values, "repoURL: \"\"") {
t.Errorf("values.yaml should not contain empty repoURL when OCIParentNamespace is set; got:\n%s", values)
}
}

Comment thread
mohityadav8 marked this conversation as resolved.
// TestBundleGolden_ReadinessGate freezes the argocd-helm bundle output when a
// component ships a readiness gate. The delegated argocd.Generator emits a
// 002-<name>-readiness/ local-helm folder after the primary; the argocdhelm
Expand Down
28 changes: 14 additions & 14 deletions pkg/cli/bundle.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"io"
"log/slog"
"os"
"path"
"path/filepath"
"strings"

Expand Down Expand Up @@ -117,6 +116,12 @@ type bundleCmdOptions struct {
// deployer's "aicr-bundle" default is used when this is empty. See #1019.
bundleChartName string

// ociParentNamespace is the OCI registry + repository path with the
// chart segment stripped; used as the default repoURL baked into the
// argocd-helm bundle's values.yaml. Derived from ociRef when --output
// is an OCI reference; empty for local-directory output. See #1342.
ociParentNamespace string

// appName overrides the parent Argo Application's metadata.name. Empty
// means each deployer applies its own default ("aicr-stack" for
// argocd-helm, "nvidia-stack" for argocd). See #1011.
Expand Down Expand Up @@ -207,6 +212,9 @@ func parseBundleCmdOptions(cmd *cli.Command, cfg *appcfg.AICRConfig) (*bundleCmd
// resolves against an artifact that doesn't exist in the registry
// when the user picks a non-default name. See #1019.
opts.bundleChartName = opts.ociRef.ChartName()
// Derive the parent namespace (strip chart segment) so the argocd-helm
// deployer can bake it into values.yaml as the default repoURL. See #1342.
opts.ociParentNamespace = opts.ociRef.ParentNamespace()
} else {
// Resolve local output path to absolute to ensure consistent behavior
// regardless of how the binary is invoked.
Expand Down Expand Up @@ -837,6 +845,7 @@ func runBundleCmd(ctx context.Context, cmd *cli.Command) error {
config.WithOCISourceName(opts.ociSourceName),
config.WithFluxNamespace(opts.fluxNamespace),
config.WithBundleChartName(opts.bundleChartName),
config.WithOCIParentNamespace(opts.ociParentNamespace),
config.WithAppName(opts.appName),
)

Expand Down Expand Up @@ -1040,21 +1049,12 @@ func printArgoCDHelmOCIInstructions(w io.Writer, ref *oci.Reference) {

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
}
// Delegate to canonical derivation so baked repoURL and printed hint always match. See #1342.
repoURL := ref.ParentNamespace()
Comment thread
mohityadav8 marked this conversation as resolved.

fmt.Fprintf(w, "\nargocd-helm bundle pushed: %s\n", chartRef)
fmt.Fprintln(w, "\nTo install:")
fmt.Fprintf(w, " helm install <release> %s \\\n", chartRef)
fmt.Fprintln(w, " --namespace argocd \\")
fmt.Fprintf(w, " --set repoURL=%s\n", repoURL)
fmt.Fprintln(w, " --namespace argocd")
fmt.Fprintf(w, " # repoURL defaults to %s (override with --set repoURL=oci://mirror if mirroring)\n", repoURL)
}
14 changes: 14 additions & 0 deletions pkg/oci/reference.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,20 @@ func (r *Reference) ChartName() string {
return base
}

// ParentNamespace returns the OCI registry + repository path with the
// chart-name segment (last path element) stripped, prefixed with the
// oci:// scheme. Returns "" for non-OCI references. See #1342.
func (r *Reference) ParentNamespace() string {
if !r.IsOCI || r.Repository == "" {
return ""
}
parentPath := path.Dir(r.Repository)
if parentPath == "." || parentPath == "/" {
return URIScheme + r.Registry
}
return URIScheme + r.Registry + "/" + parentPath
}

// WithTag returns a copy of the reference with the specified tag.
// For non-OCI references, returns the same reference unchanged.
func (r *Reference) WithTag(tag string) *Reference {
Expand Down