From 5157db170b7c64bd1d4b9041e238523966cb1667 Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Sat, 14 Mar 2026 08:28:41 +0100 Subject: [PATCH 01/14] Feat | App of Apps Support Tests | Add appofapps_test.go and fix makeChildManifest helper Docs | Add App of Apps --- cmd/main.go | 34 +- cmd/options.go | 14 + docs/app-of-apps.md | 110 +++++ integration-test/branch-17/target/output.html | 168 +++++++ integration-test/branch-17/target/output.md | 87 ++++ integration-test/integration_test.go | 20 + mkdocs.yml | 1 + pkg/reposerverextract/appofapps.go | 434 ++++++++++++++++++ pkg/reposerverextract/appofapps_test.go | 418 +++++++++++++++++ 9 files changed, 1276 insertions(+), 10 deletions(-) create mode 100644 docs/app-of-apps.md create mode 100644 integration-test/branch-17/target/output.html create mode 100644 integration-test/branch-17/target/output.md create mode 100644 pkg/reposerverextract/appofapps.go create mode 100644 pkg/reposerverextract/appofapps_test.go diff --git a/cmd/main.go b/cmd/main.go index 3f346a2f..1d135f8e 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -320,16 +320,30 @@ func run(cfg *Config) error { // Extract resources by streaming source files directly to the Argo CD repo server via gRPC. // This bypasses the cluster reconciliation loop used by extract.RenderApplicationsFromBothBranches. - baseManifests, targetManifests, extractDuration, err = reposerverextract.RenderApplicationsFromBothBranches( - argocd, - baseBranch, - targetBranch, - cfg.Timeout, - cfg.Concurrency, - baseApps.SelectedApps, - targetApps.SelectedApps, - cfg.Repo, - ) + if cfg.TraverseAppOfApps { + baseManifests, targetManifests, extractDuration, err = reposerverextract.RenderApplicationsFromBothBranchesWithAppOfApps( + argocd, + baseBranch, + targetBranch, + cfg.Timeout, + cfg.Concurrency, + baseApps.SelectedApps, + targetApps.SelectedApps, + cfg.Repo, + appSelectionOptions, + ) + } else { + baseManifests, targetManifests, extractDuration, err = reposerverextract.RenderApplicationsFromBothBranches( + argocd, + baseBranch, + targetBranch, + cfg.Timeout, + cfg.Concurrency, + baseApps.SelectedApps, + targetApps.SelectedApps, + cfg.Repo, + ) + } } else { // Extract resources from the cluster based on each branch, passing the manifests directly deleteAfterProcessing := !cfg.CreateCluster diff --git a/cmd/options.go b/cmd/options.go index 290778ad..86d20a2b 100644 --- a/cmd/options.go +++ b/cmd/options.go @@ -82,6 +82,7 @@ var ( DefaultArgocdConfigPath = "./argocd-config" DefaultOutputAppManifests = false DefaultOutputBranchManifests = false + DefaultTraverseAppOfApps = false ) // RawOptions holds the raw CLI/env inputs - used only for parsing @@ -130,6 +131,7 @@ type RawOptions struct { Concurrency uint `mapstructure:"concurrency"` OutputAppManifests bool `mapstructure:"output-app-manifests"` OutputBranchManifests bool `mapstructure:"output-branch-manifests"` + TraverseAppOfApps bool `mapstructure:"traverse-app-of-apps"` } // Config is the final, validated, ready-to-use configuration @@ -173,6 +175,7 @@ type Config struct { Concurrency uint OutputAppManifests bool OutputBranchManifests bool + TraverseAppOfApps bool // Parsed/processed fields - no "parsed" prefix needed FileRegex *regexp.Regexp @@ -268,6 +271,7 @@ func Parse() *Config { viper.SetDefault("argocd-config-dir", DefaultArgocdConfigPath) viper.SetDefault("output-app-manifests", DefaultOutputAppManifests) viper.SetDefault("output-branch-manifests", DefaultOutputBranchManifests) + viper.SetDefault("traverse-app-of-apps", DefaultTraverseAppOfApps) // Basic flags rootCmd.Flags().BoolP("debug", "d", false, "Activate debug mode") @@ -325,6 +329,7 @@ func Parse() *Config { rootCmd.Flags().String("argocd-ui-url", DefaultArgocdUIURL, "Argo CD URL to generate application links in diff output (e.g., https://argocd.example.com)") rootCmd.Flags().Bool("output-app-manifests", DefaultOutputAppManifests, "Write per-application manifest files to the output folder (output/base/ and output/target/)") rootCmd.Flags().Bool("output-branch-manifests", DefaultOutputBranchManifests, "Write all application manifests per branch to a single file (output/base-branch.yaml and output/target-branch.yaml)") + rootCmd.Flags().Bool("traverse-app-of-apps", DefaultTraverseAppOfApps, "Recursively render child Applications discovered in rendered manifests (app-of-apps pattern). Only supported with --render-method=repo-server-api") // Check if version flag was specified directly for _, arg := range os.Args[1:] { @@ -410,6 +415,7 @@ func (o *RawOptions) ToConfig() (*Config, error) { Concurrency: o.Concurrency, OutputAppManifests: o.OutputAppManifests, OutputBranchManifests: o.OutputBranchManifests, + TraverseAppOfApps: o.TraverseAppOfApps, } var err error @@ -461,6 +467,11 @@ func (o *RawOptions) ToConfig() (*Config, error) { } } + // --traverse-app-of-apps is only supported with the repo-server-api render method + if cfg.TraverseAppOfApps && cfg.RenderMethod != RenderMethodRepoServerAPI { + return nil, fmt.Errorf("--traverse-app-of-apps requires --render-method=repo-server-api (current: %s)", cfg.RenderMethod) + } + // Check if argocd CLI is installed when not using API mode if cfg.RenderMethod == RenderMethodCLI && !cfg.DryRun { if _, err := exec.LookPath("argocd"); err != nil { @@ -736,4 +747,7 @@ func (o *Config) LogConfig() { if o.OutputBranchManifests { log.Info().Msgf("✨ - output-branch-manifests: %t", o.OutputBranchManifests) } + if o.TraverseAppOfApps { + log.Info().Msgf("✨ - traverse-app-of-apps: %t", o.TraverseAppOfApps) + } } diff --git a/docs/app-of-apps.md b/docs/app-of-apps.md new file mode 100644 index 00000000..43522f4d --- /dev/null +++ b/docs/app-of-apps.md @@ -0,0 +1,110 @@ +# App of Apps + +!!! warning "πŸ§ͺ Experimental" + App of Apps support is an experimental feature. The behaviour and flags described on this page may change in future releases without a deprecation notice. + +The [App of Apps pattern](https://argo-cd.readthedocs.io/en/stable/operator-manual/cluster-bootstrapping/) is a common Argo CD pattern where a parent Application renders child Application manifests. The parent application points to a directory of Application YAML files, and Argo CD creates those child applications automatically. + +Without App of Apps support, `argocd-diff-preview` renders only the applications it discovers directly in your repository files. Child applications that are *generated* by a parent - and therefore never exist as files in the repo - are invisible to the tool. + +With the `--traverse-app-of-apps` flag, `argocd-diff-preview` can discover and render those child applications automatically. + +--- + +## Consider alternatives first + +!!! tip "Prefer simpler alternatives when possible" + The `--traverse-app-of-apps` feature is **slower** and **more limited** than the standard rendering flow. Before enabling it, consider whether one of the alternatives below covers your use case. + +**Alternative 1 - Pre-render your Application manifests** + +If your child Application manifests are stored in a Git repository (which is the common case), `argocd-diff-preview` will find and render them automatically without any special flags. The tool scans your repository for all `kind: Application` and `kind: ApplicationSet` files and renders them directly. + +Only use `--traverse-app-of-apps` when the child Application manifests are *not* committed to the repository and exist only as rendered output from a parent application. + +**Alternative 2 - Helm or Kustomize generated Applications** + +If your parent application uses Helm or Kustomize to generate child Application manifests, you can pre-render them in your CI pipeline and place the output in the branch folder. `argocd-diff-preview` will then pick them up as regular files. See [Helm/Kustomize generated Argo CD applications](generated-applications.md) for details and examples. + +--- + +## How it works + +When `--traverse-app-of-apps` is enabled, the tool performs a breadth-first expansion: + +1. **Render a parent application** - exactly as it normally would. +2. **Scan the rendered manifests** for any resources of `kind: Application`. +3. **Enqueue child applications** - each discovered child is added to the render queue as if it were a top-level application. +4. **Repeat** - until no new child applications are found or the maximum depth is reached. + +--- + +## Requirements + +- **Render method:** `--traverse-app-of-apps` requires `--render-method=repo-server-api`. The flag will cause an error if used with any other render method. + +--- + +## Usage + +```bash +argocd-diff-preview \ + --render-method=repo-server-api \ + --traverse-app-of-apps +``` + +Or via environment variables: + +```bash +RENDER_METHOD=repo-server-api \ +TRAVERSE_APP_OF_APPS=true \ +argocd-diff-preview +``` + +--- + +## Application selection + +Child applications discovered through the App of Apps expansion are subject to the same [application selection](application-selection.md) filters as top-level applications: + +| Filter | Applied to child apps? | +|---|---| +| Watch-pattern annotations (`--files-changed`) | βœ… Yes - the child app's own annotations are evaluated | +| Label selectors (`--selector`) | βœ… Yes | +| `--watch-if-no-watch-pattern-found` | βœ… Yes | +| File path regex (`--file-regex`) | ❌ No - child apps have no physical file path | + +!!! tip "Watch patterns on child apps" + You can add `argocd-diff-preview/watch-pattern` or `argocd.argoproj.io/manifest-generate-paths` annotations directly to your child Application manifests. These annotations are evaluated against the PR's changed files, just like they are for top-level applications. + +--- + +## Cycle and diamond protection + +The expansion engine tracks every `(app-id, branch)` pair it has already rendered. This means: + +- **Cycles** (A β†’ B β†’ A) are detected and broken automatically. +- **Diamond dependencies** (A β†’ C and B β†’ C) cause C to be rendered only once. + +--- + +## Depth limit + +The expansion stops after a maximum depth of **5 levels** to guard against runaway trees. If your App of Apps hierarchy is deeper than 5 levels, applications beyond that depth will not be rendered and a warning will be logged. + +--- + +## Output + +Diff output for child applications looks identical to that of top-level applications. The application name in the diff header includes a breadcrumb showing which parent generated it, making it easy to trace the app-of-apps tree. + +For example, a diff generated with a two-level app-of-apps hierarchy might look like this: + +``` +
+child-app-1 (parent: my-root-app) +
+ +#### ConfigMap: default/some-config +... +``` diff --git a/integration-test/branch-17/target/output.html b/integration-test/branch-17/target/output.html new file mode 100644 index 00000000..ad919bcb --- /dev/null +++ b/integration-test/branch-17/target/output.html @@ -0,0 +1,168 @@ + + + + + + +
+

Argo CD Diff Preview

+ +

Summary:

+
Added (1):
++ level-2c-app (+8)
+
+Modified (3):
+Β± level-1b-app (+1|-1)
+Β± level-2a-app (+2|-1)
+Β± level-2b-app (+1)
+ +
+
+ +level-1b-app (parent: root-app) + + +

ConfigMap: default/level-1b-config

+
+ + + + + + + + + + + +
 apiVersion: v1
 data:
   app: level-1b
-  color: blue
+  color: purple
 kind: ConfigMap
 metadata:
   name: level-1b-config
   namespace: default
+
+ +
+ +
+ +level-2a-app (parent: level-1a-app) + + +

ConfigMap: default/level-2a-config

+
+ + + + + + + + + + + + +
 apiVersion: v1
 data:
   app: level-2a
-  color: green
+  color: yellow
+  environment: production
 kind: ConfigMap
 metadata:
   name: level-2a-config
   namespace: default
+
+ +
+ +
+ +level-2b-app (parent: level-1a-app) + + +

ConfigMap: default/level-2b-config

+
+ + + + + + + + + + + +
 apiVersion: v1
 data:
   app: level-2b
   color: red
+  replicas: "3"
 kind: ConfigMap
 metadata:
   name: level-2b-config
   namespace: default
+
+ +
+ +
+ +level-2c-app (parent: level-1a-app) + + +

ConfigMap: default/level-2c-config

+
+ + + + + + + + + + +
+apiVersion: v1
+data:
+  app: level-2c
+  color: orange
+kind: ConfigMap
+metadata:
+  name: level-2c-config
+  namespace: default
+
+ +
+
+ +
_Stats_:
+[Applications: 2], [Full Run: Xs], [Rendering: Xs], [Cluster: Xs], [Argo CD: Xs]
+
+ + diff --git a/integration-test/branch-17/target/output.md b/integration-test/branch-17/target/output.md new file mode 100644 index 00000000..bfb0d5e1 --- /dev/null +++ b/integration-test/branch-17/target/output.md @@ -0,0 +1,87 @@ +## Argo CD Diff Preview + +Summary: +```yaml +Added (1): ++ level-2c-app (+8) + +Modified (3): +Β± level-1b-app (+1|-1) +Β± level-2a-app (+2|-1) +Β± level-2b-app (+1) +``` + +
+level-1b-app (parent: root-app) +
+ +#### ConfigMap: default/level-1b-config +```diff + apiVersion: v1 + data: + app: level-1b +- color: blue ++ color: purple + kind: ConfigMap + metadata: + name: level-1b-config + namespace: default +``` +
+ +
+level-2a-app (parent: level-1a-app) +
+ +#### ConfigMap: default/level-2a-config +```diff + apiVersion: v1 + data: + app: level-2a +- color: green ++ color: yellow ++ environment: production + kind: ConfigMap + metadata: + name: level-2a-config + namespace: default +``` +
+ +
+level-2b-app (parent: level-1a-app) +
+ +#### ConfigMap: default/level-2b-config +```diff + apiVersion: v1 + data: + app: level-2b + color: red ++ replicas: "3" + kind: ConfigMap + metadata: + name: level-2b-config + namespace: default +``` +
+ +
+level-2c-app (parent: level-1a-app) +
+ +#### ConfigMap: default/level-2c-config +```diff ++apiVersion: v1 ++data: ++ app: level-2c ++ color: orange ++kind: ConfigMap ++metadata: ++ name: level-2c-config ++ namespace: default +``` +
+ +_Stats_: +[Applications: 2], [Full Run: Xs], [Rendering: Xs], [Cluster: Xs], [Argo CD: Xs] diff --git a/integration-test/integration_test.go b/integration-test/integration_test.go index f7862dda..c96e54c5 100644 --- a/integration-test/integration_test.go +++ b/integration-test/integration_test.go @@ -63,6 +63,7 @@ type TestCase struct { DisableClusterRoles string // Use no-cluster-roles/values.yaml (sets createClusterRoles: false) ArgocdConfigDir string // Custom argocd-config directory (relative to integration-test/); overrides auto-derived path ArgocdUIURL string // Argo CD URL for generating application links in diff output + TraverseAppOfApps string // If "true", enables recursive child app discovery (--traverse-app-of-apps) ExpectFailure bool // If true, the test is expected to fail } @@ -272,6 +273,17 @@ var testCases = []TestCase{ CreateCluster: "true", WatchIfNoWatchPatternFound: "false", }, + // Tests the app-of-apps pattern with the repo-server-api render method. + // A single root Application renders child Application YAMLs, which are + // discovered recursively (BFS) and each rendered independently. + { + Name: "branch-17/target", + TargetBranch: "integration-test/branch-17/target", + BaseBranch: "integration-test/branch-17/base", + RenderMethod: "repo-server-api", + FileRegex: "examples/app-of-apps/root-app\\.yaml", + TraverseAppOfApps: "true", + }, // This test verifies that disabling cluster roles without using the API fails. // When createClusterRoles: false is set but --render-method=cli is used, // the tool should fail because it can't access cluster resources via CLI. @@ -865,6 +877,10 @@ func runWithDocker(tc TestCase, createCluster bool) error { args = append(args, "-e", fmt.Sprintf("ARGOCD_UI_URL=%s", tc.ArgocdUIURL)) } + if tc.TraverseAppOfApps == "true" { + args = append(args, "-e", "TRAVERSE_APP_OF_APPS=true") + } + // Add image (no additional args needed - all config is via env vars) args = append(args, *dockerImage) @@ -956,6 +972,10 @@ func buildArgs(tc TestCase, createCluster bool) []string { args = append(args, "--argocd-ui-url", tc.ArgocdUIURL) } + if tc.TraverseAppOfApps == "true" { + args = append(args, "--traverse-app-of-apps") + } + // When the test requires cluster roles to be disabled (API mode or DisableClusterRoles flag), // pass --argocd-config-dir pointing at the no-cluster-roles directory (createClusterRoles: false). // If ArgocdConfigDir is explicitly set, use that directory instead. diff --git a/mkdocs.yml b/mkdocs.yml index 596a44d9..3c56cfaf 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -40,6 +40,7 @@ nav: - Multi-repo: multi-repo.md - application-selection.md - Rendering Methods: rendering-methods.md +- App of Apps: app-of-apps.md - Filter Output: filter-output.md - Output formats: output.md - All Options: options.md diff --git a/pkg/reposerverextract/appofapps.go b/pkg/reposerverextract/appofapps.go new file mode 100644 index 00000000..65ff622b --- /dev/null +++ b/pkg/reposerverextract/appofapps.go @@ -0,0 +1,434 @@ +// Package reposerverextract - app-of-apps expansion. +// +// This file contains all logic for recursively discovering and rendering child +// Applications that appear in a parent application's rendered manifests +// (the "app-of-apps" pattern). It is intentionally isolated so the feature +// can be removed cleanly in the future if it is no longer needed. +// +// The feature is only active when --traverse-app-of-apps is set. +package reposerverextract + +import ( + "context" + "fmt" + "strings" + "sync" + "sync/atomic" + "time" + + "github.com/rs/zerolog/log" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/dag-andersen/argocd-diff-preview/pkg/argoapplication" + argocdPkg "github.com/dag-andersen/argocd-diff-preview/pkg/argocd" + "github.com/dag-andersen/argocd-diff-preview/pkg/extract" + "github.com/dag-andersen/argocd-diff-preview/pkg/git" + "github.com/dag-andersen/argocd-diff-preview/pkg/reposerver" +) + +// maxAppOfAppsDepth is the maximum recursion depth allowed when following +// child Applications discovered in rendered manifests (app-of-apps pattern). +// A depth of 0 means the seed apps themselves; depth 1 means their children, +// and so on. This prevents infinite loops in circular app-of-apps graphs. +const maxAppOfAppsDepth = 10 + +// workItem is a single unit of rendering work, carrying the app to render and +// how deep in the app-of-apps hierarchy it sits. +type workItem struct { + app argoapplication.ArgoResource + depth int +} + +// renderResult captures a single rendered application together with any child +// Application resources that were discovered in its manifests. +type renderResult struct { + // extracted is the ExtractedApp for the rendered application. Its + // Manifests slice already has Application resources stripped out. + extracted extract.ExtractedApp + + // childApps are the ArgoResource values built from Application manifests + // that were discovered inside the rendered output. They have been patched + // and are ready to be enqueued for rendering. + childApps []argoapplication.ArgoResource + + // depth is the depth of the app that produced this result, used to decide + // whether to enqueue its children. + depth int + + err error +} + +// visitedKey returns a unique string key for an (appID, branch) pair, used to +// track which applications have already been rendered during app-of-apps +// expansion. +func visitedKey(id string, branch git.BranchType) string { + return id + "|" + string(branch) +} + +// RenderApplicationsFromBothBranchesWithAppOfApps is like +// RenderApplicationsFromBothBranches but additionally discovers and renders +// child Applications found in rendered manifests (the app-of-apps pattern). +// +// When a rendered app's manifests contain argoproj.io/Application resources, +// those children are patched and enqueued for rendering recursively β€” up to +// maxAppOfAppsDepth levels deep. Child Application YAML manifests are excluded +// from the parent's diff output; each child gets its own ExtractedApp entry. +// +// A visited set prevents re-rendering the same app twice, guarding against +// cycles (Aβ†’Bβ†’A) and diamond dependencies (Aβ†’C, Bβ†’C). +// +// Child apps are filtered by Selector, FilesChanged (via watch-pattern annotations), +// IgnoreInvalidWatchPattern, and WatchIfNoWatchPatternFound β€” the same as top-level +// apps. FileRegex is excluded because it filters by physical file path, and child +// apps have no file path (their FileName is a breadcrumb like "parent: "). +func RenderApplicationsFromBothBranchesWithAppOfApps( + argocd *argocdPkg.ArgoCDInstallation, + baseBranch *git.Branch, + targetBranch *git.Branch, + timeout uint64, + maxConcurrency uint, + baseApps []argoapplication.ArgoResource, + targetApps []argoapplication.ArgoResource, + prRepo string, + appSelectionOptions argoapplication.ApplicationSelectionOptions, +) ([]extract.ExtractedApp, []extract.ExtractedApp, time.Duration, error) { + startTime := time.Now() + + branchFolderByType := map[git.BranchType]string{ + git.Base: baseBranch.FolderName(), + git.Target: targetBranch.FolderName(), + } + + log.Info().Msgf("πŸ“Œ Final number of Applications planned to be rendered via repo server: [Base: %d], [Target: %d]", + len(baseApps), len(targetApps)) + + if err := extract.VerifyNoApplicationSets(baseApps); err != nil { + return nil, nil, time.Since(startTime), err + } + + if err := extract.VerifyNoApplicationSets(targetApps); err != nil { + return nil, nil, time.Since(startTime), err + } + + namespacedScopedResources, err := argocd.K8sClient.GetListOfNamespacedScopedResources() + if err != nil { + return nil, nil, time.Since(startTime), fmt.Errorf("failed to get list of namespaced scoped resources: %w", err) + } + + // Collect all unique repository URLs referenced by the Applications so that + // FetchRepoCreds can enrich them with credentials from repo-creds templates. + appRepoURLs := collectRepoURLs(baseApps, targetApps) + + // Fetch all repository credentials from the cluster once, upfront. + // The repo server has no access to Kubernetes secrets - credentials must be + // provided by the caller in every ManifestRequest. We mirror what the + // ArgoCD app controller does in controller/state.go before calling the repo server. + creds, err := FetchRepoCreds(context.Background(), argocd.K8sClient, argocd.Namespace, appRepoURLs) + if err != nil { + return nil, nil, time.Since(startTime), fmt.Errorf("failed to fetch repository credentials: %w", err) + } + + // Create a single repo server client shared across all goroutines. + // EnsurePortForward is idempotent and mutex-protected inside the client. + repoClient := reposerver.NewClient(argocd.K8sClient, argocd.Namespace) + defer repoClient.Cleanup() + + if err := repoClient.EnsurePortForward(); err != nil { + return nil, nil, time.Since(startTime), fmt.Errorf("failed to set up port forward to repo server: %w", err) + } + + log.Info().Msgf("πŸ€– Rendering Applications via repo server with app-of-apps traversal (timeout in %d seconds)", timeout) + + remainingTime := func() int { + return max(0, int(timeout)-int(time.Since(startTime).Seconds())) + } + + // ── Single-pool expansion ──────────────────────────────────────────────── + // All apps (seed + discovered children) go through the same worker pool. + // A pending counter tracks how many items are in-flight or queued; when it + // reaches zero every goroutine has finished and all results are collected. + // A visited set (mutex-protected) prevents re-rendering the same app twice. + + var ( + extractedBaseApps []extract.ExtractedApp + extractedTargetApps []extract.ExtractedApp + renderedApps atomic.Int32 + pending atomic.Int32 + firstError error + visitedMu sync.Mutex + ) + + visited := make(map[string]bool) + + semSize := int(maxConcurrency) + if semSize == 0 { + semSize = 1 + } + sem := make(chan struct{}, semSize) + + // work is a buffered channel; workers send newly discovered children back + // onto it. We size it generously so senders are never blocked. + work := make(chan workItem, 1024) + results := make(chan renderResult, 1024) + + // enqueue increments pending before sending so the counter is always >= + // actual in-flight count. + enqueue := func(app argoapplication.ArgoResource, depth int) { + pending.Add(1) + work <- workItem{app: app, depth: depth} + } + + // Seed the queue with the initial base + target apps (depth 0). + visitedMu.Lock() + for _, app := range append(baseApps, targetApps...) { + visited[visitedKey(app.Id, app.Branch)] = true + enqueue(app, 0) + } + visitedMu.Unlock() + + progressDone := make(chan bool) + go func() { + ticker := time.NewTicker(5 * time.Second) + defer ticker.Stop() + for { + select { + case <-ticker.C: + log.Info().Msgf("πŸ€– Rendered %d applications via repo server so far (timeout in %d seconds)", + renderedApps.Load(), remainingTime()) + case <-progressDone: + return + } + } + }() + + // Single collector goroutine: reads results, collects extracted apps, and + // enqueues newly discovered children back onto the work channel. + collectorDone := make(chan struct{}) + go func() { + defer close(collectorDone) + for r := range results { + if r.err != nil { + if firstError == nil { + firstError = r.err + } + log.Error().Err(r.err).Msg("❌ Failed to render application via repo server:") + pending.Add(-1) + continue + } + + switch r.extracted.Branch { + case git.Base: + extractedBaseApps = append(extractedBaseApps, r.extracted) + case git.Target: + extractedTargetApps = append(extractedTargetApps, r.extracted) + default: + if firstError == nil { + firstError = fmt.Errorf("unknown branch type: '%s'", r.extracted.Branch) + } + } + + // Enqueue children that haven't been seen yet and pass the selection filter. + // Child apps are filtered by Selector, FilesChanged (via watch-pattern annotations), + // IgnoreInvalidWatchPattern, and WatchIfNoWatchPatternFound β€” exactly as top-level apps are. + // FilesChanged works correctly here: the PR diff is the same regardless of whether an + // app was discovered from a file or from a parent's rendered output; the watch pattern + // on the child app is what determines whether it is affected. + // + // FileRegex is intentionally excluded because it filters by the physical filename of + // the Application YAML file. Child apps don't come from a file; their FileName is + // "parent: " (a breadcrumb), which would give false matches against any regex. + if r.depth < maxAppOfAppsDepth { + childSelectionOptions := argoapplication.ApplicationSelectionOptions{ + Selector: appSelectionOptions.Selector, + FilesChanged: appSelectionOptions.FilesChanged, + IgnoreInvalidWatchPattern: appSelectionOptions.IgnoreInvalidWatchPattern, + WatchIfNoWatchPatternFound: appSelectionOptions.WatchIfNoWatchPatternFound, + // FileRegex intentionally omitted: child apps have no real file path + } + selection := argoapplication.ApplicationSelection(r.childApps, childSelectionOptions) + for _, skipped := range selection.SkippedApps { + log.Debug().Str("App", skipped.GetLongName()).Msg("Skipping child Application excluded by ApplicationSelectionOptions") + } + visitedMu.Lock() + for _, child := range selection.SelectedApps { + key := visitedKey(child.Id, child.Branch) + if visited[key] { + log.Debug().Str("App", child.GetLongName()).Msg("Skipping already-visited child Application") + continue + } + visited[key] = true + enqueue(child, r.depth+1) + } + visitedMu.Unlock() + } else if len(r.childApps) > 0 { + log.Warn().Msgf("⚠️ App-of-apps depth limit (%d) reached; not enqueuing %d child(ren) of %s", + maxAppOfAppsDepth, len(r.childApps), r.extracted.Name) + } + + pending.Add(-1) + + // When all pending work is done, close the work channel so workers exit. + if pending.Load() == 0 { + close(work) + } + } + }() + + // Workers: pull from work channel, render, send result. + var wg sync.WaitGroup + for item := range work { + sem <- struct{}{} + wg.Add(1) + go func(item workItem) { + defer wg.Done() + defer func() { <-sem }() + + if remainingTime() <= 0 { + results <- renderResult{err: fmt.Errorf("timeout reached before starting to render application: %s", item.app.GetLongName())} + return + } + + ctx, cancel := context.WithTimeout(context.Background(), time.Duration(remainingTime())*time.Second) + defer cancel() + + manifests, childApps, err := renderAppWithChildDiscovery(ctx, repoClient, item.app, branchFolderByType, namespacedScopedResources, creds, prRepo, argocd.Namespace) + if err != nil { + results <- renderResult{err: fmt.Errorf("failed to render app %s: %w", item.app.GetLongName(), err)} + return + } + + renderedApps.Add(1) + results <- renderResult{ + extracted: extract.CreateExtractedApp(item.app.Id, item.app.Name, item.app.FileName, manifests, item.app.Branch), + childApps: childApps, + depth: item.depth, + } + }(item) + } + + // All work items have been dequeued. Wait for in-flight workers to finish + // sending their results before closing the results channel. + wg.Wait() + close(results) + <-collectorDone + + close(progressDone) + + if firstError != nil { + return nil, nil, time.Since(startTime), firstError + } + + duration := time.Since(startTime) + log.Info().Msgf("πŸŽ‰ Rendered all %d applications via repo server in %s", + renderedApps.Load(), duration.Round(time.Second)) + log.Info().Msgf("πŸ€– Got %d resources from %s-branch and %d from %s-branch via repo server", + len(extractedBaseApps), git.Base, len(extractedTargetApps), git.Target) + + return extractedBaseApps, extractedTargetApps, time.Since(startTime), nil +} + +// renderAppWithChildDiscovery renders a single application and separates the +// resulting manifests into two groups: +// +// 1. Regular Kubernetes manifests (returned as the first value) – these are +// included in the parent application's diff output. +// +// 2. Child Application resources (returned as the second value) – these are +// patched and queued for recursive rendering. They are excluded from the +// parent's manifest list so that the diff only shows the actual cluster +// resources they produce, not the Application objects themselves. +func renderAppWithChildDiscovery( + ctx context.Context, + repoClient *reposerver.Client, + app argoapplication.ArgoResource, + branchFolderByType map[git.BranchType]string, + namespacedScopedResources map[schema.GroupKind]bool, + creds *RepoCreds, + prRepo string, + argocdNamespace string, +) ([]unstructured.Unstructured, []argoapplication.ArgoResource, error) { + allManifests, err := renderApp(ctx, repoClient, app, branchFolderByType, namespacedScopedResources, creds, prRepo) + if err != nil { + return nil, nil, err + } + + // ── Separate regular manifests from child Application resources ────────── + var regularManifests []unstructured.Unstructured + var childApps []argoapplication.ArgoResource + + for _, m := range allManifests { + if m.GetKind() == "Application" && strings.HasPrefix(m.GetAPIVersion(), "argoproj.io/") { + child, err := buildChildArgoResource(m, app, argocdNamespace) + if err != nil { + log.Warn().Err(err). + Str("parentApp", app.GetLongName()). + Str("childName", m.GetName()). + Msg("⚠️ Could not build child ArgoResource from discovered Application manifest; skipping") + continue + } + childApps = append(childApps, *child) + log.Debug(). + Str("parentApp", app.GetLongName()). + Str("childApp", child.GetLongName()). + Msg("Discovered child Application via app-of-apps pattern") + } else { + regularManifests = append(regularManifests, m) + } + } + + if len(childApps) > 0 { + log.Info(). + Str("parentApp", app.GetLongName()). + Msgf("πŸ” Discovered %d child Application(s) in rendered manifests", len(childApps)) + } + + return regularManifests, childApps, nil +} + +// buildChildArgoResource constructs a patched ArgoResource from a child +// Application manifest that was found in a parent app's rendered output. +// +// The child inherits the parent's branch. Its FileName is set to +// "parent: " so the diff output lets users trace the app-of-apps +// tree without needing to know the physical file path. +func buildChildArgoResource( + manifest unstructured.Unstructured, + parent argoapplication.ArgoResource, + argocdNamespace string, +) (*argoapplication.ArgoResource, error) { + name := manifest.GetName() + if name == "" { + return nil, fmt.Errorf("child Application manifest has no name") + } + + docCopy := manifest.DeepCopy() + + child := argoapplication.NewArgoResource( + docCopy, + argoapplication.Application, + name, // Id + name, // Name + fmt.Sprintf("parent: %s", parent.Name), // FileName: breadcrumb for tracing the app-of-apps tree + parent.Branch, + ) + + // Apply the same patching as for top-level applications so that the child + // renders against the correct cluster / project / branch. + child.SetNamespace(argocdNamespace) + + if err := child.RemoveSyncPolicy(); err != nil { + return nil, fmt.Errorf("failed to remove sync policy from child %q: %w", name, err) + } + if err := child.SetProjectToDefault(); err != nil { + return nil, fmt.Errorf("failed to set project to default for child %q: %w", name, err) + } + if err := child.SetDestinationServerToLocal(); err != nil { + return nil, fmt.Errorf("failed to set destination server for child %q: %w", name, err) + } + if err := child.RemoveArgoCDFinalizers(); err != nil { + return nil, fmt.Errorf("failed to remove finalizers from child %q: %w", name, err) + } + + return child, nil +} diff --git a/pkg/reposerverextract/appofapps_test.go b/pkg/reposerverextract/appofapps_test.go new file mode 100644 index 00000000..5b4849f7 --- /dev/null +++ b/pkg/reposerverextract/appofapps_test.go @@ -0,0 +1,418 @@ +package reposerverextract + +// Tests for the app-of-apps expansion logic in appofapps.go. +// +// RenderApplicationsFromBothBranchesWithAppOfApps requires a live Argo CD repo +// server and is covered by integration tests. This file focuses on the pure +// helper functions that can be exercised without any network or cluster: +// +// - buildChildArgoResource – constructs a patched ArgoResource from a child +// Application manifest found in rendered output. +// - visitedKey – produces a unique deduplication key. + +import ( + "fmt" + "testing" + + "github.com/dag-andersen/argocd-diff-preview/pkg/argoapplication" + "github.com/dag-andersen/argocd-diff-preview/pkg/git" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/yaml" +) + +// ───────────────────────────────────────────────────────────────────────────── +// Helpers +// ───────────────────────────────────────────────────────────────────────────── + +// makeParent builds a minimal parent ArgoResource on the given branch. +func makeParent(t *testing.T, name string, branch git.BranchType) argoapplication.ArgoResource { + t.Helper() + app := makeApp(t, fmt.Sprintf(` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: %s + namespace: argocd +spec: + source: + repoURL: https://github.com/org/repo.git + path: apps/%s + destination: + namespace: argocd +`, name, name)) + app.Branch = branch + return app +} + +// makeChildManifest builds an unstructured Application manifest as it would +// appear in a parent app's rendered output. +func makeChildManifest(t *testing.T, rawYAML string) unstructured.Unstructured { + t.Helper() + var obj unstructured.Unstructured + require.NoError(t, yaml.Unmarshal([]byte(rawYAML), &obj)) + return obj +} + +// ───────────────────────────────────────────────────────────────────────────── +// buildChildArgoResource +// ───────────────────────────────────────────────────────────────────────────── + +// TestBuildChildArgoResource_FileName verifies that the child's FileName is set +// to "parent: " so users can trace the app-of-apps tree. +func TestBuildChildArgoResource_FileName(t *testing.T) { + parent := makeParent(t, "parent-app", git.Base) + + child := makeChildManifest(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: child-app +spec: + source: + repoURL: https://github.com/org/repo.git + path: apps/child + destination: + namespace: child-ns +`) + + result, err := buildChildArgoResource(child, parent, "argocd") + require.NoError(t, err) + assert.Equal(t, "parent: parent-app", result.FileName, + "child FileName must be a breadcrumb pointing to the parent") +} + +// TestBuildChildArgoResource_InheritsBranch verifies that the child inherits the +// parent's branch type (both Base and Target cases). +func TestBuildChildArgoResource_InheritsBranch(t *testing.T) { + for _, branch := range []git.BranchType{git.Base, git.Target} { + t.Run(string(branch), func(t *testing.T) { + parent := makeParent(t, "parent-app", branch) + + child := makeChildManifest(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: child-app +spec: + source: + repoURL: https://github.com/org/repo.git + path: apps/child + destination: + namespace: child-ns +`) + + result, err := buildChildArgoResource(child, parent, "argocd") + require.NoError(t, err) + assert.Equal(t, branch, result.Branch, + "child must inherit parent's branch type (%s)", branch) + }) + } +} + +// TestBuildChildArgoResource_IdAndName verifies that Id and Name are set from +// the manifest's metadata.name. +func TestBuildChildArgoResource_IdAndName(t *testing.T) { + parent := makeParent(t, "parent-app", git.Base) + + child := makeChildManifest(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: my-child-app +spec: + source: + repoURL: https://github.com/org/repo.git + path: apps/child + destination: + namespace: default +`) + + result, err := buildChildArgoResource(child, parent, "argocd") + require.NoError(t, err) + assert.Equal(t, "my-child-app", result.Id) + assert.Equal(t, "my-child-app", result.Name) +} + +// TestBuildChildArgoResource_KindIsApplication verifies the Kind field. +func TestBuildChildArgoResource_KindIsApplication(t *testing.T) { + parent := makeParent(t, "parent-app", git.Base) + + child := makeChildManifest(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: child-app +spec: + source: + repoURL: https://github.com/org/repo.git + path: apps/child + destination: + namespace: default +`) + + result, err := buildChildArgoResource(child, parent, "argocd") + require.NoError(t, err) + assert.Equal(t, argoapplication.Application, result.Kind) +} + +// TestBuildChildArgoResource_PatchesNamespace verifies that the child's +// namespace is overwritten with the ArgoCD namespace. +func TestBuildChildArgoResource_PatchesNamespace(t *testing.T) { + parent := makeParent(t, "parent-app", git.Base) + + child := makeChildManifest(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: child-app + namespace: some-other-namespace +spec: + source: + repoURL: https://github.com/org/repo.git + path: apps/child + destination: + namespace: child-ns +`) + + result, err := buildChildArgoResource(child, parent, "my-argocd-ns") + require.NoError(t, err) + assert.Equal(t, "my-argocd-ns", result.Yaml.GetNamespace(), + "child namespace must be overwritten with the ArgoCD namespace") +} + +// TestBuildChildArgoResource_RemovesSyncPolicy verifies that automated sync is +// stripped so the child is never accidentally synced. +func TestBuildChildArgoResource_RemovesSyncPolicy(t *testing.T) { + parent := makeParent(t, "parent-app", git.Base) + + child := makeChildManifest(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: child-app +spec: + syncPolicy: + automated: + prune: true + selfHeal: true + source: + repoURL: https://github.com/org/repo.git + path: apps/child + destination: + namespace: child-ns +`) + + result, err := buildChildArgoResource(child, parent, "argocd") + require.NoError(t, err) + + _, found, _ := unstructured.NestedMap(result.Yaml.Object, "spec", "syncPolicy") + assert.False(t, found, "syncPolicy must be removed from child Application") +} + +// TestBuildChildArgoResource_SetsProjectToDefault verifies that any custom +// project is replaced with "default". +func TestBuildChildArgoResource_SetsProjectToDefault(t *testing.T) { + parent := makeParent(t, "parent-app", git.Base) + + child := makeChildManifest(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: child-app +spec: + project: my-restricted-project + source: + repoURL: https://github.com/org/repo.git + path: apps/child + destination: + namespace: child-ns +`) + + result, err := buildChildArgoResource(child, parent, "argocd") + require.NoError(t, err) + + project, _, _ := unstructured.NestedString(result.Yaml.Object, "spec", "project") + assert.Equal(t, "default", project, "project must be reset to 'default'") +} + +// TestBuildChildArgoResource_SetsDestinationServerToLocal verifies that the +// child's destination is always pointed at the local in-cluster server. +func TestBuildChildArgoResource_SetsDestinationServerToLocal(t *testing.T) { + parent := makeParent(t, "parent-app", git.Base) + + child := makeChildManifest(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: child-app +spec: + source: + repoURL: https://github.com/org/repo.git + path: apps/child + destination: + server: https://some-external-cluster.example.com + namespace: child-ns +`) + + result, err := buildChildArgoResource(child, parent, "argocd") + require.NoError(t, err) + + server, _, _ := unstructured.NestedString(result.Yaml.Object, "spec", "destination", "server") + assert.Equal(t, "https://kubernetes.default.svc", server, + "destination server must be set to the local cluster") +} + +// TestBuildChildArgoResource_RemovesArgoCDFinalizers verifies that the +// resources-finalizer is stripped so the child can be deleted cleanly. +func TestBuildChildArgoResource_RemovesArgoCDFinalizers(t *testing.T) { + parent := makeParent(t, "parent-app", git.Base) + + child := makeChildManifest(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: child-app + finalizers: + - resources-finalizer.argocd.argoproj.io + - some-other-finalizer +spec: + source: + repoURL: https://github.com/org/repo.git + path: apps/child + destination: + namespace: child-ns +`) + + result, err := buildChildArgoResource(child, parent, "argocd") + require.NoError(t, err) + + finalizers := result.Yaml.GetFinalizers() + assert.NotContains(t, finalizers, "resources-finalizer.argocd.argoproj.io", + "ArgoCD finalizer must be removed") + assert.Contains(t, finalizers, "some-other-finalizer", + "non-ArgoCD finalizers must be preserved") +} + +// TestBuildChildArgoResource_NoFinalizers verifies no panic when no finalizers. +func TestBuildChildArgoResource_NoFinalizers(t *testing.T) { + parent := makeParent(t, "parent-app", git.Base) + + child := makeChildManifest(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: child-app +spec: + source: + repoURL: https://github.com/org/repo.git + path: apps/child + destination: + namespace: child-ns +`) + + result, err := buildChildArgoResource(child, parent, "argocd") + require.NoError(t, err) + assert.NotNil(t, result) +} + +// TestBuildChildArgoResource_EmptyName verifies that a manifest with no name +// returns an error rather than creating a broken ArgoResource. +func TestBuildChildArgoResource_EmptyName(t *testing.T) { + parent := makeParent(t, "parent-app", git.Base) + + child := makeChildManifest(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {} +spec: + source: + repoURL: https://github.com/org/repo.git + path: apps/child +`) + + _, err := buildChildArgoResource(child, parent, "argocd") + assert.Error(t, err, "missing name must return an error") +} + +// TestBuildChildArgoResource_DoesNotMutateOriginal verifies that the original +// manifest is not modified (we deep-copy before patching). +func TestBuildChildArgoResource_DoesNotMutateOriginal(t *testing.T) { + parent := makeParent(t, "parent-app", git.Base) + + child := makeChildManifest(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: child-app + namespace: original-ns +spec: + project: custom-project + syncPolicy: + automated: {} + source: + repoURL: https://github.com/org/repo.git + path: apps/child + destination: + server: https://external.example.com + namespace: child-ns +`) + + originalNS := child.GetNamespace() + originalProject, _, _ := unstructured.NestedString(child.Object, "spec", "project") + + _, err := buildChildArgoResource(child, parent, "argocd") + require.NoError(t, err) + + // The original manifest must be unchanged. + assert.Equal(t, originalNS, child.GetNamespace(), + "original manifest namespace must not be mutated") + project, _, _ := unstructured.NestedString(child.Object, "spec", "project") + assert.Equal(t, originalProject, project, + "original manifest project must not be mutated") + _, found, _ := unstructured.NestedMap(child.Object, "spec", "syncPolicy") + assert.True(t, found, "original manifest syncPolicy must not be removed") +} + +// ───────────────────────────────────────────────────────────────────────────── +// visitedKey +// ───────────────────────────────────────────────────────────────────────────── + +func TestVisitedKey_Format(t *testing.T) { + key := visitedKey("my-app", git.Base) + assert.Contains(t, key, "my-app", "key must contain the app ID") + assert.Contains(t, key, string(git.Base), "key must contain the branch type") +} + +func TestVisitedKey_DifferentIDs(t *testing.T) { + key1 := visitedKey("app-a", git.Base) + key2 := visitedKey("app-b", git.Base) + assert.NotEqual(t, key1, key2, + "different app IDs on the same branch must produce different keys") +} + +func TestVisitedKey_DifferentBranches(t *testing.T) { + key1 := visitedKey("my-app", git.Base) + key2 := visitedKey("my-app", git.Target) + assert.NotEqual(t, key1, key2, + "same app ID on different branches must produce different keys") +} + +func TestVisitedKey_Deterministic(t *testing.T) { + key1 := visitedKey("my-app", git.Base) + key2 := visitedKey("my-app", git.Base) + assert.Equal(t, key1, key2, "visitedKey must be deterministic") +} + +// TestVisitedKey_NoPrefixCollision guards against naive concatenation bugs where +// ("ab", "c") == ("a", "bc") if the separator is omitted. +func TestVisitedKey_NoPrefixCollision(t *testing.T) { + // Construct two app IDs that share a prefix/suffix with the branch string + // to ensure the separator prevents false equality. + key1 := visitedKey("app|base", git.Target) + key2 := visitedKey("app", git.Base) + // These are different (id, branch) pairs and must not collide. + assert.NotEqual(t, key1, key2, + "visitedKey must use a separator that prevents prefix collisions") +} From 98ed30ed9a43c22e3a71cacc9f7e7c018364bf41 Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Mon, 16 Mar 2026 21:50:18 +0100 Subject: [PATCH 02/14] Feat | Expand discovered ApplicationSets into child Applications during app-of-apps traversal --- cmd/main.go | 1 + pkg/reposerverextract/appofapps.go | 90 +++++++++++++-- pkg/reposerverextract/appofapps_test.go | 145 +++++++++++++++++++++++- 3 files changed, 227 insertions(+), 9 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 1d135f8e..22b5eaba 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -331,6 +331,7 @@ func run(cfg *Config) error { targetApps.SelectedApps, cfg.Repo, appSelectionOptions, + tempFolder, ) } else { baseManifests, targetManifests, extractDuration, err = reposerverextract.RenderApplicationsFromBothBranches( diff --git a/pkg/reposerverextract/appofapps.go b/pkg/reposerverextract/appofapps.go index 65ff622b..c16fd349 100644 --- a/pkg/reposerverextract/appofapps.go +++ b/pkg/reposerverextract/appofapps.go @@ -92,6 +92,7 @@ func RenderApplicationsFromBothBranchesWithAppOfApps( targetApps []argoapplication.ArgoResource, prRepo string, appSelectionOptions argoapplication.ApplicationSelectionOptions, + tempFolder string, ) ([]extract.ExtractedApp, []extract.ExtractedApp, time.Duration, error) { startTime := time.Now() @@ -292,7 +293,7 @@ func RenderApplicationsFromBothBranchesWithAppOfApps( ctx, cancel := context.WithTimeout(context.Background(), time.Duration(remainingTime())*time.Second) defer cancel() - manifests, childApps, err := renderAppWithChildDiscovery(ctx, repoClient, item.app, branchFolderByType, namespacedScopedResources, creds, prRepo, argocd.Namespace) + manifests, childApps, err := renderAppWithChildDiscovery(ctx, repoClient, argocd, item.app, branchFolderByType, namespacedScopedResources, creds, prRepo, argocd.Namespace, tempFolder, item.depth) if err != nil { results <- renderResult{err: fmt.Errorf("failed to render app %s: %w", item.app.GetLongName(), err)} return @@ -338,27 +339,40 @@ func RenderApplicationsFromBothBranchesWithAppOfApps( // patched and queued for recursive rendering. They are excluded from the // parent's manifest list so that the diff only shows the actual cluster // resources they produce, not the Application objects themselves. +// +// ApplicationSet resources found in rendered manifests are expanded into their +// generated Applications via argocd.AppsetGenerateWithRetry, then treated the +// same as directly-discovered child Applications. func renderAppWithChildDiscovery( ctx context.Context, repoClient *reposerver.Client, + argocd *argocdPkg.ArgoCDInstallation, app argoapplication.ArgoResource, branchFolderByType map[git.BranchType]string, namespacedScopedResources map[schema.GroupKind]bool, creds *RepoCreds, prRepo string, argocdNamespace string, + tempFolder string, + depth int, ) ([]unstructured.Unstructured, []argoapplication.ArgoResource, error) { allManifests, err := renderApp(ctx, repoClient, app, branchFolderByType, namespacedScopedResources, creds, prRepo) if err != nil { return nil, nil, err } - // ── Separate regular manifests from child Application resources ────────── + // ── Separate regular manifests from child Application/ApplicationSet resources ────────── var regularManifests []unstructured.Unstructured var childApps []argoapplication.ArgoResource for _, m := range allManifests { - if m.GetKind() == "Application" && strings.HasPrefix(m.GetAPIVersion(), "argoproj.io/") { + if !strings.HasPrefix(m.GetAPIVersion(), "argoproj.io/") { + regularManifests = append(regularManifests, m) + continue + } + + switch m.GetKind() { + case "Application": child, err := buildChildArgoResource(m, app, argocdNamespace) if err != nil { log.Warn().Err(err). @@ -372,7 +386,53 @@ func renderAppWithChildDiscovery( Str("parentApp", app.GetLongName()). Str("childApp", child.GetLongName()). Msg("Discovered child Application via app-of-apps pattern") - } else { + + case "ApplicationSet": + // Expand the ApplicationSet into its generated Applications using the + // ArgoCD API/CLI, then treat each generated app as a child to enqueue. + appSetName := m.GetName() + log.Info(). + Str("parentApp", app.GetLongName()). + Str("appSet", appSetName). + Msgf("πŸ” Discovered child ApplicationSet in rendered manifests; expanding to Applications") + + appSetTempFolder := fmt.Sprintf("%s/appsets/depth-%d", tempFolder, depth) + docCopy := m.DeepCopy() + generatedManifests, err := argocd.AppsetGenerateWithRetry(docCopy, appSetTempFolder, 5) + if err != nil { + log.Warn().Err(err). + Str("parentApp", app.GetLongName()). + Str("appSet", appSetName). + Msg("⚠️ Could not expand child ApplicationSet; skipping") + continue + } + + breadcrumb := fmt.Sprintf("parent: %s (appset: %s)", app.Name, appSetName) + for _, genDoc := range generatedManifests { + if genDoc.GetKind() != "Application" { + log.Warn(). + Str("appSet", appSetName). + Str("kind", genDoc.GetKind()). + Msg("⚠️ ApplicationSet generated unexpected non-Application resource; skipping") + continue + } + child, err := buildChildFromGeneratedApp(genDoc, breadcrumb, app.Branch, argocdNamespace) + if err != nil { + log.Warn().Err(err). + Str("parentApp", app.GetLongName()). + Str("appSet", appSetName). + Msg("⚠️ Could not build ArgoResource from ApplicationSet-generated Application; skipping") + continue + } + childApps = append(childApps, *child) + log.Debug(). + Str("parentApp", app.GetLongName()). + Str("appSet", appSetName). + Str("childApp", child.GetLongName()). + Msg("Discovered child Application via ApplicationSet expansion") + } + + default: regularManifests = append(regularManifests, m) } } @@ -396,6 +456,20 @@ func buildChildArgoResource( manifest unstructured.Unstructured, parent argoapplication.ArgoResource, argocdNamespace string, +) (*argoapplication.ArgoResource, error) { + fileName := fmt.Sprintf("parent: %s", parent.Name) + return buildChildFromGeneratedApp(manifest, fileName, parent.Branch, argocdNamespace) +} + +// buildChildFromGeneratedApp constructs a patched ArgoResource from a generated +// Application manifest (either directly discovered in a parent's rendered output, +// or produced by expanding a child ApplicationSet). The caller supplies the +// FileName breadcrumb and branch type. +func buildChildFromGeneratedApp( + manifest unstructured.Unstructured, + fileName string, + branch git.BranchType, + argocdNamespace string, ) (*argoapplication.ArgoResource, error) { name := manifest.GetName() if name == "" { @@ -407,10 +481,10 @@ func buildChildArgoResource( child := argoapplication.NewArgoResource( docCopy, argoapplication.Application, - name, // Id - name, // Name - fmt.Sprintf("parent: %s", parent.Name), // FileName: breadcrumb for tracing the app-of-apps tree - parent.Branch, + name, // Id + name, // Name + fileName, // FileName: breadcrumb for tracing the app-of-apps tree + branch, ) // Apply the same patching as for top-level applications so that the child diff --git a/pkg/reposerverextract/appofapps_test.go b/pkg/reposerverextract/appofapps_test.go index 5b4849f7..aa6ad73a 100644 --- a/pkg/reposerverextract/appofapps_test.go +++ b/pkg/reposerverextract/appofapps_test.go @@ -376,9 +376,152 @@ spec: } // ───────────────────────────────────────────────────────────────────────────── -// visitedKey +// buildChildFromGeneratedApp // ───────────────────────────────────────────────────────────────────────────── +// TestBuildChildFromGeneratedApp_BasicFields verifies that Id, Name, FileName, +// and Branch are set correctly when building from a generated Application. +func TestBuildChildFromGeneratedApp_BasicFields(t *testing.T) { + manifest := makeChildManifest(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: generated-app +spec: + source: + repoURL: https://github.com/org/repo.git + path: apps/generated + destination: + namespace: default +`) + + breadcrumb := "parent: root (appset: my-appset)" + result, err := buildChildFromGeneratedApp(manifest, breadcrumb, git.Base, "argocd") + require.NoError(t, err) + assert.Equal(t, "generated-app", result.Id) + assert.Equal(t, "generated-app", result.Name) + assert.Equal(t, breadcrumb, result.FileName) + assert.Equal(t, git.Base, result.Branch) + assert.Equal(t, argoapplication.Application, result.Kind) +} + +// TestBuildChildFromGeneratedApp_PatchesApplied verifies that all required +// patches (namespace, project, destination, sync policy, finalizers) are applied. +func TestBuildChildFromGeneratedApp_PatchesApplied(t *testing.T) { + manifest := makeChildManifest(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: generated-app + namespace: some-other-ns + finalizers: + - resources-finalizer.argocd.argoproj.io +spec: + project: restricted-project + syncPolicy: + automated: + prune: true + source: + repoURL: https://github.com/org/repo.git + path: apps/generated + destination: + server: https://external-cluster.example.com + namespace: default +`) + + result, err := buildChildFromGeneratedApp(manifest, "parent: root (appset: my-appset)", git.Target, "argocd") + require.NoError(t, err) + + assert.Equal(t, "argocd", result.Yaml.GetNamespace(), "namespace must be overwritten with ArgoCD namespace") + + project, _, _ := unstructured.NestedString(result.Yaml.Object, "spec", "project") + assert.Equal(t, "default", project, "project must be reset to 'default'") + + _, found, _ := unstructured.NestedMap(result.Yaml.Object, "spec", "syncPolicy") + assert.False(t, found, "syncPolicy must be removed") + + server, _, _ := unstructured.NestedString(result.Yaml.Object, "spec", "destination", "server") + assert.Equal(t, "https://kubernetes.default.svc", server, "destination server must point to local cluster") + + finalizers := result.Yaml.GetFinalizers() + assert.NotContains(t, finalizers, "resources-finalizer.argocd.argoproj.io", "ArgoCD finalizer must be removed") +} + +// TestBuildChildFromGeneratedApp_EmptyName verifies that a manifest with no name +// returns an error. +func TestBuildChildFromGeneratedApp_EmptyName(t *testing.T) { + manifest := makeChildManifest(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: {} +spec: + source: + repoURL: https://github.com/org/repo.git + path: apps/generated +`) + + _, err := buildChildFromGeneratedApp(manifest, "parent: root (appset: my-appset)", git.Base, "argocd") + assert.Error(t, err, "missing name must return an error") +} + +// TestBuildChildFromGeneratedApp_DoesNotMutateOriginal verifies that the original +// manifest is not modified (we deep-copy before patching). +func TestBuildChildFromGeneratedApp_DoesNotMutateOriginal(t *testing.T) { + manifest := makeChildManifest(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: generated-app + namespace: original-ns +spec: + project: custom-project + syncPolicy: + automated: {} + source: + repoURL: https://github.com/org/repo.git + path: apps/generated + destination: + server: https://external.example.com + namespace: default +`) + + originalNS := manifest.GetNamespace() + originalProject, _, _ := unstructured.NestedString(manifest.Object, "spec", "project") + + _, err := buildChildFromGeneratedApp(manifest, "parent: root (appset: my-appset)", git.Base, "argocd") + require.NoError(t, err) + + assert.Equal(t, originalNS, manifest.GetNamespace(), "original manifest namespace must not be mutated") + project, _, _ := unstructured.NestedString(manifest.Object, "spec", "project") + assert.Equal(t, originalProject, project, "original manifest project must not be mutated") + _, found, _ := unstructured.NestedMap(manifest.Object, "spec", "syncPolicy") + assert.True(t, found, "original manifest syncPolicy must not be removed") +} + +// TestBuildChildArgoResource_UsesParentNameBreadcrumb verifies that +// buildChildArgoResource (which wraps buildChildFromGeneratedApp) sets +// FileName to "parent: " - distinct from the appset breadcrumb format. +func TestBuildChildArgoResource_UsesParentNameBreadcrumb(t *testing.T) { + parent := makeParent(t, "root-app", git.Base) + child := makeChildManifest(t, ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: child-app +spec: + source: + repoURL: https://github.com/org/repo.git + path: apps/child + destination: + namespace: default +`) + + result, err := buildChildArgoResource(child, parent, "argocd") + require.NoError(t, err) + assert.Equal(t, "parent: root-app", result.FileName, + "direct child FileName must be 'parent: '") +} + func TestVisitedKey_Format(t *testing.T) { key := visitedKey("my-app", git.Base) assert.Contains(t, key, "my-app", "key must contain the app ID") From 5e4c647ccae6477cad4f179cc2a83734a274fba8 Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Mon, 16 Mar 2026 23:14:05 +0100 Subject: [PATCH 03/14] Feat | Patch child Applications and ApplicationSets during app-of-apps traversal using PatchApplication --- cmd/main.go | 2 +- integration-test/branch-17/target/output.html | 51 +- integration-test/branch-17/target/output.md | 41 +- pkg/argoapplication/patching.go | 6 +- pkg/argoapplication/patching_test.go | 439 +++++++++++++++ pkg/reposerverextract/appofapps.go | 110 ++-- pkg/reposerverextract/appofapps_test.go | 518 +----------------- 7 files changed, 578 insertions(+), 589 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 22b5eaba..522af0e3 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -369,7 +369,7 @@ func run(cfg *Config) error { ExtractDuration: extractDuration + convertAppSetsToAppsDuration, ArgoCDInstallationDuration: argocdInstallationDuration, ClusterCreationDuration: clusterCreationDuration, - ApplicationCount: len(baseApps.SelectedApps) + len(targetApps.SelectedApps), + ApplicationCount: len(baseManifests) + len(targetManifests), } // Write manifest files if requested diff --git a/integration-test/branch-17/target/output.html b/integration-test/branch-17/target/output.html index ad919bcb..c8b0622a 100644 --- a/integration-test/branch-17/target/output.html +++ b/integration-test/branch-17/target/output.html @@ -59,11 +59,13 @@

Argo CD Diff Preview

Summary:

-
Added (1):
+
Added (2):
++ level-1c-staging-app (+8)
 + level-2c-app (+8)
 
-Modified (3):
+Modified (4):
 Β± level-1b-app (+1|-1)
+Β± level-1c-prod-app (+1|-1)
 Β± level-2a-app (+2|-1)
 Β± level-2b-app (+1)

 
@@ -91,6 +93,29 @@ 
ConfigMap: default/level-1b-config
+
+ +level-1c-prod-app (parent: root-app (appset: level-1c-appset)) + + +

ConfigMap: default/level-1c-prod-config

+
+ + + + + + + + + + + +
 apiVersion: v1
 data:
   app: level-1c-prod
-  color: blue
+  color: purple
 kind: ConfigMap
 metadata:
   name: level-1c-prod-config
   namespace: default
+
+ +
+
level-2a-app (parent: level-1a-app) @@ -138,6 +163,28 @@

ConfigMap: default/level-2b-config

+
+ +level-1c-staging-app (parent: root-app (appset: level-1c-appset)) + + +

ConfigMap: default/level-1c-staging-config

+
+ + + + + + + + + + +
+apiVersion: v1
+data:
+  app: level-1c-staging
+  color: orange
+kind: ConfigMap
+metadata:
+  name: level-1c-staging-config
+  namespace: default
+
+ +
+
level-2c-app (parent: level-1a-app) diff --git a/integration-test/branch-17/target/output.md b/integration-test/branch-17/target/output.md index bfb0d5e1..a9d70ce0 100644 --- a/integration-test/branch-17/target/output.md +++ b/integration-test/branch-17/target/output.md @@ -2,11 +2,13 @@ Summary: ```yaml -Added (1): +Added (2): ++ level-1c-staging-app (+8) + level-2c-app (+8) -Modified (3): +Modified (4): Β± level-1b-app (+1|-1) +Β± level-1c-prod-app (+1|-1) Β± level-2a-app (+2|-1) Β± level-2b-app (+1) ``` @@ -29,6 +31,24 @@ Modified (3): ```
+
+level-1c-prod-app (parent: root-app (appset: level-1c-appset)) +
+ +#### ConfigMap: default/level-1c-prod-config +```diff + apiVersion: v1 + data: + app: level-1c-prod +- color: blue ++ color: purple + kind: ConfigMap + metadata: + name: level-1c-prod-config + namespace: default +``` +
+
level-2a-app (parent: level-1a-app)
@@ -66,6 +86,23 @@ Modified (3): ```
+
+level-1c-staging-app (parent: root-app (appset: level-1c-appset)) +
+ +#### ConfigMap: default/level-1c-staging-config +```diff ++apiVersion: v1 ++data: ++ app: level-1c-staging ++ color: orange ++kind: ConfigMap ++metadata: ++ name: level-1c-staging-config ++ namespace: default +``` +
+
level-2c-app (parent: level-1a-app)
diff --git a/pkg/argoapplication/patching.go b/pkg/argoapplication/patching.go index eac8e33d..58c74366 100644 --- a/pkg/argoapplication/patching.go +++ b/pkg/argoapplication/patching.go @@ -20,7 +20,7 @@ func patchApplications( var patchedApps []ArgoResource for _, app := range applications { - patchedApp, err := patchApplication( + patchedApp, err := PatchApplication( argocdNamespace, app, branch, @@ -36,8 +36,8 @@ func patchApplications( return patchedApps, nil } -// patchApplication patches a single ArgoResource -func patchApplication( +// PatchApplication patches a single ArgoResource +func PatchApplication( argocdNamespace string, app ArgoResource, branch *git.Branch, diff --git a/pkg/argoapplication/patching_test.go b/pkg/argoapplication/patching_test.go index 00b837be..f8bcb612 100644 --- a/pkg/argoapplication/patching_test.go +++ b/pkg/argoapplication/patching_test.go @@ -4,6 +4,7 @@ import ( "fmt" "testing" + "github.com/dag-andersen/argocd-diff-preview/pkg/git" "github.com/rs/zerolog" "github.com/stretchr/testify/assert" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -805,6 +806,444 @@ spec:` return string(yamlBytes) } +// TestPatchApplication verifies the full PatchApplication pipeline on a single +// ArgoResource. Each sub-test exercises one or more of the patches that +// PatchApplication chains together (namespace, project, destination, sync +// policy, finalizers, source redirect). +func TestPatchApplication(t *testing.T) { + zerolog.SetGlobalLevel(zerolog.FatalLevel) + + const ( + argocdNamespace = "argocd" + prRepo = "https://github.com/org/repo.git" + branchName = "my-feature" + ) + + branch := git.NewBranch(branchName, git.Target) + + tests := []struct { + name string + kind ApplicationKind + inputYAML string + wantYAML string + redirectRevisions []string + }{ + { + name: "namespace is set to argocd", + kind: Application, + inputYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: my-app + namespace: some-other-namespace +spec: + project: my-project + source: + repoURL: https://github.com/org/repo.git + targetRevision: HEAD + path: apps/my-app + destination: + server: https://kubernetes.default.svc + namespace: default +`, + wantYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: my-app + namespace: argocd +spec: + project: default + source: + repoURL: https://github.com/org/repo.git + targetRevision: my-feature + path: apps/my-app + destination: + server: https://kubernetes.default.svc + namespace: default +`, + }, + { + name: "sync policy is removed", + kind: Application, + inputYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: sync-app + namespace: argocd +spec: + project: default + source: + repoURL: https://github.com/org/repo.git + targetRevision: HEAD + path: apps/sync-app + destination: + server: https://kubernetes.default.svc + namespace: default + syncPolicy: + automated: + prune: true + selfHeal: true + syncOptions: + - CreateNamespace=true +`, + wantYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: sync-app + namespace: argocd +spec: + project: default + source: + repoURL: https://github.com/org/repo.git + targetRevision: my-feature + path: apps/sync-app + destination: + server: https://kubernetes.default.svc + namespace: default +`, + }, + { + name: "project is reset to default", + kind: Application, + inputYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: proj-app + namespace: argocd +spec: + project: production + source: + repoURL: https://github.com/org/repo.git + targetRevision: HEAD + path: apps/proj-app + destination: + server: https://kubernetes.default.svc + namespace: default +`, + wantYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: proj-app + namespace: argocd +spec: + project: default + source: + repoURL: https://github.com/org/repo.git + targetRevision: my-feature + path: apps/proj-app + destination: + server: https://kubernetes.default.svc + namespace: default +`, + }, + { + name: "destination server is redirected to in-cluster", + kind: Application, + inputYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: dest-app + namespace: argocd +spec: + project: default + source: + repoURL: https://github.com/org/repo.git + targetRevision: HEAD + path: apps/dest-app + destination: + name: remote-cluster + namespace: production +`, + wantYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: dest-app + namespace: argocd +spec: + project: default + source: + repoURL: https://github.com/org/repo.git + targetRevision: my-feature + path: apps/dest-app + destination: + server: https://kubernetes.default.svc + namespace: production +`, + }, + { + name: "argocd finalizer is removed", + kind: Application, + inputYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: final-app + namespace: argocd + finalizers: + - resources-finalizer.argocd.argoproj.io + - some-other-finalizer +spec: + project: default + source: + repoURL: https://github.com/org/repo.git + targetRevision: HEAD + path: apps/final-app + destination: + server: https://kubernetes.default.svc + namespace: default +`, + wantYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: final-app + namespace: argocd + finalizers: + - some-other-finalizer +spec: + project: default + source: + repoURL: https://github.com/org/repo.git + targetRevision: my-feature + path: apps/final-app + destination: + server: https://kubernetes.default.svc + namespace: default +`, + }, + { + name: "source targetRevision is redirected to branch", + kind: Application, + inputYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: src-app + namespace: argocd +spec: + project: default + source: + repoURL: https://github.com/org/repo.git + targetRevision: main + path: apps/src-app + destination: + server: https://kubernetes.default.svc + namespace: default +`, + wantYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: src-app + namespace: argocd +spec: + project: default + source: + repoURL: https://github.com/org/repo.git + targetRevision: my-feature + path: apps/src-app + destination: + server: https://kubernetes.default.svc + namespace: default +`, + }, + { + name: "source revision not redirected when repoURL does not match", + kind: Application, + inputYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: external-app + namespace: argocd +spec: + project: default + source: + repoURL: https://github.com/other/unrelated.git + targetRevision: HEAD + path: apps/external-app + destination: + server: https://kubernetes.default.svc + namespace: default +`, + wantYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: external-app + namespace: argocd +spec: + project: default + source: + repoURL: https://github.com/other/unrelated.git + targetRevision: HEAD + path: apps/external-app + destination: + server: https://kubernetes.default.svc + namespace: default +`, + }, + { + name: "specific redirectRevisions: only matching revision is redirected", + kind: Application, + inputYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: selective-app + namespace: argocd +spec: + project: default + source: + repoURL: https://github.com/org/repo.git + targetRevision: HEAD + path: apps/selective-app + destination: + server: https://kubernetes.default.svc + namespace: default +`, + wantYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: selective-app + namespace: argocd +spec: + project: default + source: + repoURL: https://github.com/org/repo.git + targetRevision: my-feature + path: apps/selective-app + destination: + server: https://kubernetes.default.svc + namespace: default +`, + redirectRevisions: []string{"HEAD", "main"}, + }, + { + name: "specific redirectRevisions: non-matching revision is left unchanged", + kind: Application, + inputYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: pinned-app + namespace: argocd +spec: + project: default + source: + repoURL: https://github.com/org/repo.git + targetRevision: v1.2.3 + path: apps/pinned-app + destination: + server: https://kubernetes.default.svc + namespace: default +`, + wantYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: pinned-app + namespace: argocd +spec: + project: default + source: + repoURL: https://github.com/org/repo.git + targetRevision: v1.2.3 + path: apps/pinned-app + destination: + server: https://kubernetes.default.svc + namespace: default +`, + redirectRevisions: []string{"HEAD", "main"}, + }, + { + name: "ApplicationSet namespace and project patched", + kind: ApplicationSet, + inputYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: ApplicationSet +metadata: + name: my-appset + namespace: some-namespace +spec: + generators: + - git: + repoURL: https://github.com/org/repo.git + revision: HEAD + template: + metadata: + namespace: argocd + spec: + project: platform + destination: + server: https://kubernetes.default.svc + namespace: default +`, + wantYAML: ` +apiVersion: argoproj.io/v1alpha1 +kind: ApplicationSet +metadata: + name: my-appset + namespace: argocd +spec: + generators: + - git: + repoURL: https://github.com/org/repo.git + revision: my-feature + template: + metadata: + namespace: argocd + spec: + project: default + destination: + server: https://kubernetes.default.svc + namespace: default +`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var obj map[string]any + err := yaml.Unmarshal([]byte(tt.inputYAML), &obj) + assert.NoError(t, err) + + resource := &ArgoResource{ + Yaml: &unstructured.Unstructured{Object: obj}, + Kind: tt.kind, + Id: "test-id", + Name: "test-name", + FileName: "parent: root-app", + } + + redirectRevisions := tt.redirectRevisions + + // ── Normal cases ──────────────────────────────────────────────── + patched, err := PatchApplication(argocdNamespace, *resource, branch, prRepo, redirectRevisions) + assert.NoError(t, err) + assert.NotNil(t, patched) + + // FileName breadcrumb must be preserved unchanged. + assert.Equal(t, resource.FileName, patched.FileName, + "PatchApplication must preserve FileName") + + got, err := patched.AsString() + assert.NoError(t, err) + + assert.Equal(t, normalizeYAML(tt.wantYAML), normalizeYAML(got)) + }) + } +} + func TestRedirectSourceHydrator(t *testing.T) { zerolog.SetGlobalLevel(zerolog.FatalLevel) diff --git a/pkg/reposerverextract/appofapps.go b/pkg/reposerverextract/appofapps.go index c16fd349..38181cbf 100644 --- a/pkg/reposerverextract/appofapps.go +++ b/pkg/reposerverextract/appofapps.go @@ -101,6 +101,11 @@ func RenderApplicationsFromBothBranchesWithAppOfApps( git.Target: targetBranch.FolderName(), } + branchByType := map[git.BranchType]*git.Branch{ + git.Base: baseBranch, + git.Target: targetBranch, + } + log.Info().Msgf("πŸ“Œ Final number of Applications planned to be rendered via repo server: [Base: %d], [Target: %d]", len(baseApps), len(targetApps)) @@ -293,7 +298,7 @@ func RenderApplicationsFromBothBranchesWithAppOfApps( ctx, cancel := context.WithTimeout(context.Background(), time.Duration(remainingTime())*time.Second) defer cancel() - manifests, childApps, err := renderAppWithChildDiscovery(ctx, repoClient, argocd, item.app, branchFolderByType, namespacedScopedResources, creds, prRepo, argocd.Namespace, tempFolder, item.depth) + manifests, childApps, err := renderAppWithChildDiscovery(ctx, repoClient, argocd, item.app, branchFolderByType, branchByType, namespacedScopedResources, creds, prRepo, argocd.Namespace, tempFolder, item.depth) if err != nil { results <- renderResult{err: fmt.Errorf("failed to render app %s: %w", item.app.GetLongName(), err)} return @@ -349,6 +354,7 @@ func renderAppWithChildDiscovery( argocd *argocdPkg.ArgoCDInstallation, app argoapplication.ArgoResource, branchFolderByType map[git.BranchType]string, + branchByType map[git.BranchType]*git.Branch, namespacedScopedResources map[schema.GroupKind]bool, creds *RepoCreds, prRepo string, @@ -373,12 +379,19 @@ func renderAppWithChildDiscovery( switch m.GetKind() { case "Application": - child, err := buildChildArgoResource(m, app, argocdNamespace) + name := m.GetName() + if name == "" { + log.Warn().Str("parentApp", app.GetLongName()).Msg("⚠️ Discovered child Application has no name; skipping") + continue + } + fileName := fmt.Sprintf("parent: %s", app.Name) + resource := argoapplication.NewArgoResource(&m, argoapplication.Application, name, name, fileName, app.Branch) + child, err := argoapplication.PatchApplication(argocdNamespace, *resource, branchByType[app.Branch], prRepo, nil) if err != nil { log.Warn().Err(err). Str("parentApp", app.GetLongName()). - Str("childName", m.GetName()). - Msg("⚠️ Could not build child ArgoResource from discovered Application manifest; skipping") + Str("childName", name). + Msg("⚠️ Could not patch child Application; skipping") continue } childApps = append(childApps, *child) @@ -397,8 +410,22 @@ func renderAppWithChildDiscovery( Msgf("πŸ” Discovered child ApplicationSet in rendered manifests; expanding to Applications") appSetTempFolder := fmt.Sprintf("%s/appsets/depth-%d", tempFolder, depth) - docCopy := m.DeepCopy() - generatedManifests, err := argocd.AppsetGenerateWithRetry(docCopy, appSetTempFolder, 5) + branch := branchByType[app.Branch] + + // Patch the ApplicationSet the same way top-level ApplicationSets are patched + // before being sent to the API. This strips spec.template.metadata.namespace + // (e.g. "argocd") which ArgoCD's /api/v1/applicationsets/generate endpoint rejects. + appSetResource := argoapplication.NewArgoResource(&m, argoapplication.ApplicationSet, appSetName, appSetName, app.FileName, app.Branch) + patchedAppSet, err := argoapplication.PatchApplication(argocdNamespace, *appSetResource, branch, prRepo, nil) + if err != nil { + log.Warn().Err(err). + Str("parentApp", app.GetLongName()). + Str("appSet", appSetName). + Msg("⚠️ Failed to patch child ApplicationSet before expansion; skipping") + continue + } + + generatedManifests, err := argocd.AppsetGenerateWithRetry(patchedAppSet.Yaml, appSetTempFolder, 5) if err != nil { log.Warn().Err(err). Str("parentApp", app.GetLongName()). @@ -416,12 +443,18 @@ func renderAppWithChildDiscovery( Msg("⚠️ ApplicationSet generated unexpected non-Application resource; skipping") continue } - child, err := buildChildFromGeneratedApp(genDoc, breadcrumb, app.Branch, argocdNamespace) + name := genDoc.GetName() + if name == "" { + log.Warn().Str("appSet", appSetName).Msg("⚠️ ApplicationSet-generated Application has no name; skipping") + continue + } + resource := argoapplication.NewArgoResource(&genDoc, argoapplication.Application, name, name, breadcrumb, app.Branch) + child, err := argoapplication.PatchApplication(argocdNamespace, *resource, branch, prRepo, nil) if err != nil { log.Warn().Err(err). Str("parentApp", app.GetLongName()). Str("appSet", appSetName). - Msg("⚠️ Could not build ArgoResource from ApplicationSet-generated Application; skipping") + Msg("⚠️ Could not patch ApplicationSet-generated Application; skipping") continue } childApps = append(childApps, *child) @@ -445,64 +478,3 @@ func renderAppWithChildDiscovery( return regularManifests, childApps, nil } - -// buildChildArgoResource constructs a patched ArgoResource from a child -// Application manifest that was found in a parent app's rendered output. -// -// The child inherits the parent's branch. Its FileName is set to -// "parent: " so the diff output lets users trace the app-of-apps -// tree without needing to know the physical file path. -func buildChildArgoResource( - manifest unstructured.Unstructured, - parent argoapplication.ArgoResource, - argocdNamespace string, -) (*argoapplication.ArgoResource, error) { - fileName := fmt.Sprintf("parent: %s", parent.Name) - return buildChildFromGeneratedApp(manifest, fileName, parent.Branch, argocdNamespace) -} - -// buildChildFromGeneratedApp constructs a patched ArgoResource from a generated -// Application manifest (either directly discovered in a parent's rendered output, -// or produced by expanding a child ApplicationSet). The caller supplies the -// FileName breadcrumb and branch type. -func buildChildFromGeneratedApp( - manifest unstructured.Unstructured, - fileName string, - branch git.BranchType, - argocdNamespace string, -) (*argoapplication.ArgoResource, error) { - name := manifest.GetName() - if name == "" { - return nil, fmt.Errorf("child Application manifest has no name") - } - - docCopy := manifest.DeepCopy() - - child := argoapplication.NewArgoResource( - docCopy, - argoapplication.Application, - name, // Id - name, // Name - fileName, // FileName: breadcrumb for tracing the app-of-apps tree - branch, - ) - - // Apply the same patching as for top-level applications so that the child - // renders against the correct cluster / project / branch. - child.SetNamespace(argocdNamespace) - - if err := child.RemoveSyncPolicy(); err != nil { - return nil, fmt.Errorf("failed to remove sync policy from child %q: %w", name, err) - } - if err := child.SetProjectToDefault(); err != nil { - return nil, fmt.Errorf("failed to set project to default for child %q: %w", name, err) - } - if err := child.SetDestinationServerToLocal(); err != nil { - return nil, fmt.Errorf("failed to set destination server for child %q: %w", name, err) - } - if err := child.RemoveArgoCDFinalizers(); err != nil { - return nil, fmt.Errorf("failed to remove finalizers from child %q: %w", name, err) - } - - return child, nil -} diff --git a/pkg/reposerverextract/appofapps_test.go b/pkg/reposerverextract/appofapps_test.go index aa6ad73a..b43aeeff 100644 --- a/pkg/reposerverextract/appofapps_test.go +++ b/pkg/reposerverextract/appofapps_test.go @@ -3,525 +3,22 @@ package reposerverextract // Tests for the app-of-apps expansion logic in appofapps.go. // // RenderApplicationsFromBothBranchesWithAppOfApps requires a live Argo CD repo -// server and is covered by integration tests. This file focuses on the pure +// server and is covered by integration tests. This file focuses on the pure // helper functions that can be exercised without any network or cluster: // -// - buildChildArgoResource – constructs a patched ArgoResource from a child -// Application manifest found in rendered output. -// - visitedKey – produces a unique deduplication key. +// - visitedKey – produces a unique deduplication key. +// +// Patching logic for discovered child Applications and ApplicationSets is +// delegated entirely to argoapplication.PatchApplication, which is tested in +// pkg/argoapplication. import ( - "fmt" "testing" - "github.com/dag-andersen/argocd-diff-preview/pkg/argoapplication" "github.com/dag-andersen/argocd-diff-preview/pkg/git" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "sigs.k8s.io/yaml" ) -// ───────────────────────────────────────────────────────────────────────────── -// Helpers -// ───────────────────────────────────────────────────────────────────────────── - -// makeParent builds a minimal parent ArgoResource on the given branch. -func makeParent(t *testing.T, name string, branch git.BranchType) argoapplication.ArgoResource { - t.Helper() - app := makeApp(t, fmt.Sprintf(` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: %s - namespace: argocd -spec: - source: - repoURL: https://github.com/org/repo.git - path: apps/%s - destination: - namespace: argocd -`, name, name)) - app.Branch = branch - return app -} - -// makeChildManifest builds an unstructured Application manifest as it would -// appear in a parent app's rendered output. -func makeChildManifest(t *testing.T, rawYAML string) unstructured.Unstructured { - t.Helper() - var obj unstructured.Unstructured - require.NoError(t, yaml.Unmarshal([]byte(rawYAML), &obj)) - return obj -} - -// ───────────────────────────────────────────────────────────────────────────── -// buildChildArgoResource -// ───────────────────────────────────────────────────────────────────────────── - -// TestBuildChildArgoResource_FileName verifies that the child's FileName is set -// to "parent: " so users can trace the app-of-apps tree. -func TestBuildChildArgoResource_FileName(t *testing.T) { - parent := makeParent(t, "parent-app", git.Base) - - child := makeChildManifest(t, ` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: child-app -spec: - source: - repoURL: https://github.com/org/repo.git - path: apps/child - destination: - namespace: child-ns -`) - - result, err := buildChildArgoResource(child, parent, "argocd") - require.NoError(t, err) - assert.Equal(t, "parent: parent-app", result.FileName, - "child FileName must be a breadcrumb pointing to the parent") -} - -// TestBuildChildArgoResource_InheritsBranch verifies that the child inherits the -// parent's branch type (both Base and Target cases). -func TestBuildChildArgoResource_InheritsBranch(t *testing.T) { - for _, branch := range []git.BranchType{git.Base, git.Target} { - t.Run(string(branch), func(t *testing.T) { - parent := makeParent(t, "parent-app", branch) - - child := makeChildManifest(t, ` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: child-app -spec: - source: - repoURL: https://github.com/org/repo.git - path: apps/child - destination: - namespace: child-ns -`) - - result, err := buildChildArgoResource(child, parent, "argocd") - require.NoError(t, err) - assert.Equal(t, branch, result.Branch, - "child must inherit parent's branch type (%s)", branch) - }) - } -} - -// TestBuildChildArgoResource_IdAndName verifies that Id and Name are set from -// the manifest's metadata.name. -func TestBuildChildArgoResource_IdAndName(t *testing.T) { - parent := makeParent(t, "parent-app", git.Base) - - child := makeChildManifest(t, ` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: my-child-app -spec: - source: - repoURL: https://github.com/org/repo.git - path: apps/child - destination: - namespace: default -`) - - result, err := buildChildArgoResource(child, parent, "argocd") - require.NoError(t, err) - assert.Equal(t, "my-child-app", result.Id) - assert.Equal(t, "my-child-app", result.Name) -} - -// TestBuildChildArgoResource_KindIsApplication verifies the Kind field. -func TestBuildChildArgoResource_KindIsApplication(t *testing.T) { - parent := makeParent(t, "parent-app", git.Base) - - child := makeChildManifest(t, ` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: child-app -spec: - source: - repoURL: https://github.com/org/repo.git - path: apps/child - destination: - namespace: default -`) - - result, err := buildChildArgoResource(child, parent, "argocd") - require.NoError(t, err) - assert.Equal(t, argoapplication.Application, result.Kind) -} - -// TestBuildChildArgoResource_PatchesNamespace verifies that the child's -// namespace is overwritten with the ArgoCD namespace. -func TestBuildChildArgoResource_PatchesNamespace(t *testing.T) { - parent := makeParent(t, "parent-app", git.Base) - - child := makeChildManifest(t, ` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: child-app - namespace: some-other-namespace -spec: - source: - repoURL: https://github.com/org/repo.git - path: apps/child - destination: - namespace: child-ns -`) - - result, err := buildChildArgoResource(child, parent, "my-argocd-ns") - require.NoError(t, err) - assert.Equal(t, "my-argocd-ns", result.Yaml.GetNamespace(), - "child namespace must be overwritten with the ArgoCD namespace") -} - -// TestBuildChildArgoResource_RemovesSyncPolicy verifies that automated sync is -// stripped so the child is never accidentally synced. -func TestBuildChildArgoResource_RemovesSyncPolicy(t *testing.T) { - parent := makeParent(t, "parent-app", git.Base) - - child := makeChildManifest(t, ` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: child-app -spec: - syncPolicy: - automated: - prune: true - selfHeal: true - source: - repoURL: https://github.com/org/repo.git - path: apps/child - destination: - namespace: child-ns -`) - - result, err := buildChildArgoResource(child, parent, "argocd") - require.NoError(t, err) - - _, found, _ := unstructured.NestedMap(result.Yaml.Object, "spec", "syncPolicy") - assert.False(t, found, "syncPolicy must be removed from child Application") -} - -// TestBuildChildArgoResource_SetsProjectToDefault verifies that any custom -// project is replaced with "default". -func TestBuildChildArgoResource_SetsProjectToDefault(t *testing.T) { - parent := makeParent(t, "parent-app", git.Base) - - child := makeChildManifest(t, ` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: child-app -spec: - project: my-restricted-project - source: - repoURL: https://github.com/org/repo.git - path: apps/child - destination: - namespace: child-ns -`) - - result, err := buildChildArgoResource(child, parent, "argocd") - require.NoError(t, err) - - project, _, _ := unstructured.NestedString(result.Yaml.Object, "spec", "project") - assert.Equal(t, "default", project, "project must be reset to 'default'") -} - -// TestBuildChildArgoResource_SetsDestinationServerToLocal verifies that the -// child's destination is always pointed at the local in-cluster server. -func TestBuildChildArgoResource_SetsDestinationServerToLocal(t *testing.T) { - parent := makeParent(t, "parent-app", git.Base) - - child := makeChildManifest(t, ` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: child-app -spec: - source: - repoURL: https://github.com/org/repo.git - path: apps/child - destination: - server: https://some-external-cluster.example.com - namespace: child-ns -`) - - result, err := buildChildArgoResource(child, parent, "argocd") - require.NoError(t, err) - - server, _, _ := unstructured.NestedString(result.Yaml.Object, "spec", "destination", "server") - assert.Equal(t, "https://kubernetes.default.svc", server, - "destination server must be set to the local cluster") -} - -// TestBuildChildArgoResource_RemovesArgoCDFinalizers verifies that the -// resources-finalizer is stripped so the child can be deleted cleanly. -func TestBuildChildArgoResource_RemovesArgoCDFinalizers(t *testing.T) { - parent := makeParent(t, "parent-app", git.Base) - - child := makeChildManifest(t, ` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: child-app - finalizers: - - resources-finalizer.argocd.argoproj.io - - some-other-finalizer -spec: - source: - repoURL: https://github.com/org/repo.git - path: apps/child - destination: - namespace: child-ns -`) - - result, err := buildChildArgoResource(child, parent, "argocd") - require.NoError(t, err) - - finalizers := result.Yaml.GetFinalizers() - assert.NotContains(t, finalizers, "resources-finalizer.argocd.argoproj.io", - "ArgoCD finalizer must be removed") - assert.Contains(t, finalizers, "some-other-finalizer", - "non-ArgoCD finalizers must be preserved") -} - -// TestBuildChildArgoResource_NoFinalizers verifies no panic when no finalizers. -func TestBuildChildArgoResource_NoFinalizers(t *testing.T) { - parent := makeParent(t, "parent-app", git.Base) - - child := makeChildManifest(t, ` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: child-app -spec: - source: - repoURL: https://github.com/org/repo.git - path: apps/child - destination: - namespace: child-ns -`) - - result, err := buildChildArgoResource(child, parent, "argocd") - require.NoError(t, err) - assert.NotNil(t, result) -} - -// TestBuildChildArgoResource_EmptyName verifies that a manifest with no name -// returns an error rather than creating a broken ArgoResource. -func TestBuildChildArgoResource_EmptyName(t *testing.T) { - parent := makeParent(t, "parent-app", git.Base) - - child := makeChildManifest(t, ` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: {} -spec: - source: - repoURL: https://github.com/org/repo.git - path: apps/child -`) - - _, err := buildChildArgoResource(child, parent, "argocd") - assert.Error(t, err, "missing name must return an error") -} - -// TestBuildChildArgoResource_DoesNotMutateOriginal verifies that the original -// manifest is not modified (we deep-copy before patching). -func TestBuildChildArgoResource_DoesNotMutateOriginal(t *testing.T) { - parent := makeParent(t, "parent-app", git.Base) - - child := makeChildManifest(t, ` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: child-app - namespace: original-ns -spec: - project: custom-project - syncPolicy: - automated: {} - source: - repoURL: https://github.com/org/repo.git - path: apps/child - destination: - server: https://external.example.com - namespace: child-ns -`) - - originalNS := child.GetNamespace() - originalProject, _, _ := unstructured.NestedString(child.Object, "spec", "project") - - _, err := buildChildArgoResource(child, parent, "argocd") - require.NoError(t, err) - - // The original manifest must be unchanged. - assert.Equal(t, originalNS, child.GetNamespace(), - "original manifest namespace must not be mutated") - project, _, _ := unstructured.NestedString(child.Object, "spec", "project") - assert.Equal(t, originalProject, project, - "original manifest project must not be mutated") - _, found, _ := unstructured.NestedMap(child.Object, "spec", "syncPolicy") - assert.True(t, found, "original manifest syncPolicy must not be removed") -} - -// ───────────────────────────────────────────────────────────────────────────── -// buildChildFromGeneratedApp -// ───────────────────────────────────────────────────────────────────────────── - -// TestBuildChildFromGeneratedApp_BasicFields verifies that Id, Name, FileName, -// and Branch are set correctly when building from a generated Application. -func TestBuildChildFromGeneratedApp_BasicFields(t *testing.T) { - manifest := makeChildManifest(t, ` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: generated-app -spec: - source: - repoURL: https://github.com/org/repo.git - path: apps/generated - destination: - namespace: default -`) - - breadcrumb := "parent: root (appset: my-appset)" - result, err := buildChildFromGeneratedApp(manifest, breadcrumb, git.Base, "argocd") - require.NoError(t, err) - assert.Equal(t, "generated-app", result.Id) - assert.Equal(t, "generated-app", result.Name) - assert.Equal(t, breadcrumb, result.FileName) - assert.Equal(t, git.Base, result.Branch) - assert.Equal(t, argoapplication.Application, result.Kind) -} - -// TestBuildChildFromGeneratedApp_PatchesApplied verifies that all required -// patches (namespace, project, destination, sync policy, finalizers) are applied. -func TestBuildChildFromGeneratedApp_PatchesApplied(t *testing.T) { - manifest := makeChildManifest(t, ` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: generated-app - namespace: some-other-ns - finalizers: - - resources-finalizer.argocd.argoproj.io -spec: - project: restricted-project - syncPolicy: - automated: - prune: true - source: - repoURL: https://github.com/org/repo.git - path: apps/generated - destination: - server: https://external-cluster.example.com - namespace: default -`) - - result, err := buildChildFromGeneratedApp(manifest, "parent: root (appset: my-appset)", git.Target, "argocd") - require.NoError(t, err) - - assert.Equal(t, "argocd", result.Yaml.GetNamespace(), "namespace must be overwritten with ArgoCD namespace") - - project, _, _ := unstructured.NestedString(result.Yaml.Object, "spec", "project") - assert.Equal(t, "default", project, "project must be reset to 'default'") - - _, found, _ := unstructured.NestedMap(result.Yaml.Object, "spec", "syncPolicy") - assert.False(t, found, "syncPolicy must be removed") - - server, _, _ := unstructured.NestedString(result.Yaml.Object, "spec", "destination", "server") - assert.Equal(t, "https://kubernetes.default.svc", server, "destination server must point to local cluster") - - finalizers := result.Yaml.GetFinalizers() - assert.NotContains(t, finalizers, "resources-finalizer.argocd.argoproj.io", "ArgoCD finalizer must be removed") -} - -// TestBuildChildFromGeneratedApp_EmptyName verifies that a manifest with no name -// returns an error. -func TestBuildChildFromGeneratedApp_EmptyName(t *testing.T) { - manifest := makeChildManifest(t, ` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: {} -spec: - source: - repoURL: https://github.com/org/repo.git - path: apps/generated -`) - - _, err := buildChildFromGeneratedApp(manifest, "parent: root (appset: my-appset)", git.Base, "argocd") - assert.Error(t, err, "missing name must return an error") -} - -// TestBuildChildFromGeneratedApp_DoesNotMutateOriginal verifies that the original -// manifest is not modified (we deep-copy before patching). -func TestBuildChildFromGeneratedApp_DoesNotMutateOriginal(t *testing.T) { - manifest := makeChildManifest(t, ` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: generated-app - namespace: original-ns -spec: - project: custom-project - syncPolicy: - automated: {} - source: - repoURL: https://github.com/org/repo.git - path: apps/generated - destination: - server: https://external.example.com - namespace: default -`) - - originalNS := manifest.GetNamespace() - originalProject, _, _ := unstructured.NestedString(manifest.Object, "spec", "project") - - _, err := buildChildFromGeneratedApp(manifest, "parent: root (appset: my-appset)", git.Base, "argocd") - require.NoError(t, err) - - assert.Equal(t, originalNS, manifest.GetNamespace(), "original manifest namespace must not be mutated") - project, _, _ := unstructured.NestedString(manifest.Object, "spec", "project") - assert.Equal(t, originalProject, project, "original manifest project must not be mutated") - _, found, _ := unstructured.NestedMap(manifest.Object, "spec", "syncPolicy") - assert.True(t, found, "original manifest syncPolicy must not be removed") -} - -// TestBuildChildArgoResource_UsesParentNameBreadcrumb verifies that -// buildChildArgoResource (which wraps buildChildFromGeneratedApp) sets -// FileName to "parent: " - distinct from the appset breadcrumb format. -func TestBuildChildArgoResource_UsesParentNameBreadcrumb(t *testing.T) { - parent := makeParent(t, "root-app", git.Base) - child := makeChildManifest(t, ` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: child-app -spec: - source: - repoURL: https://github.com/org/repo.git - path: apps/child - destination: - namespace: default -`) - - result, err := buildChildArgoResource(child, parent, "argocd") - require.NoError(t, err) - assert.Equal(t, "parent: root-app", result.FileName, - "direct child FileName must be 'parent: '") -} - func TestVisitedKey_Format(t *testing.T) { key := visitedKey("my-app", git.Base) assert.Contains(t, key, "my-app", "key must contain the app ID") @@ -551,11 +48,8 @@ func TestVisitedKey_Deterministic(t *testing.T) { // TestVisitedKey_NoPrefixCollision guards against naive concatenation bugs where // ("ab", "c") == ("a", "bc") if the separator is omitted. func TestVisitedKey_NoPrefixCollision(t *testing.T) { - // Construct two app IDs that share a prefix/suffix with the branch string - // to ensure the separator prevents false equality. key1 := visitedKey("app|base", git.Target) key2 := visitedKey("app", git.Base) - // These are different (id, branch) pairs and must not collide. assert.NotEqual(t, key1, key2, "visitedKey must use a separator that prevents prefix collisions") } From 763287df93bee4ec31423cd69a0e9a21730ca177 Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Tue, 17 Mar 2026 18:57:43 +0100 Subject: [PATCH 04/14] Fix | Include Application/ApplicationSet objects in diff output for app-of-apps --- integration-test/branch-17/target/output.html | 70 ++++++++++++++++++- integration-test/branch-17/target/output.md | 58 ++++++++++++++- pkg/reposerverextract/appofapps.go | 58 +++++++-------- 3 files changed, 153 insertions(+), 33 deletions(-) diff --git a/integration-test/branch-17/target/output.html b/integration-test/branch-17/target/output.html index c8b0622a..07a0bf6b 100644 --- a/integration-test/branch-17/target/output.html +++ b/integration-test/branch-17/target/output.html @@ -63,13 +63,42 @@

Argo CD Diff Preview

+ level-1c-staging-app (+8) + level-2c-app (+8) -Modified (4): +Modified (6): +Β± level-1a-app (+13) Β± level-1b-app (+1|-1) Β± level-1c-prod-app (+1|-1) Β± level-2a-app (+2|-1) -Β± level-2b-app (+1) +Β± level-2b-app (+1) +Β± root-app (+1)
+
+ +level-1a-app (parent: root-app) + + +

Application: argocd/level-2c-app

+
+ + + + + + + + + + + + + + + +
+apiVersion: argoproj.io/v1alpha1
+kind: Application
+metadata:
+  name: level-2c-app
+  namespace: argocd
+spec:
+  destination:
+    name: in-cluster
+    namespace: default
+  project: default
+  source:
+    path: examples/app-of-apps/apps/level-2c
+    repoURL: https://github.com/dag-andersen/argocd-diff-preview
+
+ +
+
level-1b-app (parent: root-app) @@ -163,6 +192,41 @@

ConfigMap: default/level-2b-config

+
+ +root-app (examples/app-of-apps/root-app.yaml) + + +

ApplicationSet: argocd/level-1c-appset

+
+ + + + + + + + + + + + + + + + + + + + + + + +
 apiVersion: argoproj.io/v1alpha1
 kind: ApplicationSet
 metadata:
   name: level-1c-appset
   namespace: argocd
 spec:
   generators:
   - list:
       elements:
       - env: prod
+      - env: staging
   template:
     metadata:
       name: level-1c-{{env}}-app
       namespace: argocd
     spec:
       destination:
         name: in-cluster
         namespace: default
       project: default
       source:
+
+ +
+
level-1c-staging-app (parent: root-app (appset: level-1c-appset)) @@ -209,7 +273,7 @@

ConfigMap: default/level-2c-config

_Stats_:
-[Applications: 2], [Full Run: Xs], [Rendering: Xs], [Cluster: Xs], [Argo CD: Xs]
+[Applications: 14], [Full Run: Xs], [Rendering: Xs], [Cluster: Xs], [Argo CD: Xs] diff --git a/integration-test/branch-17/target/output.md b/integration-test/branch-17/target/output.md index a9d70ce0..23cdeff1 100644 --- a/integration-test/branch-17/target/output.md +++ b/integration-test/branch-17/target/output.md @@ -6,13 +6,37 @@ Added (2): + level-1c-staging-app (+8) + level-2c-app (+8) -Modified (4): +Modified (6): +Β± level-1a-app (+13) Β± level-1b-app (+1|-1) Β± level-1c-prod-app (+1|-1) Β± level-2a-app (+2|-1) Β± level-2b-app (+1) +Β± root-app (+1) ``` +
+level-1a-app (parent: root-app) +
+ +#### Application: argocd/level-2c-app +```diff ++apiVersion: argoproj.io/v1alpha1 ++kind: Application ++metadata: ++ name: level-2c-app ++ namespace: argocd ++spec: ++ destination: ++ name: in-cluster ++ namespace: default ++ project: default ++ source: ++ path: examples/app-of-apps/apps/level-2c ++ repoURL: https://github.com/dag-andersen/argocd-diff-preview +``` +
+
level-1b-app (parent: root-app)
@@ -86,6 +110,36 @@ Modified (4): ```
+
+root-app (examples/app-of-apps/root-app.yaml) +
+ +#### ApplicationSet: argocd/level-1c-appset +```diff + apiVersion: argoproj.io/v1alpha1 + kind: ApplicationSet + metadata: + name: level-1c-appset + namespace: argocd + spec: + generators: + - list: + elements: + - env: prod ++ - env: staging + template: + metadata: + name: level-1c-{{env}}-app + namespace: argocd + spec: + destination: + name: in-cluster + namespace: default + project: default + source: +``` +
+
level-1c-staging-app (parent: root-app (appset: level-1c-appset))
@@ -121,4 +175,4 @@ Modified (4):
_Stats_: -[Applications: 2], [Full Run: Xs], [Rendering: Xs], [Cluster: Xs], [Argo CD: Xs] +[Applications: 14], [Full Run: Xs], [Rendering: Xs], [Cluster: Xs], [Argo CD: Xs] diff --git a/pkg/reposerverextract/appofapps.go b/pkg/reposerverextract/appofapps.go index 38181cbf..a2f491d9 100644 --- a/pkg/reposerverextract/appofapps.go +++ b/pkg/reposerverextract/appofapps.go @@ -334,20 +334,22 @@ func RenderApplicationsFromBothBranchesWithAppOfApps( return extractedBaseApps, extractedTargetApps, time.Since(startTime), nil } -// renderAppWithChildDiscovery renders a single application and separates the -// resulting manifests into two groups: +// renderAppWithChildDiscovery renders a single application and returns: // -// 1. Regular Kubernetes manifests (returned as the first value) – these are -// included in the parent application's diff output. +// 1. All rendered manifests to include in the diff (returned as the first +// value). This is the original allManifests slice returned by the repo +// server, including Application/ApplicationSet objects as-is so that +// changes to those objects (e.g. annotation changes) are visible in the +// diff output. // -// 2. Child Application resources (returned as the second value) – these are -// patched and queued for recursive rendering. They are excluded from the -// parent's manifest list so that the diff only shows the actual cluster -// resources they produce, not the Application objects themselves. +// 2. Child ArgoResource values to enqueue for recursive rendering (returned as +// the second value). Application resources are deep-copied, patched, and +// enqueued directly. ApplicationSet resources are deep-copied, patched, and +// then expanded into their generated Applications via +// argocd.AppsetGenerateWithRetry. In both cases the deep copy ensures +// PatchApplication does not mutate the originals in allManifests. // -// ApplicationSet resources found in rendered manifests are expanded into their -// generated Applications via argocd.AppsetGenerateWithRetry, then treated the -// same as directly-discovered child Applications. +// A visited set in the caller prevents re-rendering the same child twice. func renderAppWithChildDiscovery( ctx context.Context, repoClient *reposerver.Client, @@ -367,13 +369,15 @@ func renderAppWithChildDiscovery( return nil, nil, err } - // ── Separate regular manifests from child Application/ApplicationSet resources ────────── - var regularManifests []unstructured.Unstructured + // Discover child Application/ApplicationSet resources in the rendered manifests. + // Non-argoproj.io resources are left untouched in allManifests. + // For Application/ApplicationSet entries we deep-copy the slot in allManifests + // so the diff sees the original unmodified YAML, then use the original m freely + // for patching (PatchApplication mutates the Yaml pointer in-place). var childApps []argoapplication.ArgoResource for _, m := range allManifests { if !strings.HasPrefix(m.GetAPIVersion(), "argoproj.io/") { - regularManifests = append(regularManifests, m) continue } @@ -381,17 +385,19 @@ func renderAppWithChildDiscovery( case "Application": name := m.GetName() if name == "" { - log.Warn().Str("parentApp", app.GetLongName()).Msg("⚠️ Discovered child Application has no name; skipping") + log.Warn().Str("parentApp", app.GetLongName()).Msg("⚠️ Discovered child Application has no name; skipping child rendering") continue } fileName := fmt.Sprintf("parent: %s", app.Name) - resource := argoapplication.NewArgoResource(&m, argoapplication.Application, name, name, fileName, app.Branch) + // Deep copy so PatchApplication mutates the copy, leaving m in + // allManifests (the diff) untouched. + resource := argoapplication.NewArgoResource(m.DeepCopy(), argoapplication.Application, name, name, fileName, app.Branch) child, err := argoapplication.PatchApplication(argocdNamespace, *resource, branchByType[app.Branch], prRepo, nil) if err != nil { log.Warn().Err(err). Str("parentApp", app.GetLongName()). Str("childName", name). - Msg("⚠️ Could not patch child Application; skipping") + Msg("⚠️ Could not patch child Application; skipping child rendering") continue } childApps = append(childApps, *child) @@ -401,8 +407,6 @@ func renderAppWithChildDiscovery( Msg("Discovered child Application via app-of-apps pattern") case "ApplicationSet": - // Expand the ApplicationSet into its generated Applications using the - // ArgoCD API/CLI, then treat each generated app as a child to enqueue. appSetName := m.GetName() log.Info(). Str("parentApp", app.GetLongName()). @@ -412,16 +416,17 @@ func renderAppWithChildDiscovery( appSetTempFolder := fmt.Sprintf("%s/appsets/depth-%d", tempFolder, depth) branch := branchByType[app.Branch] - // Patch the ApplicationSet the same way top-level ApplicationSets are patched - // before being sent to the API. This strips spec.template.metadata.namespace + // Patch before sending to the API. This strips spec.template.metadata.namespace // (e.g. "argocd") which ArgoCD's /api/v1/applicationsets/generate endpoint rejects. - appSetResource := argoapplication.NewArgoResource(&m, argoapplication.ApplicationSet, appSetName, appSetName, app.FileName, app.Branch) + // Deep copy so PatchApplication mutates the copy, leaving m in + // allManifests (the diff) untouched. + appSetResource := argoapplication.NewArgoResource(m.DeepCopy(), argoapplication.ApplicationSet, appSetName, appSetName, app.FileName, app.Branch) patchedAppSet, err := argoapplication.PatchApplication(argocdNamespace, *appSetResource, branch, prRepo, nil) if err != nil { log.Warn().Err(err). Str("parentApp", app.GetLongName()). Str("appSet", appSetName). - Msg("⚠️ Failed to patch child ApplicationSet before expansion; skipping") + Msg("⚠️ Failed to patch child ApplicationSet before expansion; skipping expansion") continue } @@ -430,7 +435,7 @@ func renderAppWithChildDiscovery( log.Warn().Err(err). Str("parentApp", app.GetLongName()). Str("appSet", appSetName). - Msg("⚠️ Could not expand child ApplicationSet; skipping") + Msg("⚠️ Could not expand child ApplicationSet; skipping expansion") continue } @@ -464,9 +469,6 @@ func renderAppWithChildDiscovery( Str("childApp", child.GetLongName()). Msg("Discovered child Application via ApplicationSet expansion") } - - default: - regularManifests = append(regularManifests, m) } } @@ -476,5 +478,5 @@ func renderAppWithChildDiscovery( Msgf("πŸ” Discovered %d child Application(s) in rendered manifests", len(childApps)) } - return regularManifests, childApps, nil + return allManifests, childApps, nil } From 4ad3991d28166962c77662917a843169c1c35640 Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Tue, 17 Mar 2026 19:44:47 +0100 Subject: [PATCH 05/14] Fix | Use namespace/name as visited key in app-of-apps traversal to prevent duplicate rendering --- pkg/reposerverextract/appofapps.go | 20 ++++++---- pkg/reposerverextract/appofapps_test.go | 49 +++++++++++++++++-------- 2 files changed, 47 insertions(+), 22 deletions(-) diff --git a/pkg/reposerverextract/appofapps.go b/pkg/reposerverextract/appofapps.go index a2f491d9..154d911b 100644 --- a/pkg/reposerverextract/appofapps.go +++ b/pkg/reposerverextract/appofapps.go @@ -59,11 +59,17 @@ type renderResult struct { err error } -// visitedKey returns a unique string key for an (appID, branch) pair, used to -// track which applications have already been rendered during app-of-apps -// expansion. -func visitedKey(id string, branch git.BranchType) string { - return id + "|" + string(branch) +// visitedKey returns a unique string key for a (namespace, name, branch) +// triple, used to track which applications have already been rendered during +// app-of-apps expansion. Using the raw Kubernetes namespace/name (rather than +// the deduplicated Id) ensures that a child Application discovered via +// traversal is recognised as already-visited even when the same app was +// previously seeded as a top-level app whose Id was made unique (e.g. "root" +// became "root--1"). +func visitedKey(namespace, name string, branch git.BranchType) string { + // Use \x00 (null byte) as separator: it cannot appear in Kubernetes + // namespace or name values, so there is no risk of prefix collision. + return namespace + "\x00" + name + "\x00" + string(branch) } // RenderApplicationsFromBothBranchesWithAppOfApps is like @@ -188,7 +194,7 @@ func RenderApplicationsFromBothBranchesWithAppOfApps( // Seed the queue with the initial base + target apps (depth 0). visitedMu.Lock() for _, app := range append(baseApps, targetApps...) { - visited[visitedKey(app.Id, app.Branch)] = true + visited[visitedKey(app.Yaml.GetNamespace(), app.Yaml.GetName(), app.Branch)] = true enqueue(app, 0) } visitedMu.Unlock() @@ -258,7 +264,7 @@ func RenderApplicationsFromBothBranchesWithAppOfApps( } visitedMu.Lock() for _, child := range selection.SelectedApps { - key := visitedKey(child.Id, child.Branch) + key := visitedKey(child.Yaml.GetNamespace(), child.Yaml.GetName(), child.Branch) if visited[key] { log.Debug().Str("App", child.GetLongName()).Msg("Skipping already-visited child Application") continue diff --git a/pkg/reposerverextract/appofapps_test.go b/pkg/reposerverextract/appofapps_test.go index b43aeeff..5624bdcc 100644 --- a/pkg/reposerverextract/appofapps_test.go +++ b/pkg/reposerverextract/appofapps_test.go @@ -20,36 +20,55 @@ import ( ) func TestVisitedKey_Format(t *testing.T) { - key := visitedKey("my-app", git.Base) - assert.Contains(t, key, "my-app", "key must contain the app ID") + key := visitedKey("argocd", "my-app", git.Base) + assert.Contains(t, key, "argocd", "key must contain the namespace") + assert.Contains(t, key, "my-app", "key must contain the app name") assert.Contains(t, key, string(git.Base), "key must contain the branch type") } -func TestVisitedKey_DifferentIDs(t *testing.T) { - key1 := visitedKey("app-a", git.Base) - key2 := visitedKey("app-b", git.Base) +func TestVisitedKey_DifferentNames(t *testing.T) { + key1 := visitedKey("argocd", "app-a", git.Base) + key2 := visitedKey("argocd", "app-b", git.Base) assert.NotEqual(t, key1, key2, - "different app IDs on the same branch must produce different keys") + "different app names in the same namespace and branch must produce different keys") +} + +func TestVisitedKey_DifferentNamespaces(t *testing.T) { + key1 := visitedKey("argocd", "my-app", git.Base) + key2 := visitedKey("other", "my-app", git.Base) + assert.NotEqual(t, key1, key2, + "same app name in different namespaces must produce different keys") } func TestVisitedKey_DifferentBranches(t *testing.T) { - key1 := visitedKey("my-app", git.Base) - key2 := visitedKey("my-app", git.Target) + key1 := visitedKey("argocd", "my-app", git.Base) + key2 := visitedKey("argocd", "my-app", git.Target) assert.NotEqual(t, key1, key2, - "same app ID on different branches must produce different keys") + "same app in different branches must produce different keys") } func TestVisitedKey_Deterministic(t *testing.T) { - key1 := visitedKey("my-app", git.Base) - key2 := visitedKey("my-app", git.Base) + key1 := visitedKey("argocd", "my-app", git.Base) + key2 := visitedKey("argocd", "my-app", git.Base) assert.Equal(t, key1, key2, "visitedKey must be deterministic") } // TestVisitedKey_NoPrefixCollision guards against naive concatenation bugs where -// ("ab", "c") == ("a", "bc") if the separator is omitted. +// two different (namespace, name) pairs produce the same key if no separator is used. func TestVisitedKey_NoPrefixCollision(t *testing.T) { - key1 := visitedKey("app|base", git.Target) - key2 := visitedKey("app", git.Base) + // namespace="argo-cd", name="app" must not equal namespace="argo", name="cd-app" + key1 := visitedKey("argo-cd", "app", git.Base) + key2 := visitedKey("argo", "cd-app", git.Base) assert.NotEqual(t, key1, key2, - "visitedKey must use a separator that prevents prefix collisions") + "visitedKey must use separators that prevent prefix collisions between namespace and name") +} + +// TestVisitedKey_SameKubernetesIdentity verifies that two apps with identical +// namespace/name/branch produce the same key regardless of their deduplicated Id. +// This is the core property that prevents the triple-root duplicate bug. +func TestVisitedKey_SameKubernetesIdentity(t *testing.T) { + key1 := visitedKey("argocd", "root", git.Target) + key2 := visitedKey("argocd", "root", git.Target) + assert.Equal(t, key1, key2, + "apps with the same namespace/name/branch must share a visited key even if their Ids differ") } From 927bafe3dcff7932e6d74a88da799455ce2a9724 Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Tue, 17 Mar 2026 20:43:40 +0100 Subject: [PATCH 06/14] Fix | Include spec hash in app-of-apps visited key to handle same-name apps with different content --- pkg/reposerverextract/appofapps.go | 59 +++++++++--- pkg/reposerverextract/appofapps_test.go | 122 ++++++++++++++++++++---- 2 files changed, 154 insertions(+), 27 deletions(-) diff --git a/pkg/reposerverextract/appofapps.go b/pkg/reposerverextract/appofapps.go index 154d911b..867a6633 100644 --- a/pkg/reposerverextract/appofapps.go +++ b/pkg/reposerverextract/appofapps.go @@ -10,6 +10,8 @@ package reposerverextract import ( "context" + "crypto/sha256" + "encoding/json" "fmt" "strings" "sync" @@ -59,17 +61,52 @@ type renderResult struct { err error } -// visitedKey returns a unique string key for a (namespace, name, branch) -// triple, used to track which applications have already been rendered during -// app-of-apps expansion. Using the raw Kubernetes namespace/name (rather than -// the deduplicated Id) ensures that a child Application discovered via -// traversal is recognised as already-visited even when the same app was -// previously seeded as a top-level app whose Id was made unique (e.g. "root" -// became "root--1"). -func visitedKey(namespace, name string, branch git.BranchType) string { +// visitedKey returns a unique string key for an application, used to track +// which applications have already been rendered during app-of-apps expansion. +// +// The key is based on (namespace, name, branch, specHash) where specHash is a +// SHA-256 of the app's spec field. This handles two distinct scenarios: +// +// 1. Same k8s identity, same content - a child app discovered via traversal +// that matches a top-level seed app (even if the seed's Id was deduplicated +// from "root" to "root-1"). The namespace/name/branch/specHash will all +// match, so it is correctly recognised as already-visited. +// +// 2. Same k8s identity, different content - two different files both define an +// Application named "root" but with different spec.source.path. These must +// be rendered separately; the differing specHash ensures they get distinct +// keys. +func visitedKey(yaml *unstructured.Unstructured, branch git.BranchType) string { + namespace := yaml.GetNamespace() + name := yaml.GetName() + + // Hash the spec field so that two apps with the same namespace/name but + // different source configurations are treated as distinct entries. + // Fall back to an empty hash if spec is absent or cannot be marshalled. + specHash := specHashOf(yaml) + // Use \x00 (null byte) as separator: it cannot appear in Kubernetes // namespace or name values, so there is no risk of prefix collision. - return namespace + "\x00" + name + "\x00" + string(branch) + return namespace + "\x00" + name + "\x00" + string(branch) + "\x00" + specHash +} + +// specHashOf returns a short hex-encoded SHA-256 hash of the "spec" field of +// the given unstructured object. If the spec is absent or cannot be marshalled +// to JSON, an empty string is returned (callers treat it as a valid hash). +func specHashOf(yaml *unstructured.Unstructured) string { + if yaml == nil { + return "" + } + spec, found, _ := unstructured.NestedMap(yaml.Object, "spec") + if !found { + return "" + } + b, err := json.Marshal(spec) + if err != nil { + return "" + } + sum := sha256.Sum256(b) + return fmt.Sprintf("%x", sum[:8]) // 16 hex chars - enough for deduplication } // RenderApplicationsFromBothBranchesWithAppOfApps is like @@ -194,7 +231,7 @@ func RenderApplicationsFromBothBranchesWithAppOfApps( // Seed the queue with the initial base + target apps (depth 0). visitedMu.Lock() for _, app := range append(baseApps, targetApps...) { - visited[visitedKey(app.Yaml.GetNamespace(), app.Yaml.GetName(), app.Branch)] = true + visited[visitedKey(app.Yaml, app.Branch)] = true enqueue(app, 0) } visitedMu.Unlock() @@ -264,7 +301,7 @@ func RenderApplicationsFromBothBranchesWithAppOfApps( } visitedMu.Lock() for _, child := range selection.SelectedApps { - key := visitedKey(child.Yaml.GetNamespace(), child.Yaml.GetName(), child.Branch) + key := visitedKey(child.Yaml, child.Branch) if visited[key] { log.Debug().Str("App", child.GetLongName()).Msg("Skipping already-visited child Application") continue diff --git a/pkg/reposerverextract/appofapps_test.go b/pkg/reposerverextract/appofapps_test.go index 5624bdcc..f5c841e2 100644 --- a/pkg/reposerverextract/appofapps_test.go +++ b/pkg/reposerverextract/appofapps_test.go @@ -7,6 +7,7 @@ package reposerverextract // helper functions that can be exercised without any network or cluster: // // - visitedKey – produces a unique deduplication key. +// - specHashOf – stable content hash of the spec field. // // Patching logic for discovered child Applications and ApplicationSets is // delegated entirely to argoapplication.PatchApplication, which is tested in @@ -17,39 +18,81 @@ import ( "github.com/dag-andersen/argocd-diff-preview/pkg/git" "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) +// makeUnstructuredApp builds a minimal *unstructured.Unstructured representing +// an ArgoCD Application with the given namespace, name, and spec fields. It is +// used to construct test inputs for visitedKey and specHashOf. +func makeUnstructuredApp(namespace, name string, spec map[string]any) *unstructured.Unstructured { + obj := map[string]any{ + "apiVersion": "argoproj.io/v1alpha1", + "kind": "Application", + "metadata": map[string]any{ + "name": name, + "namespace": namespace, + }, + } + if spec != nil { + obj["spec"] = spec + } + return &unstructured.Unstructured{Object: obj} +} + +func specA() map[string]any { + return map[string]any{ + "source": map[string]any{ + "repoURL": "https://github.com/example/repo", + "path": "apps/path-a", + "targetRevision": "main", + }, + } +} + +func specB() map[string]any { + return map[string]any{ + "source": map[string]any{ + "repoURL": "https://github.com/example/repo", + "path": "apps/path-b", + "targetRevision": "main", + }, + } +} + +// ── visitedKey ──────────────────────────────────────────────────────────────── + func TestVisitedKey_Format(t *testing.T) { - key := visitedKey("argocd", "my-app", git.Base) + app := makeUnstructuredApp("argocd", "my-app", specA()) + key := visitedKey(app, git.Base) assert.Contains(t, key, "argocd", "key must contain the namespace") assert.Contains(t, key, "my-app", "key must contain the app name") assert.Contains(t, key, string(git.Base), "key must contain the branch type") } func TestVisitedKey_DifferentNames(t *testing.T) { - key1 := visitedKey("argocd", "app-a", git.Base) - key2 := visitedKey("argocd", "app-b", git.Base) + key1 := visitedKey(makeUnstructuredApp("argocd", "app-a", specA()), git.Base) + key2 := visitedKey(makeUnstructuredApp("argocd", "app-b", specA()), git.Base) assert.NotEqual(t, key1, key2, "different app names in the same namespace and branch must produce different keys") } func TestVisitedKey_DifferentNamespaces(t *testing.T) { - key1 := visitedKey("argocd", "my-app", git.Base) - key2 := visitedKey("other", "my-app", git.Base) + key1 := visitedKey(makeUnstructuredApp("argocd", "my-app", specA()), git.Base) + key2 := visitedKey(makeUnstructuredApp("other", "my-app", specA()), git.Base) assert.NotEqual(t, key1, key2, "same app name in different namespaces must produce different keys") } func TestVisitedKey_DifferentBranches(t *testing.T) { - key1 := visitedKey("argocd", "my-app", git.Base) - key2 := visitedKey("argocd", "my-app", git.Target) + key1 := visitedKey(makeUnstructuredApp("argocd", "my-app", specA()), git.Base) + key2 := visitedKey(makeUnstructuredApp("argocd", "my-app", specA()), git.Target) assert.NotEqual(t, key1, key2, "same app in different branches must produce different keys") } func TestVisitedKey_Deterministic(t *testing.T) { - key1 := visitedKey("argocd", "my-app", git.Base) - key2 := visitedKey("argocd", "my-app", git.Base) + key1 := visitedKey(makeUnstructuredApp("argocd", "my-app", specA()), git.Base) + key2 := visitedKey(makeUnstructuredApp("argocd", "my-app", specA()), git.Base) assert.Equal(t, key1, key2, "visitedKey must be deterministic") } @@ -57,18 +100,65 @@ func TestVisitedKey_Deterministic(t *testing.T) { // two different (namespace, name) pairs produce the same key if no separator is used. func TestVisitedKey_NoPrefixCollision(t *testing.T) { // namespace="argo-cd", name="app" must not equal namespace="argo", name="cd-app" - key1 := visitedKey("argo-cd", "app", git.Base) - key2 := visitedKey("argo", "cd-app", git.Base) + key1 := visitedKey(makeUnstructuredApp("argo-cd", "app", specA()), git.Base) + key2 := visitedKey(makeUnstructuredApp("argo", "cd-app", specA()), git.Base) assert.NotEqual(t, key1, key2, "visitedKey must use separators that prevent prefix collisions between namespace and name") } // TestVisitedKey_SameKubernetesIdentity verifies that two apps with identical -// namespace/name/branch produce the same key regardless of their deduplicated Id. -// This is the core property that prevents the triple-root duplicate bug. +// namespace/name/branch/spec produce the same key regardless of their +// deduplicated Id. This is the core property that prevents the triple-root +// duplicate bug: a child app discovered via traversal is recognised as +// already-visited even when the same seed app had its Id renamed (e.g. +// "root" -> "root-1"). func TestVisitedKey_SameKubernetesIdentity(t *testing.T) { - key1 := visitedKey("argocd", "root", git.Target) - key2 := visitedKey("argocd", "root", git.Target) + key1 := visitedKey(makeUnstructuredApp("argocd", "root", specA()), git.Target) + key2 := visitedKey(makeUnstructuredApp("argocd", "root", specA()), git.Target) assert.Equal(t, key1, key2, - "apps with the same namespace/name/branch must share a visited key even if their Ids differ") + "apps with the same namespace/name/branch/spec must share a visited key even if their Ids differ") +} + +// TestVisitedKey_SameNameDifferentSpec is the core test for the new behaviour: +// two apps with the same namespace and name but different spec content must +// produce different visited keys so that both get rendered. +func TestVisitedKey_SameNameDifferentSpec(t *testing.T) { + key1 := visitedKey(makeUnstructuredApp("argocd", "root", specA()), git.Base) + key2 := visitedKey(makeUnstructuredApp("argocd", "root", specB()), git.Base) + assert.NotEqual(t, key1, key2, + "apps with the same namespace/name/branch but different spec must have different visited keys") +} + +// ── specHashOf ──────────────────────────────────────────────────────────────── + +func TestSpecHashOf_Deterministic(t *testing.T) { + app := makeUnstructuredApp("argocd", "my-app", specA()) + assert.Equal(t, specHashOf(app), specHashOf(app), "specHashOf must be deterministic") +} + +func TestSpecHashOf_DifferentSpec(t *testing.T) { + appA := makeUnstructuredApp("argocd", "my-app", specA()) + appB := makeUnstructuredApp("argocd", "my-app", specB()) + assert.NotEqual(t, specHashOf(appA), specHashOf(appB), + "different spec fields must produce different hashes") +} + +func TestSpecHashOf_SameSpec(t *testing.T) { + appA := makeUnstructuredApp("argocd", "my-app", specA()) + appB := makeUnstructuredApp("other-ns", "other-name", specA()) + assert.Equal(t, specHashOf(appA), specHashOf(appB), + "identical spec fields must produce the same hash regardless of namespace/name") +} + +func TestSpecHashOf_NoSpec(t *testing.T) { + app := makeUnstructuredApp("argocd", "my-app", nil) + // Should not panic and should return a consistent (empty) value. + hash1 := specHashOf(app) + hash2 := specHashOf(app) + assert.Equal(t, hash1, hash2, "specHashOf with no spec must be deterministic") +} + +func TestSpecHashOf_NilYaml(t *testing.T) { + // Should not panic. + assert.NotPanics(t, func() { specHashOf(nil) }) } From 5d2db4d651865302d9ef3d1f6df3300902fb38db Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Tue, 17 Mar 2026 20:43:43 +0100 Subject: [PATCH 07/14] Chore | Make container restart/crash-loop warnings more visible --- pkg/k8s/crash_watcher.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/k8s/crash_watcher.go b/pkg/k8s/crash_watcher.go index 6b8e7064..1bcd4544 100644 --- a/pkg/k8s/crash_watcher.go +++ b/pkg/k8s/crash_watcher.go @@ -117,12 +117,12 @@ func (c *Client) WatchForContainerRestarts(namespace, labelSelector string, poll switch event.EventType { case "restart": log.Warn().Msgf( - "⚠️ Container '%s' in pod '%s' has restarted (restarts: %d -> %d). This may cause rendering failures or timeouts.", + "🚨🚨🚨🚨🚨 Container '%s' in pod '%s' has restarted (restarts: %d -> %d). This may cause rendering failures or timeouts.", event.ContainerName, event.PodName, event.PrevRestarts, event.CurrRestarts, ) case "crash-loop": log.Warn().Msgf( - "⚠️ Container '%s' in pod '%s' is in CrashLoopBackOff. ArgoCD may not be functioning correctly.", + "🚨🚨🚨🚨🚨 Container '%s' in pod '%s' is in CrashLoopBackOff. ArgoCD may not be functioning correctly.", event.ContainerName, event.PodName, ) } From b4c2dcc840d84d3779473ea3b3a6bbc1b7ac9092 Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Tue, 17 Mar 2026 21:48:10 +0100 Subject: [PATCH 08/14] Fix | Use content similarity to pair duplicate-identity apps in MatchApps --- pkg/matching/apps.go | 81 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 63 insertions(+), 18 deletions(-) diff --git a/pkg/matching/apps.go b/pkg/matching/apps.go index a905fe3b..587e44e7 100644 --- a/pkg/matching/apps.go +++ b/pkg/matching/apps.go @@ -61,15 +61,8 @@ func MatchApps(baseApps, targetApps []extract.ExtractedApp) []Pair { for _, key := range sortedIdentityKeys { baseIdxs := baseByIdentity[key] targetIdxs := targetByIdentity[key] - - matchLen := min(len(baseIdxs), len(targetIdxs)) - for i := range matchLen { - bi := baseIdxs[i] - ti := targetIdxs[i] - pairs = append(pairs, Pair{ - Base: &baseApps[bi], - Target: &targetApps[ti], - }) + for bi, ti := range pairByContent(baseApps, targetApps, baseIdxs, targetIdxs) { + pairs = append(pairs, Pair{Base: &baseApps[bi], Target: &targetApps[ti]}) matchedBase[bi] = true matchedTarget[ti] = true } @@ -102,15 +95,8 @@ func MatchApps(baseApps, targetApps []extract.ExtractedApp) []Pair { for _, name := range sortedBaseNames { baseIdxs := baseByName[name] targetIdxs := targetByName[name] - - matchLen := min(len(baseIdxs), len(targetIdxs)) - for i := range matchLen { - bi := baseIdxs[i] - ti := targetIdxs[i] - pairs = append(pairs, Pair{ - Base: &baseApps[bi], - Target: &targetApps[ti], - }) + for bi, ti := range pairByContent(baseApps, targetApps, baseIdxs, targetIdxs) { + pairs = append(pairs, Pair{Base: &baseApps[bi], Target: &targetApps[ti]}) matchedBase[bi] = true matchedTarget[ti] = true } @@ -165,6 +151,65 @@ func MatchApps(baseApps, targetApps []extract.ExtractedApp) []Pair { return pairs } +// pairByContent pairs base and target indices using content similarity when +// there are multiple candidates sharing the same identity key or name. +// +// When there is only one base and one target candidate the result is trivial +// (no similarity computation needed). When there are multiple candidates on +// either side, greedy similarity matching is used so that e.g. two apps that +// share a name and SourcePath but render completely different resources (such +// as a prod and a test variant discovered via app-of-apps traversal from two +// different root apps) are still paired correctly rather than arbitrarily by +// insertion order. +// +// Returns a map of baseIdx β†’ targetIdx for each matched pair. +func pairByContent(baseApps, targetApps []extract.ExtractedApp, baseIdxs, targetIdxs []int) map[int]int { + result := make(map[int]int) + if len(baseIdxs) == 0 || len(targetIdxs) == 0 { + return result + } + + // Fast path: single candidate on each side - no need for similarity. + if len(baseIdxs) == 1 && len(targetIdxs) == 1 { + result[baseIdxs[0]] = targetIdxs[0] + return result + } + + // Multiple candidates: rank all pairs by content similarity and greedily + // pick the best non-overlapping matches. + type candidate struct { + bi, ti int + score float64 + } + var candidates []candidate + for _, bi := range baseIdxs { + for _, ti := range targetIdxs { + score := resourceSetSimilarity(baseApps[bi].Manifests, targetApps[ti].Manifests) + candidates = append(candidates, candidate{bi, ti, score}) + } + } + sort.SliceStable(candidates, func(i, j int) bool { + if math.Abs(candidates[i].score-candidates[j].score) > 1e-9 { + return candidates[i].score > candidates[j].score + } + if candidates[i].bi != candidates[j].bi { + return candidates[i].bi < candidates[j].bi + } + return candidates[i].ti < candidates[j].ti + }) + + usedBase := make(map[int]bool) + usedTarget := make(map[int]bool) + for _, c := range candidates { + if !usedBase[c.bi] && !usedTarget[c.ti] { + result[c.bi] = c.ti + usedBase[c.bi] = true + usedTarget[c.ti] = true + } + } + return result +} + // matchAppsBySimilarity finds best matches for apps using content similarity func matchAppsBySimilarity( baseApps, targetApps []extract.ExtractedApp, From cad8407c90c566b5544beda3ed9cd43454bc16cd Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Tue, 17 Mar 2026 23:00:24 +0100 Subject: [PATCH 09/14] Add integration_test branch-17/target-2 --- .../{target => target-1}/output.html | 0 .../branch-17/{target => target-1}/output.md | 0 .../branch-17/target-2/output.html | 279 ++++++++++++++++++ integration-test/branch-17/target-2/output.md | 178 +++++++++++ integration-test/integration_test.go | 15 +- 5 files changed, 471 insertions(+), 1 deletion(-) rename integration-test/branch-17/{target => target-1}/output.html (100%) rename integration-test/branch-17/{target => target-1}/output.md (100%) create mode 100644 integration-test/branch-17/target-2/output.html create mode 100644 integration-test/branch-17/target-2/output.md diff --git a/integration-test/branch-17/target/output.html b/integration-test/branch-17/target-1/output.html similarity index 100% rename from integration-test/branch-17/target/output.html rename to integration-test/branch-17/target-1/output.html diff --git a/integration-test/branch-17/target/output.md b/integration-test/branch-17/target-1/output.md similarity index 100% rename from integration-test/branch-17/target/output.md rename to integration-test/branch-17/target-1/output.md diff --git a/integration-test/branch-17/target-2/output.html b/integration-test/branch-17/target-2/output.html new file mode 100644 index 00000000..f80b573a --- /dev/null +++ b/integration-test/branch-17/target-2/output.html @@ -0,0 +1,279 @@ + + + + + + +
+

Argo CD Diff Preview

+ +

Summary:

+
Added (2):
++ level-1c-staging-app (+8)
++ level-2c-app (+8)
+
+Modified (6):
+Β± level-1a-app (+13)
+Β± level-1b-app (+1|-1)
+Β± level-1c-prod-app (+1|-1)
+Β± level-2a-app (+2|-1)
+Β± level-2b-app (+1)
+Β± root-app (+1)
+ +
+
+ +level-1a-app (examples/app-of-apps/apps/root/level-1a.yaml) + + +

Application: argocd/level-2c-app

+
+ + + + + + + + + + + + + + + +
+apiVersion: argoproj.io/v1alpha1
+kind: Application
+metadata:
+  name: level-2c-app
+  namespace: argocd
+spec:
+  destination:
+    name: in-cluster
+    namespace: default
+  project: default
+  source:
+    path: examples/app-of-apps/apps/level-2c
+    repoURL: https://github.com/dag-andersen/argocd-diff-preview
+
+ +
+ +
+ +level-1b-app (examples/app-of-apps/apps/root/level-1b.yaml) + + +

ConfigMap: default/level-1b-config

+
+ + + + + + + + + + + +
 apiVersion: v1
 data:
   app: level-1b
-  color: blue
+  color: purple
 kind: ConfigMap
 metadata:
   name: level-1b-config
   namespace: default
+
+ +
+ +
+ +level-1c-prod-app (examples/app-of-apps/apps/root/level-1c-appset.yaml) + + +

ConfigMap: default/level-1c-prod-config

+
+ + + + + + + + + + + +
 apiVersion: v1
 data:
   app: level-1c-prod
-  color: blue
+  color: purple
 kind: ConfigMap
 metadata:
   name: level-1c-prod-config
   namespace: default
+
+ +
+ +
+ +level-2a-app (examples/app-of-apps/apps/level-1a/level-2a.yaml) + + +

ConfigMap: default/level-2a-config

+
+ + + + + + + + + + + + +
 apiVersion: v1
 data:
   app: level-2a
-  color: green
+  color: yellow
+  environment: production
 kind: ConfigMap
 metadata:
   name: level-2a-config
   namespace: default
+
+ +
+ +
+ +level-2b-app (examples/app-of-apps/apps/level-1a/level-2b.yaml) + + +

ConfigMap: default/level-2b-config

+
+ + + + + + + + + + + +
 apiVersion: v1
 data:
   app: level-2b
   color: red
+  replicas: "3"
 kind: ConfigMap
 metadata:
   name: level-2b-config
   namespace: default
+
+ +
+ +
+ +root-app (examples/app-of-apps/root-app.yaml) + + +

ApplicationSet: argocd/level-1c-appset

+
+ + + + + + + + + + + + + + + + + + + + + + + +
 apiVersion: argoproj.io/v1alpha1
 kind: ApplicationSet
 metadata:
   name: level-1c-appset
   namespace: argocd
 spec:
   generators:
   - list:
       elements:
       - env: prod
+      - env: staging
   template:
     metadata:
       name: level-1c-{{env}}-app
       namespace: argocd
     spec:
       destination:
         name: in-cluster
         namespace: default
       project: default
       source:
+
+ +
+ +
+ +level-1c-staging-app (examples/app-of-apps/apps/root/level-1c-appset.yaml) + + +

ConfigMap: default/level-1c-staging-config

+
+ + + + + + + + + + +
+apiVersion: v1
+data:
+  app: level-1c-staging
+  color: orange
+kind: ConfigMap
+metadata:
+  name: level-1c-staging-config
+  namespace: default
+
+ +
+ +
+ +level-2c-app (examples/app-of-apps/apps/level-1a/level-2c.yaml) + + +

ConfigMap: default/level-2c-config

+
+ + + + + + + + + + +
+apiVersion: v1
+data:
+  app: level-2c
+  color: orange
+kind: ConfigMap
+metadata:
+  name: level-2c-config
+  namespace: default
+
+ +
+
+ +
_Stats_:
+[Applications: 14], [Full Run: Xs], [Rendering: Xs], [Cluster: Xs], [Argo CD: Xs]
+
+ + diff --git a/integration-test/branch-17/target-2/output.md b/integration-test/branch-17/target-2/output.md new file mode 100644 index 00000000..cea1a724 --- /dev/null +++ b/integration-test/branch-17/target-2/output.md @@ -0,0 +1,178 @@ +## Argo CD Diff Preview + +Summary: +```yaml +Added (2): ++ level-1c-staging-app (+8) ++ level-2c-app (+8) + +Modified (6): +Β± level-1a-app (+13) +Β± level-1b-app (+1|-1) +Β± level-1c-prod-app (+1|-1) +Β± level-2a-app (+2|-1) +Β± level-2b-app (+1) +Β± root-app (+1) +``` + +
+level-1a-app (examples/app-of-apps/apps/root/level-1a.yaml) +
+ +#### Application: argocd/level-2c-app +```diff ++apiVersion: argoproj.io/v1alpha1 ++kind: Application ++metadata: ++ name: level-2c-app ++ namespace: argocd ++spec: ++ destination: ++ name: in-cluster ++ namespace: default ++ project: default ++ source: ++ path: examples/app-of-apps/apps/level-2c ++ repoURL: https://github.com/dag-andersen/argocd-diff-preview +``` +
+ +
+level-1b-app (examples/app-of-apps/apps/root/level-1b.yaml) +
+ +#### ConfigMap: default/level-1b-config +```diff + apiVersion: v1 + data: + app: level-1b +- color: blue ++ color: purple + kind: ConfigMap + metadata: + name: level-1b-config + namespace: default +``` +
+ +
+level-1c-prod-app (examples/app-of-apps/apps/root/level-1c-appset.yaml) +
+ +#### ConfigMap: default/level-1c-prod-config +```diff + apiVersion: v1 + data: + app: level-1c-prod +- color: blue ++ color: purple + kind: ConfigMap + metadata: + name: level-1c-prod-config + namespace: default +``` +
+ +
+level-2a-app (examples/app-of-apps/apps/level-1a/level-2a.yaml) +
+ +#### ConfigMap: default/level-2a-config +```diff + apiVersion: v1 + data: + app: level-2a +- color: green ++ color: yellow ++ environment: production + kind: ConfigMap + metadata: + name: level-2a-config + namespace: default +``` +
+ +
+level-2b-app (examples/app-of-apps/apps/level-1a/level-2b.yaml) +
+ +#### ConfigMap: default/level-2b-config +```diff + apiVersion: v1 + data: + app: level-2b + color: red ++ replicas: "3" + kind: ConfigMap + metadata: + name: level-2b-config + namespace: default +``` +
+ +
+root-app (examples/app-of-apps/root-app.yaml) +
+ +#### ApplicationSet: argocd/level-1c-appset +```diff + apiVersion: argoproj.io/v1alpha1 + kind: ApplicationSet + metadata: + name: level-1c-appset + namespace: argocd + spec: + generators: + - list: + elements: + - env: prod ++ - env: staging + template: + metadata: + name: level-1c-{{env}}-app + namespace: argocd + spec: + destination: + name: in-cluster + namespace: default + project: default + source: +``` +
+ +
+level-1c-staging-app (examples/app-of-apps/apps/root/level-1c-appset.yaml) +
+ +#### ConfigMap: default/level-1c-staging-config +```diff ++apiVersion: v1 ++data: ++ app: level-1c-staging ++ color: orange ++kind: ConfigMap ++metadata: ++ name: level-1c-staging-config ++ namespace: default +``` +
+ +
+level-2c-app (examples/app-of-apps/apps/level-1a/level-2c.yaml) +
+ +#### ConfigMap: default/level-2c-config +```diff ++apiVersion: v1 ++data: ++ app: level-2c ++ color: orange ++kind: ConfigMap ++metadata: ++ name: level-2c-config ++ namespace: default +``` +
+ +_Stats_: +[Applications: 14], [Full Run: Xs], [Rendering: Xs], [Cluster: Xs], [Argo CD: Xs] diff --git a/integration-test/integration_test.go b/integration-test/integration_test.go index c96e54c5..e5a61992 100644 --- a/integration-test/integration_test.go +++ b/integration-test/integration_test.go @@ -277,13 +277,26 @@ var testCases = []TestCase{ // A single root Application renders child Application YAMLs, which are // discovered recursively (BFS) and each rendered independently. { - Name: "branch-17/target", + Name: "branch-17/target-1", TargetBranch: "integration-test/branch-17/target", BaseBranch: "integration-test/branch-17/base", + Suffix: "-1", RenderMethod: "repo-server-api", FileRegex: "examples/app-of-apps/root-app\\.yaml", TraverseAppOfApps: "true", }, + // Same as branch-17/target but watches the entire examples/app-of-apps folder + // instead of only the root-app.yaml file. This exercises the watch pattern + // against all files under the folder (app YAMLs, configmaps, etc.). + { + Name: "branch-17/target-2", + TargetBranch: "integration-test/branch-17/target", + BaseBranch: "integration-test/branch-17/base", + Suffix: "-2", + RenderMethod: "repo-server-api", + FileRegex: "examples/app-of-apps/.*", + TraverseAppOfApps: "true", + }, // This test verifies that disabling cluster roles without using the API fails. // When createClusterRoles: false is set but --render-method=cli is used, // the tool should fail because it can't access cluster resources via CLI. From cf0f15cbebcd9db2dded3d4dbf4d0883bef5170e Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Tue, 17 Mar 2026 23:29:34 +0100 Subject: [PATCH 10/14] Fix | Prevent deadlock when last in-flight app-of-apps item fails --- docs/app-of-apps.md | 2 +- pkg/reposerverextract/appofapps.go | 103 +++++++++++++++-------------- 2 files changed, 55 insertions(+), 50 deletions(-) diff --git a/docs/app-of-apps.md b/docs/app-of-apps.md index 43522f4d..80057b0f 100644 --- a/docs/app-of-apps.md +++ b/docs/app-of-apps.md @@ -90,7 +90,7 @@ The expansion engine tracks every `(app-id, branch)` pair it has already rendere ## Depth limit -The expansion stops after a maximum depth of **5 levels** to guard against runaway trees. If your App of Apps hierarchy is deeper than 5 levels, applications beyond that depth will not be rendered and a warning will be logged. +The expansion stops after a maximum depth of **10 levels** to guard against runaway trees. If your App of Apps hierarchy is deeper than 10 levels, applications beyond that depth will not be rendered and a warning will be logged. --- diff --git a/pkg/reposerverextract/appofapps.go b/pkg/reposerverextract/appofapps.go index 867a6633..8adba519 100644 --- a/pkg/reposerverextract/appofapps.go +++ b/pkg/reposerverextract/appofapps.go @@ -253,6 +253,13 @@ func RenderApplicationsFromBothBranchesWithAppOfApps( // Single collector goroutine: reads results, collects extracted apps, and // enqueues newly discovered children back onto the work channel. + // + // INVARIANT: This is the only goroutine that decrements or reads `pending`. + // The enqueue helper (which increments pending) is only called from this + // goroutine (for child apps) or from the main goroutine during seeding + // (which completes before any results are processed). Because decrement + // and zero-check both happen here, there is no TOCTOU race: when + // pending.Load() == 0 it truly means no items are queued or in-flight. collectorDone := make(chan struct{}) go func() { defer close(collectorDone) @@ -262,62 +269,60 @@ func RenderApplicationsFromBothBranchesWithAppOfApps( firstError = r.err } log.Error().Err(r.err).Msg("❌ Failed to render application via repo server:") - pending.Add(-1) - continue - } - - switch r.extracted.Branch { - case git.Base: - extractedBaseApps = append(extractedBaseApps, r.extracted) - case git.Target: - extractedTargetApps = append(extractedTargetApps, r.extracted) - default: - if firstError == nil { - firstError = fmt.Errorf("unknown branch type: '%s'", r.extracted.Branch) + } else { + switch r.extracted.Branch { + case git.Base: + extractedBaseApps = append(extractedBaseApps, r.extracted) + case git.Target: + extractedTargetApps = append(extractedTargetApps, r.extracted) + default: + if firstError == nil { + firstError = fmt.Errorf("unknown branch type: '%s'", r.extracted.Branch) + } } - } - // Enqueue children that haven't been seen yet and pass the selection filter. - // Child apps are filtered by Selector, FilesChanged (via watch-pattern annotations), - // IgnoreInvalidWatchPattern, and WatchIfNoWatchPatternFound β€” exactly as top-level apps are. - // FilesChanged works correctly here: the PR diff is the same regardless of whether an - // app was discovered from a file or from a parent's rendered output; the watch pattern - // on the child app is what determines whether it is affected. - // - // FileRegex is intentionally excluded because it filters by the physical filename of - // the Application YAML file. Child apps don't come from a file; their FileName is - // "parent: " (a breadcrumb), which would give false matches against any regex. - if r.depth < maxAppOfAppsDepth { - childSelectionOptions := argoapplication.ApplicationSelectionOptions{ - Selector: appSelectionOptions.Selector, - FilesChanged: appSelectionOptions.FilesChanged, - IgnoreInvalidWatchPattern: appSelectionOptions.IgnoreInvalidWatchPattern, - WatchIfNoWatchPatternFound: appSelectionOptions.WatchIfNoWatchPatternFound, - // FileRegex intentionally omitted: child apps have no real file path - } - selection := argoapplication.ApplicationSelection(r.childApps, childSelectionOptions) - for _, skipped := range selection.SkippedApps { - log.Debug().Str("App", skipped.GetLongName()).Msg("Skipping child Application excluded by ApplicationSelectionOptions") - } - visitedMu.Lock() - for _, child := range selection.SelectedApps { - key := visitedKey(child.Yaml, child.Branch) - if visited[key] { - log.Debug().Str("App", child.GetLongName()).Msg("Skipping already-visited child Application") - continue + // Enqueue children that haven't been seen yet and pass the selection filter. + // Child apps are filtered by Selector, FilesChanged (via watch-pattern annotations), + // IgnoreInvalidWatchPattern, and WatchIfNoWatchPatternFound β€” exactly as top-level apps are. + // FilesChanged works correctly here: the PR diff is the same regardless of whether an + // app was discovered from a file or from a parent's rendered output; the watch pattern + // on the child app is what determines whether it is affected. + // + // FileRegex is intentionally excluded because it filters by the physical filename of + // the Application YAML file. Child apps don't come from a file; their FileName is + // "parent: " (a breadcrumb), which would give false matches against any regex. + if r.depth < maxAppOfAppsDepth { + childSelectionOptions := argoapplication.ApplicationSelectionOptions{ + Selector: appSelectionOptions.Selector, + FilesChanged: appSelectionOptions.FilesChanged, + IgnoreInvalidWatchPattern: appSelectionOptions.IgnoreInvalidWatchPattern, + WatchIfNoWatchPatternFound: appSelectionOptions.WatchIfNoWatchPatternFound, + // FileRegex intentionally omitted: child apps have no real file path + } + selection := argoapplication.ApplicationSelection(r.childApps, childSelectionOptions) + for _, skipped := range selection.SkippedApps { + log.Debug().Str("App", skipped.GetLongName()).Msg("Skipping child Application excluded by ApplicationSelectionOptions") + } + visitedMu.Lock() + for _, child := range selection.SelectedApps { + key := visitedKey(child.Yaml, child.Branch) + if visited[key] { + log.Debug().Str("App", child.GetLongName()).Msg("Skipping already-visited child Application") + continue + } + visited[key] = true + enqueue(child, r.depth+1) } - visited[key] = true - enqueue(child, r.depth+1) + visitedMu.Unlock() + } else if len(r.childApps) > 0 { + log.Warn().Msgf("⚠️ App-of-apps depth limit (%d) reached; not enqueuing %d child(ren) of %s", + maxAppOfAppsDepth, len(r.childApps), r.extracted.Name) } - visitedMu.Unlock() - } else if len(r.childApps) > 0 { - log.Warn().Msgf("⚠️ App-of-apps depth limit (%d) reached; not enqueuing %d child(ren) of %s", - maxAppOfAppsDepth, len(r.childApps), r.extracted.Name) } - pending.Add(-1) - + // Decrement pending for both success and error paths. // When all pending work is done, close the work channel so workers exit. + pending.Add(-1) if pending.Load() == 0 { close(work) } From c4512fea7f270b1fd86b7267454aa232a51b1654 Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Tue, 17 Mar 2026 23:29:34 +0100 Subject: [PATCH 11/14] Tests | Add direct unit tests for pairByContent greedy matching --- pkg/matching/matching_test.go | 212 ++++++++++++++++++++++++++++++++++ 1 file changed, 212 insertions(+) diff --git a/pkg/matching/matching_test.go b/pkg/matching/matching_test.go index 733410fd..8b0a5717 100644 --- a/pkg/matching/matching_test.go +++ b/pkg/matching/matching_test.go @@ -2139,3 +2139,215 @@ func TestDiffAction_String(t *testing.T) { }) } } + +// --------------------------------------------------------------------------- +// pairByContent unit tests +// --------------------------------------------------------------------------- + +func TestPairByContent_EmptyBaseIndices(t *testing.T) { + app := makeApp("a", "app", nil) + result := pairByContent([]extract.ExtractedApp{app}, []extract.ExtractedApp{app}, nil, []int{0}) + if len(result) != 0 { + t.Errorf("expected no pairs for empty base indices, got %d", len(result)) + } +} + +func TestPairByContent_EmptyTargetIndices(t *testing.T) { + app := makeApp("a", "app", nil) + result := pairByContent([]extract.ExtractedApp{app}, []extract.ExtractedApp{app}, []int{0}, nil) + if len(result) != 0 { + t.Errorf("expected no pairs for empty target indices, got %d", len(result)) + } +} + +func TestPairByContent_SingleCandidate(t *testing.T) { + // Fast path: one base, one target β†’ paired without similarity computation. + base := makeApp("a", "app", []unstructured.Unstructured{ + makeResource("apps/v1", "Deployment", "default", "d1", nil), + }) + target := makeApp("a", "app", []unstructured.Unstructured{ + makeResource("apps/v1", "Deployment", "default", "d1", nil), + }) + result := pairByContent([]extract.ExtractedApp{base}, []extract.ExtractedApp{target}, []int{0}, []int{0}) + if len(result) != 1 { + t.Fatalf("expected 1 pair, got %d", len(result)) + } + if ti, ok := result[0]; !ok || ti != 0 { + t.Errorf("expected base[0]β†’target[0], got %v", result) + } +} + +func TestPairByContent_GreedyMatchByContent(t *testing.T) { + // Two base apps and two target apps with the same identity key but + // different content. The greedy matcher should pair them by similarity + // (base[0]↔target[0] identical, base[1]↔target[1] identical) rather than + // mixing them up. + deployA := makeResource("apps/v1", "Deployment", "default", "frontend", map[string]any{ + "spec": map[string]any{"replicas": int64(3), "image": "nginx"}, + }) + deployB := makeResource("apps/v1", "Deployment", "default", "backend", map[string]any{ + "spec": map[string]any{"replicas": int64(1), "image": "redis"}, + }) + + baseApps := []extract.ExtractedApp{ + makeApp("shared-id", "app", []unstructured.Unstructured{deployA}), + makeApp("shared-id", "app", []unstructured.Unstructured{deployB}), + } + targetApps := []extract.ExtractedApp{ + makeApp("shared-id", "app", []unstructured.Unstructured{deployA}), + makeApp("shared-id", "app", []unstructured.Unstructured{deployB}), + } + + result := pairByContent(baseApps, targetApps, []int{0, 1}, []int{0, 1}) + + if len(result) != 2 { + t.Fatalf("expected 2 pairs, got %d", len(result)) + } + // base[0] has deployA content β†’ should match target[0] (also deployA) + if result[0] != 0 { + t.Errorf("expected base[0]β†’target[0], got base[0]β†’target[%d]", result[0]) + } + // base[1] has deployB content β†’ should match target[1] (also deployB) + if result[1] != 1 { + t.Errorf("expected base[1]β†’target[1], got base[1]β†’target[%d]", result[1]) + } +} + +func TestPairByContent_GreedyMatchSwappedOrder(t *testing.T) { + // Same as above but target apps are in reversed order. + // Greedy matcher should still pair by content, not by position. + deployA := makeResource("apps/v1", "Deployment", "default", "frontend", map[string]any{ + "spec": map[string]any{"replicas": int64(3), "image": "nginx"}, + }) + deployB := makeResource("apps/v1", "Deployment", "default", "backend", map[string]any{ + "spec": map[string]any{"replicas": int64(1), "image": "redis"}, + }) + + baseApps := []extract.ExtractedApp{ + makeApp("shared-id", "app", []unstructured.Unstructured{deployA}), + makeApp("shared-id", "app", []unstructured.Unstructured{deployB}), + } + targetApps := []extract.ExtractedApp{ + makeApp("shared-id", "app", []unstructured.Unstructured{deployB}), // swapped + makeApp("shared-id", "app", []unstructured.Unstructured{deployA}), // swapped + } + + result := pairByContent(baseApps, targetApps, []int{0, 1}, []int{0, 1}) + + if len(result) != 2 { + t.Fatalf("expected 2 pairs, got %d", len(result)) + } + // base[0] (deployA) should match target[1] (deployA) + if result[0] != 1 { + t.Errorf("expected base[0]β†’target[1], got base[0]β†’target[%d]", result[0]) + } + // base[1] (deployB) should match target[0] (deployB) + if result[1] != 0 { + t.Errorf("expected base[1]β†’target[0], got base[1]β†’target[%d]", result[1]) + } +} + +func TestPairByContent_UnevenCandidates_MoreBase(t *testing.T) { + // 3 base candidates, 2 target candidates. The best 2 base apps should be + // matched; the third has no partner. + deployA := makeResource("apps/v1", "Deployment", "default", "frontend", map[string]any{ + "spec": map[string]any{"replicas": int64(3), "image": "nginx"}, + }) + deployB := makeResource("apps/v1", "Deployment", "default", "backend", map[string]any{ + "spec": map[string]any{"replicas": int64(1), "image": "redis"}, + }) + deployC := makeResource("apps/v1", "Deployment", "default", "worker", map[string]any{ + "spec": map[string]any{"replicas": int64(5), "image": "python"}, + }) + + baseApps := []extract.ExtractedApp{ + makeApp("id", "app", []unstructured.Unstructured{deployA}), + makeApp("id", "app", []unstructured.Unstructured{deployB}), + makeApp("id", "app", []unstructured.Unstructured{deployC}), + } + targetApps := []extract.ExtractedApp{ + makeApp("id", "app", []unstructured.Unstructured{deployA}), + makeApp("id", "app", []unstructured.Unstructured{deployB}), + } + + result := pairByContent(baseApps, targetApps, []int{0, 1, 2}, []int{0, 1}) + + if len(result) != 2 { + t.Fatalf("expected 2 pairs, got %d", len(result)) + } + // base[0] (deployA) β†’ target[0] (deployA), base[1] (deployB) β†’ target[1] (deployB) + if result[0] != 0 { + t.Errorf("expected base[0]β†’target[0], got base[0]β†’target[%d]", result[0]) + } + if result[1] != 1 { + t.Errorf("expected base[1]β†’target[1], got base[1]β†’target[%d]", result[1]) + } + // base[2] should NOT be paired + if _, ok := result[2]; ok { + t.Errorf("expected base[2] to be unpaired, but it was matched to target[%d]", result[2]) + } +} + +func TestPairByContent_UnevenCandidates_MoreTarget(t *testing.T) { + // 2 base candidates, 3 target candidates. + deployA := makeResource("apps/v1", "Deployment", "default", "frontend", map[string]any{ + "spec": map[string]any{"replicas": int64(3), "image": "nginx"}, + }) + deployB := makeResource("apps/v1", "Deployment", "default", "backend", map[string]any{ + "spec": map[string]any{"replicas": int64(1), "image": "redis"}, + }) + deployC := makeResource("apps/v1", "Deployment", "default", "worker", map[string]any{ + "spec": map[string]any{"replicas": int64(5), "image": "python"}, + }) + + baseApps := []extract.ExtractedApp{ + makeApp("id", "app", []unstructured.Unstructured{deployA}), + makeApp("id", "app", []unstructured.Unstructured{deployB}), + } + targetApps := []extract.ExtractedApp{ + makeApp("id", "app", []unstructured.Unstructured{deployC}), + makeApp("id", "app", []unstructured.Unstructured{deployA}), + makeApp("id", "app", []unstructured.Unstructured{deployB}), + } + + result := pairByContent(baseApps, targetApps, []int{0, 1}, []int{0, 1, 2}) + + if len(result) != 2 { + t.Fatalf("expected 2 pairs, got %d", len(result)) + } + // base[0] (deployA) β†’ target[1] (deployA) + if result[0] != 1 { + t.Errorf("expected base[0]β†’target[1], got base[0]β†’target[%d]", result[0]) + } + // base[1] (deployB) β†’ target[2] (deployB) + if result[1] != 2 { + t.Errorf("expected base[1]β†’target[2], got base[1]β†’target[%d]", result[1]) + } +} + +func TestPairByContent_DeterministicTieBreaking(t *testing.T) { + // When all candidates have the same similarity score (all empty manifests), + // the tie-breaking should be deterministic: lowest base index first, then + // lowest target index. + baseApps := []extract.ExtractedApp{ + makeApp("id", "app", nil), + makeApp("id", "app", nil), + } + targetApps := []extract.ExtractedApp{ + makeApp("id", "app", nil), + makeApp("id", "app", nil), + } + + // Run multiple times to verify determinism. + for i := 0; i < 10; i++ { + result := pairByContent(baseApps, targetApps, []int{0, 1}, []int{0, 1}) + if len(result) != 2 { + t.Fatalf("iteration %d: expected 2 pairs, got %d", i, len(result)) + } + // With tie-breaking: base[0]β†’target[0] picked first (lowest bi, lowest ti), + // then base[1]β†’target[1] (next available). + if result[0] != 0 || result[1] != 1 { + t.Errorf("iteration %d: expected deterministic {0β†’0, 1β†’1}, got %v", i, result) + } + } +} From 3b49c8ad8c8ab1fbb0f92ce83b779dfbea81106b Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Tue, 17 Mar 2026 23:47:50 +0100 Subject: [PATCH 12/14] Docs | Warn about application selection filters hiding app-of-apps children --- docs/app-of-apps.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/docs/app-of-apps.md b/docs/app-of-apps.md index 80057b0f..06d5c4c7 100644 --- a/docs/app-of-apps.md +++ b/docs/app-of-apps.md @@ -74,9 +74,31 @@ Child applications discovered through the App of Apps expansion are subject to t | `--watch-if-no-watch-pattern-found` | βœ… Yes | | File path regex (`--file-regex`) | ❌ No - child apps have no physical file path | +!!! warning "Filters apply at every level of the tree" + A child application is only discovered if its **parent is rendered**. If a parent application is excluded by a selector, watch-pattern, or any other filter, the tool never renders it - and therefore never sees its children. This means changes further down the tree can go undetected. + + For example, if you use `--selector "team=frontend"` and your root app does not have the label `team: frontend`, none of its children will be processed - even if a child app *does* carry that label. + + When using application selection filters together with `--traverse-app-of-apps`, make sure your **root and intermediate applications pass the filters**, not just the leaf applications you care about. + !!! tip "Watch patterns on child apps" You can add `argocd-diff-preview/watch-pattern` or `argocd.argoproj.io/manifest-generate-paths` annotations directly to your child Application manifests. These annotations are evaluated against the PR's changed files, just like they are for top-level applications. +### Recommended: use `--file-regex` to select only root applications + +If you follow the App of Apps pattern, a practical approach is to use `--file-regex` to select only the root application files and let the tree traversal take care of the rest. This way the root apps are always rendered, and all children are discovered automatically. + +For example, if your root application is defined in `apps/root.yaml`: + +```bash +argocd-diff-preview \ + --render-method=repo-server-api \ + --traverse-app-of-apps \ + --file-regex="^apps/root\.yaml$" +``` + +This avoids the problem described above where filters accidentally exclude a parent and silently hide changes in its children. + --- ## Cycle and diamond protection From 48ba35bb36c81ac966a1a53dd894b69deb449cf1 Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Wed, 18 Mar 2026 20:41:29 +0100 Subject: [PATCH 13/14] Fix linting error --- pkg/matching/matching_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/matching/matching_test.go b/pkg/matching/matching_test.go index 8b0a5717..a8e7676f 100644 --- a/pkg/matching/matching_test.go +++ b/pkg/matching/matching_test.go @@ -2339,7 +2339,7 @@ func TestPairByContent_DeterministicTieBreaking(t *testing.T) { } // Run multiple times to verify determinism. - for i := 0; i < 10; i++ { + for i := range 10 { result := pairByContent(baseApps, targetApps, []int{0, 1}, []int{0, 1}) if len(result) != 2 { t.Fatalf("iteration %d: expected 2 pairs, got %d", i, len(result)) From b97b0ccab9f113df3781e54ac379dfcd9b3d50df Mon Sep 17 00:00:00 2001 From: Dag Andersen Date: Wed, 18 Mar 2026 21:07:50 +0100 Subject: [PATCH 14/14] cleanup --- docs/app-of-apps.md | 10 +++------- pkg/reposerverextract/appofapps.go | 18 +++++++++--------- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/docs/app-of-apps.md b/docs/app-of-apps.md index 06d5c4c7..e393cdf0 100644 --- a/docs/app-of-apps.md +++ b/docs/app-of-apps.md @@ -16,15 +16,11 @@ With the `--traverse-app-of-apps` flag, `argocd-diff-preview` can discover and r !!! tip "Prefer simpler alternatives when possible" The `--traverse-app-of-apps` feature is **slower** and **more limited** than the standard rendering flow. Before enabling it, consider whether one of the alternatives below covers your use case. -**Alternative 1 - Pre-render your Application manifests** +**Pre-render your Application manifests** -If your child Application manifests are stored in a Git repository (which is the common case), `argocd-diff-preview` will find and render them automatically without any special flags. The tool scans your repository for all `kind: Application` and `kind: ApplicationSet` files and renders them directly. +If your applications do not exist as plain manifests inside the repo, but are instead generated from Helm or Kustomize, you can pre-render them in your CI pipeline and place the output in the branch folder. `argocd-diff-preview` will then pick them up as regular files. See [Helm/Kustomize generated Argo CD applications](./generated-applications.md) for details and examples. -Only use `--traverse-app-of-apps` when the child Application manifests are *not* committed to the repository and exist only as rendered output from a parent application. - -**Alternative 2 - Helm or Kustomize generated Applications** - -If your parent application uses Helm or Kustomize to generate child Application manifests, you can pre-render them in your CI pipeline and place the output in the branch folder. `argocd-diff-preview` will then pick them up as regular files. See [Helm/Kustomize generated Argo CD applications](generated-applications.md) for details and examples. +Only use `--traverse-app-of-apps` when the child Applications are *not* committed as plain manifests to the repository AND can *not* be pre-rendered easily. --- diff --git a/pkg/reposerverextract/appofapps.go b/pkg/reposerverextract/appofapps.go index 8adba519..d73a9ebe 100644 --- a/pkg/reposerverextract/appofapps.go +++ b/pkg/reposerverextract/appofapps.go @@ -433,7 +433,7 @@ func renderAppWithChildDiscovery( case "Application": name := m.GetName() if name == "" { - log.Warn().Str("parentApp", app.GetLongName()).Msg("⚠️ Discovered child Application has no name; skipping child rendering") + log.Warn().Str("parentApp", app.Name).Msg("⚠️ Discovered child Application has no name; skipping child rendering") continue } fileName := fmt.Sprintf("parent: %s", app.Name) @@ -443,21 +443,21 @@ func renderAppWithChildDiscovery( child, err := argoapplication.PatchApplication(argocdNamespace, *resource, branchByType[app.Branch], prRepo, nil) if err != nil { log.Warn().Err(err). - Str("parentApp", app.GetLongName()). + Str("parentApp", app.Name). Str("childName", name). Msg("⚠️ Could not patch child Application; skipping child rendering") continue } childApps = append(childApps, *child) log.Debug(). - Str("parentApp", app.GetLongName()). + Str("parentApp", app.Name). Str("childApp", child.GetLongName()). Msg("Discovered child Application via app-of-apps pattern") case "ApplicationSet": appSetName := m.GetName() log.Info(). - Str("parentApp", app.GetLongName()). + Str("parentApp", app.Name). Str("appSet", appSetName). Msgf("πŸ” Discovered child ApplicationSet in rendered manifests; expanding to Applications") @@ -472,7 +472,7 @@ func renderAppWithChildDiscovery( patchedAppSet, err := argoapplication.PatchApplication(argocdNamespace, *appSetResource, branch, prRepo, nil) if err != nil { log.Warn().Err(err). - Str("parentApp", app.GetLongName()). + Str("parentApp", app.Name). Str("appSet", appSetName). Msg("⚠️ Failed to patch child ApplicationSet before expansion; skipping expansion") continue @@ -481,7 +481,7 @@ func renderAppWithChildDiscovery( generatedManifests, err := argocd.AppsetGenerateWithRetry(patchedAppSet.Yaml, appSetTempFolder, 5) if err != nil { log.Warn().Err(err). - Str("parentApp", app.GetLongName()). + Str("parentApp", app.Name). Str("appSet", appSetName). Msg("⚠️ Could not expand child ApplicationSet; skipping expansion") continue @@ -505,14 +505,14 @@ func renderAppWithChildDiscovery( child, err := argoapplication.PatchApplication(argocdNamespace, *resource, branch, prRepo, nil) if err != nil { log.Warn().Err(err). - Str("parentApp", app.GetLongName()). + Str("parentApp", app.Name). Str("appSet", appSetName). Msg("⚠️ Could not patch ApplicationSet-generated Application; skipping") continue } childApps = append(childApps, *child) log.Debug(). - Str("parentApp", app.GetLongName()). + Str("parentApp", app.Name). Str("appSet", appSetName). Str("childApp", child.GetLongName()). Msg("Discovered child Application via ApplicationSet expansion") @@ -522,7 +522,7 @@ func renderAppWithChildDiscovery( if len(childApps) > 0 { log.Info(). - Str("parentApp", app.GetLongName()). + Str("parentApp", app.Name). Msgf("πŸ” Discovered %d child Application(s) in rendered manifests", len(childApps)) }