fix(appdir): include multi-source apps when any source matches the branch - #548
Open
nidiodolfini wants to merge 1 commit into
Open
nidiodolfini wants to merge 1 commit into
nidiodolfini wants to merge 1 commit into
Conversation
…anch getTargetRevision reads only Sources[0]; a chart-first multi-source app (OCI chart pinned in source[0], values in a git ref source) was excluded because the chart's targetRevision (a version) never equals the PR target branch. shouldInclude now iterates all sources.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes app inclusion filtering for ArgoCD multi-source Applications in pkg/appdir, ensuring apps are not incorrectly excluded from PR checks when the first source is a versioned Helm chart but a later git/values source targets the PR branch.
Changes:
- Update
shouldIncludeto iterate over all application sources and include the app if any source matches the target branch (or is empty /HEADwith main/master handling). - Add clarifying comments documenting the multi-source “chart-first + git values ref” scenario and the prior exclusion behavior.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+124
to
+125
| for _, source := range getSources(app) { | ||
| targetRevision := source.TargetRevision |
Comment on lines
118
to
+124
| func shouldInclude(app v1alpha1.Application, targetBranch string) bool { | ||
| targetRevision := getTargetRevision(app) | ||
| if targetRevision == "" { | ||
| return true | ||
| } | ||
|
|
||
| if targetRevision == targetBranch { | ||
| return true | ||
| } | ||
|
|
||
| if targetRevision == "HEAD" { | ||
| if targetBranch == "main" { | ||
| // Consider every source: a multi-source Application whose values live in a | ||
| // git source (targetRevision=<branch>) must be included even when the first | ||
| // source is a versioned Helm chart (targetRevision=<chart version>). | ||
| // getTargetRevision(app) only reads app.Spec.GetSource() == Sources[0], so | ||
| // "chart-first" multi-source apps were incorrectly excluded from PR checks. | ||
| for _, source := range getSources(app) { |
Collaborator
|
@nidiodolfini, please write a unit test covering both single-source and multi-source. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
shouldInclude(pkg/appdir/app_directory.go) drops multi-source Applications whose first source is a versioned Helm chart.getTargetRevision(app)returnsapp.Spec.GetSource().TargetRevision, which for a multi-source app isSources[0]. A common ArgoCD multi-source layout is:source[0]: an OCI/Helm chart withtargetRevision: <chart version>(e.g.0.5.13) andhelm.valueFiles: ["$values/…"]source[1]: a git ref source (ref: values,targetRevision: <branch>) providing the valuesWhen a PR changes the values file in the git repo, the affected app is correctly matched by the change list, but then excluded:
shouldIncludecompares the chart's version (0.5.13) against the PR's target branch (main) and returnsfalse. Result:No affected apps or appsets, skipping.Fix
Iterate all sources; include the app if any source's
targetRevisionmatches the branch (or is empty /HEAD). This lets chart-first multi-source apps be checked when their git values source targets the PR branch. Single-source behavior is unchanged.Verified against a live ArgoCD instance where a
.argocd/values.yamlPR now correctly produces a diff comment (previously "No changes").