fix: CMP tar should only pack annotation paths when use-manifest-generate-paths is enabled - #29326
Open
waterWang wants to merge 4 commits into
Open
Conversation
❗ Preview Environment deployment failed on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
PR Summary by QodoFix CMP tar to only stream manifest-generate-paths inclusions
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Bundle ReportChanges will decrease total bundle size by 282 bytes (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: argo-cd-ui-array-pushAssets Changed:
|
Code Review by Qodo
1. Inclusion filtering lacks tests
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+254
to
+257
| if t.inclusions != nil && relativePath != "." { | ||
| if fi.IsDir() { | ||
| if !dirMayContainInclusion(relativePath, t.inclusions) { | ||
| return filepath.SkipDir |
There was a problem hiding this comment.
1. Inclusion filtering lacks tests 📘 Rule violation ☼ Reliability
This PR changes CMP tar inclusion behavior (path-based matching plus directory pruning) but does not add/adjust automated tests to validate the new inclusion semantics. Without coverage, regressions could reintroduce over-inclusive tars or incorrectly exclude required files, impacting manifest generation reliability.
Agent Prompt
## Issue description
The PR introduces new inclusion-filtering behavior for CMP tar creation (path-based matching and directory pruning), but there are no accompanying tests that fail before the fix and pass after.
## Issue Context
Key new logic was added in `util/io/files/tar.go` (`pathMatchesInclusion`, `dirMayContainInclusion`, and new inclusion handling in `tgzFile`), and wiring was added from reposerver -> CMP stream to pass inclusion paths.
## Fix Focus Areas
- util/io/files/tar_test.go[1-290]
- util/io/files/tar.go[188-271]
- util/cmp/stream_test.go[51-94]
- util/cmp/stream.go[100-170]
- reposerver/repository/utils_test.go[11-45]
- reposerver/repository/utils.go[86-112]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+199
to
+203
| if strings.HasPrefix(relativePath, pattern+sep) { | ||
| return true | ||
| } | ||
| if matched, err := filepath.Match(pattern, relativePath); err == nil && matched { | ||
| return true |
There was a problem hiding this comment.
2. Basename inclusions broken 🐞 Bug ≡ Correctness
files.Tgz inclusions now match against the full relative path and also prune directories, which breaks callers that pass basename-only patterns (e.g. *.yaml) expecting them to match files anywhere in the tree. This can cause argocd app diff --server-side-generate to send zero files (and fail) despite the flag help text promising filename-based matching.
Agent Prompt
### Issue description
`util/io/files/tar.go` changed inclusion matching from basename-only to full relative path. This breaks existing inclusion patterns used by the CLI/server-side diff path (`--local-include` defaults to `*.yaml,*.yml,*.json`) which explicitly documents filename-based matching.
### Issue Context
Call chain:
- `cmd/argocd/commands/app_diff.go` passes `localIncludes` to `manifeststream.SendApplicationManifestQueryWithFiles`.
- `util/manifeststream/stream.go` passes `inclusions` directly into `tgzstream.CompressFiles`.
- `util/tgzstream/stream.go` passes `included` into `files.Tgz`.
- `util/io/files/tar.go` now matches `filepath.Match(pattern, relativePath)` and prunes directories via `dirMayContainInclusion`, so patterns without path separators (e.g. `*.yaml` or `kustomization.yaml`) no longer match nested files and may even cause `SkipDir` to prune the entire tree.
### How to fix
1. Preserve backward-compatible behavior for inclusion patterns that do **not** contain a path separator:
- If `pattern` contains no separator (`/` on linux), treat it as a basename pattern:
- Match against `filepath.Base(relativePath)` (exact and glob).
- Only treat patterns containing a separator as path-based patterns (current new behavior).
2. Update `dirMayContainInclusion` so that if any inclusion pattern contains no separator (basename-only), it returns `true` for all directories (cannot safely prune).
3. Add/extend unit tests in `util/io/files/tar_test.go` to cover:
- `inclusions = []string{"*.yaml"}` includes nested YAML files.
- `inclusions = []string{"kustomization.yaml"}` includes nested `kustomization.yaml`.
- `inclusions = []string{"apps/app1"}` (path-based) still prunes unrelated directories.
### Fix Focus Areas
- util/io/files/tar.go[188-238]
- util/io/files/tar.go[254-270]
- cmd/argocd/commands/app_diff.go[794-801]
- util/manifeststream/stream.go[41-49]
- util/tgzstream/stream.go[28-44]
- util/io/files/tar_test.go[1-120]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
Bug
With
--plugin-use-manifest-generate-pathsenabled, the repo-server still packs the entire common-root directory ofargocd.argoproj.io/manifest-generate-pathsand streams it to the CMP sidecar, instead of sending only the annotation paths. In large monorepos this causes tar/gRPC/untar overhead proportional to the entire common-root tree, leading toGenerateManifesttimeouts (~250× latency increase shown in reporter benchmarks).Fix
util/io/files/tar.go: Replace basename-basedinclusionsmatching with path-based matching (pathMatchesInclusion/dirMayContainInclusion) so annotations like.;../../infraonly include the paths specified, andSkipDirprunes unrelated trees.util/cmp/stream.go: AddWithInclusionsoption toSenderOption; pass it toCompressFilesinstead ofnil.reposerver/repository/utils.go: AddgetManifestGenerateIncludeRelsto compute relative inclusion paths from annotation.reposerver/repository/repository.go: WireincludeRelsfromrunConfigManagementPluginSidecarsthroughgenerateManifestsCMPto the CMP stream.Fixes #29309