From b29011120bb3f724de83274316ee652c08f8d20d Mon Sep 17 00:00:00 2001 From: Timothy O'Brien Date: Tue, 16 Jun 2026 15:44:26 -0700 Subject: [PATCH 1/2] Fix | App-of-apps children honor --redirect-target-revisions (#446) During --traverse-app-of-apps expansion, child Applications/ApplicationSets discovered in rendered manifests were patched with redirectRevisions=nil. An empty list means "redirect everything", so the --redirect-target-revisions filter was silently ignored for children: a child source pinned to a ref not in the list (e.g. a feature branch holding value files absent from the base branch) was still redirected to the PR/base branch and failed to render, failing the whole run. Thread the configured redirectRevisions from RenderApplicationsFromBothBranchesWithAppOfApps through renderAppWithChildDiscovery to every child PatchApplication call so children go through the same RedirectSources/RedirectGenerators filter as seed Applications. Extract the single-Application child patching into a buildChildApplication helper and cover the threading with unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/main.go | 1 + docs/app-of-apps.md | 2 + pkg/reposerverextract/appofapps.go | 38 +++++++++-- pkg/reposerverextract/appofapps_test.go | 88 +++++++++++++++++++++++++ 4 files changed, 122 insertions(+), 7 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 9c83e13f..8f43addd 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -333,6 +333,7 @@ func run(cfg *Config) error { cfg.RepoSelector, appSelectionOptions, tempFolder, + redirectRevisions, ) } else { baseManifests, targetManifests, extractDuration, err = reposerverextract.RenderApplicationsFromBothBranches( diff --git a/docs/app-of-apps.md b/docs/app-of-apps.md index 5c13a673..5c2a7450 100644 --- a/docs/app-of-apps.md +++ b/docs/app-of-apps.md @@ -43,6 +43,8 @@ When `--traverse-app-of-apps` is enabled, the tool performs a breadth-first expa 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. +Discovered children are patched exactly like top-level applications. In particular, source/generator redirection honors [`--redirect-target-revisions`](options.md): a child source whose `targetRevision` is **not** in the configured list is left untouched and fetched from its real ref, rather than being redirected to the base/target branch. + --- ## Requirements diff --git a/pkg/reposerverextract/appofapps.go b/pkg/reposerverextract/appofapps.go index 3b64f656..8e83e511 100644 --- a/pkg/reposerverextract/appofapps.go +++ b/pkg/reposerverextract/appofapps.go @@ -137,6 +137,7 @@ func RenderApplicationsFromBothBranchesWithAppOfApps( repoSelector repository.Selector, appSelectionOptions argoapplication.ApplicationSelectionOptions, tempFolder string, + redirectRevisions []string, ) ([]extract.ExtractedApp, []extract.ExtractedApp, time.Duration, error) { startTime := time.Now() @@ -352,7 +353,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, branchByType, namespacedScopedResources, creds, &repoSelector, argocd.Namespace, tempFolder, item.depth, kubeVersion, apiVersions) + manifests, childApps, err := renderAppWithChildDiscovery(ctx, repoClient, argocd, item.app, branchFolderByType, branchByType, namespacedScopedResources, creds, &repoSelector, argocd.Namespace, tempFolder, item.depth, kubeVersion, apiVersions, redirectRevisions) if err != nil { results <- renderResult{err: fmt.Errorf("failed to render app %s: %w", item.app.GetLongName(), err)} return @@ -388,6 +389,29 @@ func RenderApplicationsFromBothBranchesWithAppOfApps( return extractedBaseApps, extractedTargetApps, time.Since(startTime), nil } +// buildChildApplication deep-copies a discovered Application manifest, wraps it +// in an ArgoResource, and patches it. The deep copy ensures PatchApplication +// (which mutates the Yaml pointer in-place) does not modify the manifest that +// remains in the parent's diff output. +// +// redirectRevisions is threaded through to PatchApplication so that child +// Applications honor --redirect-target-revisions exactly like seed Applications: +// a source whose targetRevision is not in the list is left untouched rather than +// being unconditionally redirected to the base/target branch. +func buildChildApplication( + argocdNamespace string, + m unstructured.Unstructured, + fileName string, + branchType git.BranchType, + branch *git.Branch, + repoSelector repository.Selector, + redirectRevisions []string, +) (*argoapplication.ArgoResource, error) { + name := m.GetName() + resource := argoapplication.NewArgoResource(m.DeepCopy(), argoapplication.Application, name, name, fileName, branchType) + return argoapplication.PatchApplication(argocdNamespace, *resource, branch, repoSelector, redirectRevisions) +} + // renderAppWithChildDiscovery renders a single application and returns: // // 1. All rendered manifests to include in the diff (returned as the first @@ -419,6 +443,7 @@ func renderAppWithChildDiscovery( depth int, kubeVersion string, apiVersions []string, + redirectRevisions []string, ) ([]unstructured.Unstructured, []argoapplication.ArgoResource, error) { allManifests, err := renderApp(ctx, repoClient, app, branchFolderByType, namespacedScopedResources, creds, repoSelector, kubeVersion, apiVersions) if err != nil { @@ -446,9 +471,9 @@ func renderAppWithChildDiscovery( } fileName := fmt.Sprintf("parent: %s", app.Name) // 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], *repoSelector, nil) + // allManifests (the diff) untouched. redirectRevisions is threaded + // through so children honor --redirect-target-revisions like seed apps. + child, err := buildChildApplication(argocdNamespace, m, fileName, app.Branch, branchByType[app.Branch], *repoSelector, redirectRevisions) if err != nil { log.Warn().Err(err). Str("parentApp", app.Name). @@ -477,7 +502,7 @@ func renderAppWithChildDiscovery( // 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, *repoSelector, nil) + patchedAppSet, err := argoapplication.PatchApplication(argocdNamespace, *appSetResource, branch, *repoSelector, redirectRevisions) if err != nil { log.Warn().Err(err). Str("parentApp", app.Name). @@ -509,8 +534,7 @@ func renderAppWithChildDiscovery( 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, *repoSelector, nil) + child, err := buildChildApplication(argocdNamespace, genDoc, breadcrumb, app.Branch, branch, *repoSelector, redirectRevisions) if err != nil { log.Warn().Err(err). Str("parentApp", app.Name). diff --git a/pkg/reposerverextract/appofapps_test.go b/pkg/reposerverextract/appofapps_test.go index f5c841e2..f77d6f49 100644 --- a/pkg/reposerverextract/appofapps_test.go +++ b/pkg/reposerverextract/appofapps_test.go @@ -17,7 +17,9 @@ import ( "testing" "github.com/dag-andersen/argocd-diff-preview/pkg/git" + "github.com/dag-andersen/argocd-diff-preview/pkg/repository" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) @@ -162,3 +164,89 @@ func TestSpecHashOf_NilYaml(t *testing.T) { // Should not panic. assert.NotPanics(t, func() { specHashOf(nil) }) } + +// ── buildChildApplication: --redirect-target-revisions threading ──────────────── +// +// Regression tests for https://github.com/dag-andersen/argocd-diff-preview/issues/446 +// +// Child Applications discovered during app-of-apps traversal must honor +// --redirect-target-revisions exactly like seed Applications: a source whose +// targetRevision is NOT in the configured list must be left untouched and +// fetched from its real ref, instead of being unconditionally redirected to the +// base/target branch. + +// childAppManifest builds a minimal discovered child Application manifest whose +// single source points at the matched repo and is pinned to targetRevision. +func childAppManifest(repoURL, targetRevision string) unstructured.Unstructured { + return unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "argoproj.io/v1alpha1", + "kind": "Application", + "metadata": map[string]any{"name": "child-app"}, + "spec": map[string]any{ + "source": map[string]any{ + "repoURL": repoURL, + "path": "apps/child", + "targetRevision": targetRevision, + }, + }, + }} +} + +func childSourceTargetRevision(t *testing.T, child unstructured.Unstructured) string { + t.Helper() + rev, found, err := unstructured.NestedString(child.Object, "spec", "source", "targetRevision") + require.NoError(t, err) + require.True(t, found, "child source must have a targetRevision") + return rev +} + +func TestBuildChildApplication_LeavesUnlistedRevisionUntouched(t *testing.T) { + const repoURL = "https://github.com/example/repo" + selector, err := repository.NewSelector(repoURL, "") + require.NoError(t, err) + targetBranch := git.NewBranch("target", git.Target) + + // targetRevision "feature-branch" is NOT in the redirect list, so the child + // must keep its pinned ref. + manifest := childAppManifest(repoURL, "feature-branch") + child, err := buildChildApplication("argocd", manifest, "parent: root", git.Target, targetBranch, *selector, []string{"main", "HEAD"}) + require.NoError(t, err) + require.NotNil(t, child) + + assert.Equal(t, "feature-branch", childSourceTargetRevision(t, *child.Yaml), + "child source pinned to a revision outside --redirect-target-revisions must not be redirected") +} + +func TestBuildChildApplication_RedirectsListedRevision(t *testing.T) { + const repoURL = "https://github.com/example/repo" + selector, err := repository.NewSelector(repoURL, "") + require.NoError(t, err) + targetBranch := git.NewBranch("target", git.Target) + + // targetRevision "main" IS in the redirect list, so the child must be + // redirected to the target branch. + manifest := childAppManifest(repoURL, "main") + child, err := buildChildApplication("argocd", manifest, "parent: root", git.Target, targetBranch, *selector, []string{"main", "HEAD"}) + require.NoError(t, err) + require.NotNil(t, child) + + assert.Equal(t, "target", childSourceTargetRevision(t, *child.Yaml), + "child source pinned to a revision listed in --redirect-target-revisions must be redirected") +} + +func TestBuildChildApplication_EmptyListRedirectsAll(t *testing.T) { + const repoURL = "https://github.com/example/repo" + selector, err := repository.NewSelector(repoURL, "") + require.NoError(t, err) + targetBranch := git.NewBranch("target", git.Target) + + // An empty redirect list preserves the original "redirect everything" + // default behavior. + manifest := childAppManifest(repoURL, "feature-branch") + child, err := buildChildApplication("argocd", manifest, "parent: root", git.Target, targetBranch, *selector, nil) + require.NoError(t, err) + require.NotNil(t, child) + + assert.Equal(t, "target", childSourceTargetRevision(t, *child.Yaml), + "with no --redirect-target-revisions configured, every matching source is redirected") +} From 66f84b396f389ac63b642ebbd5d8094a5bd67695 Mon Sep 17 00:00:00 2001 From: Timothy O'Brien Date: Tue, 16 Jun 2026 22:16:10 -0700 Subject: [PATCH 2/2] Docs | Align app-of-apps redirect comments with codebase style Drop a redundant inline sentence (the rationale lives on the buildChildApplication doc) and condense the new test section header to match the existing single-line section headers. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/reposerverextract/appofapps.go | 3 +-- pkg/reposerverextract/appofapps_test.go | 13 +++++-------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/pkg/reposerverextract/appofapps.go b/pkg/reposerverextract/appofapps.go index 8e83e511..abc93c32 100644 --- a/pkg/reposerverextract/appofapps.go +++ b/pkg/reposerverextract/appofapps.go @@ -471,8 +471,7 @@ func renderAppWithChildDiscovery( } fileName := fmt.Sprintf("parent: %s", app.Name) // Deep copy so PatchApplication mutates the copy, leaving m in - // allManifests (the diff) untouched. redirectRevisions is threaded - // through so children honor --redirect-target-revisions like seed apps. + // allManifests (the diff) untouched. child, err := buildChildApplication(argocdNamespace, m, fileName, app.Branch, branchByType[app.Branch], *repoSelector, redirectRevisions) if err != nil { log.Warn().Err(err). diff --git a/pkg/reposerverextract/appofapps_test.go b/pkg/reposerverextract/appofapps_test.go index f77d6f49..de52f9ab 100644 --- a/pkg/reposerverextract/appofapps_test.go +++ b/pkg/reposerverextract/appofapps_test.go @@ -165,15 +165,12 @@ func TestSpecHashOf_NilYaml(t *testing.T) { assert.NotPanics(t, func() { specHashOf(nil) }) } -// ── buildChildApplication: --redirect-target-revisions threading ──────────────── +// ── buildChildApplication ─────────────────────────────────────────────────────── // -// Regression tests for https://github.com/dag-andersen/argocd-diff-preview/issues/446 -// -// Child Applications discovered during app-of-apps traversal must honor -// --redirect-target-revisions exactly like seed Applications: a source whose -// targetRevision is NOT in the configured list must be left untouched and -// fetched from its real ref, instead of being unconditionally redirected to the -// base/target branch. +// Regression tests for https://github.com/dag-andersen/argocd-diff-preview/issues/446: +// discovered child Applications must honor --redirect-target-revisions exactly like +// seed Applications (a source whose targetRevision is not in the list is left +// untouched rather than redirected to the base/target branch). // childAppManifest builds a minimal discovered child Application manifest whose // single source points at the matched repo and is pinned to targetRevision.