feat(acceptance): treat a release-referenced values file as read-only context#245
Merged
Conversation
… context An Argo CD Application's helm.valueFiles or a Flux HelmRelease's spec.chart.spec.valuesFiles names a values.yaml by path. That file has no apiVersion/kind, so the acceptance gate refused it as non-krm-yaml and, because acceptance is all-or-nothing, took the whole folder down with it — including the co-located release and a valid ClusterIssuer. Read the path-valued field from the release document the store already parses and treat the file it names as read-only context: understood, never written, never a refusal for its folder. helmValueFileRefs resolves the reference (Argo $values/… repo-root form, whole-repo, and co-located subtree) to a non-KRM file, carried on ManifestStore.ValueFileRefs; acceptance.go suppresses the non-krm-yaml refusal and scan_repo.go stops counting it as noise. The file never enters FilesByPath, so it can never be written or swept. Implements docs/design/support-boundary/values-file-projection.md §2 Move 1 (and acceptance-precision.md §1c). platform/cert-manager flips refused→accepted in support-today.md. Move 2 (editable projection) remains design. Covered by unit, acceptance-gate, scan-repo golden, pkg-contract, and live e2e tests for both the Argo and Flux spellings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughReferenced Helm values files are detected for Argo CD ChangesReferenced values-file acceptance
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ScannedFiles
participant ManifestAnalyzer
participant AcceptanceGate
participant GitTarget
ScannedFiles->>ManifestAnalyzer: parse Argo Application or Flux HelmRelease
ManifestAnalyzer->>ManifestAnalyzer: resolve existing non-KRM values.yaml
ManifestAnalyzer->>AcceptanceGate: provide referenced values-file context
AcceptanceGate-->>GitTarget: accept folder without IssueNonKRM
GitTarget->>ScannedFiles: preserve referenced values.yaml during mirroring
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/manifestanalyzer/store.go (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate comments to reflect Flux HelmRelease support.
The documentation for
ValueFileRefsand its associated methods only mentions Argo CDApplications andhelm.valueFiles, but the implementation (and PR objective) also supports FluxHelmReleases viaspec.chart.spec.valuesFiles. Update these comments to accurately reflect the supported releases, preventing future confusion about the scope of this context.
internal/manifestanalyzer/store.go#L96-103: Update the field documentation to mention both Argo CD and Flux HelmReleases.internal/manifestanalyzer/store.go#L415-418: Update the inline comment to mention both Argo CD and Flux.internal/manifestanalyzer/store.go#L511-522: Update the method godoc to mention both Argo CD and Flux.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/manifestanalyzer/store.go` at line 1, Update the documentation for ValueFileRefs and its associated methods in store.go to describe support for both Argo CD Applications using helm.valueFiles and Flux HelmReleases using spec.chart.spec.valuesFiles. Revise the field comment, the inline comment near the related processing, and the method godoc without changing implementation behavior.internal/manifestanalyzer/scan_repo_test.go (1)
237-255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting resource counts for consistency.
TestScanRepo_ReferencedValuesFileAcceptedasserts that theRenderedandEditableresources are correctly counted. Consider adding a similar assertion here to ensure that theHelmReleaseis tracked correctly (rendered=1, editable=1), matching the expectations influx-helmrelease-values.golden.json. As per coding guidelines, follow the existing codebase’s testing patterns when adding new Go code.♻️ Proposed refactor
if got.Resources.NonKRM != 0 { t.Errorf("the referenced values file is context, not noise: nonKrm = %d, want 0", got.Resources.NonKRM) } + if got.Resources.Rendered != 1 || got.Resources.Editable != 1 { + t.Errorf("HelmRelease should be managed: rendered=%d editable=%d, want 1/1", + got.Resources.Rendered, got.Resources.Editable) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/manifestanalyzer/scan_repo_test.go` around lines 237 - 255, Extend TestScanRepo_FluxHelmReleaseValuesFileAccepted with assertions for got.Resources.Rendered == 1 and got.Resources.Editable == 1, matching the existing resource-count checks in TestScanRepo_ReferencedValuesFileAccepted and the fixture golden expectations.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/support-boundary/acceptance-precision.md`:
- Around line 90-94: Update the acceptance statement in the referenced
values-file section to describe any existing referenced non-KRM values file,
rather than limiting the contract to files named values.yaml. Preserve the
documented Argo CD Application and Flux HelmRelease field references and
acceptance behavior.
In `@internal/manifestanalyzer/valuefiles.go`:
- Around line 130-141: Update decodeReleases to decode each YAML document into a
yaml.Node first, then decode that node into releaseDoc so *yaml.TypeError only
skips the current document and processing continues. Preserve the existing
termination behavior for true decoder or syntax errors, ensuring the loop cannot
retry the same invalid input indefinitely.
---
Nitpick comments:
In `@internal/manifestanalyzer/scan_repo_test.go`:
- Around line 237-255: Extend TestScanRepo_FluxHelmReleaseValuesFileAccepted
with assertions for got.Resources.Rendered == 1 and got.Resources.Editable == 1,
matching the existing resource-count checks in
TestScanRepo_ReferencedValuesFileAccepted and the fixture golden expectations.
In `@internal/manifestanalyzer/store.go`:
- Line 1: Update the documentation for ValueFileRefs and its associated methods
in store.go to describe support for both Argo CD Applications using
helm.valueFiles and Flux HelmReleases using spec.chart.spec.valuesFiles. Revise
the field comment, the inline comment near the related processing, and the
method godoc without changing implementation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bda5127e-93a6-4b87-887a-3d1309609278
📒 Files selected for processing (24)
docs/design/support-boundary/acceptance-precision.mddocs/design/support-boundary/values-file-projection.mdinternal/manifestanalyzer/acceptance.gointernal/manifestanalyzer/acceptance_test.gointernal/manifestanalyzer/scan_repo.gointernal/manifestanalyzer/scan_repo_test.gointernal/manifestanalyzer/store.gointernal/manifestanalyzer/testdata/scan-repo/supported/argocd-external-helm-values.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/supported/argocd-external-helm-values/app/application.yamlinternal/manifestanalyzer/testdata/scan-repo/supported/argocd-external-helm-values/app/clusterissuer.yamlinternal/manifestanalyzer/testdata/scan-repo/supported/argocd-external-helm-values/app/values.yamlinternal/manifestanalyzer/testdata/scan-repo/supported/flux-helmrelease-values.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/supported/flux-helmrelease-values/app/helmrelease.yamlinternal/manifestanalyzer/testdata/scan-repo/supported/flux-helmrelease-values/app/values.yamlinternal/manifestanalyzer/valuefiles.gointernal/manifestanalyzer/valuefiles_test.gopkg/manifestanalyzer/folder_test.gotest/e2e/fixtures/values-file-folder-flux/helmrelease.yamltest/e2e/fixtures/values-file-folder-flux/values.yamltest/e2e/fixtures/values-file-folder/application.yamltest/e2e/fixtures/values-file-folder/clusterissuer.yamltest/e2e/fixtures/values-file-folder/values.yamltest/e2e/values_file_acceptance_e2e_test.gotest/fixtures/gitops-layouts/support-today.md
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…lti-doc decode Move 1 (a release-referenced values file is read-only context) is an acceptance-only classification, not a source-resolution or editing feature. Make the code say what it actually proves, and fix one robustness edge — per the merge requirements recorded in values-content-architecture.md. - Comments/tests now say "named read-only context" (a scan-local path match), never "the file the release consumes". Drop the overclaim that a $values ref or a Flux sourceRef proves the source is this repo; both spellings are matched by name without source-identity proof. Record the intentional policy: an Argo bare path on an external chart, or a Flux HelmRepository source, resolves inside the fetched chart, so a co-located file is a benign named passenger and is never presented as an editable deployment input. - decodeReleases: split documents as generic yaml.Node values, then convert each independently, so an incompatible earlier document (e.g. an unrelated kind with a type-mismatched spec) can no longer abort the stream and hide a later Application/HelmRelease. Adds a regression test. - Design docs: values-content-architecture.md (new) records the Git-authoring boundary, the Argo/Flux resolution table, and the staged delivery sequence; values-file-projection.md and README.md updated to the corrected framing. No change to acceptance classification; unit coverage held at 76.5%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
What
A Helm
values.yamlnamed by a release — an Argo CDApplication'shelm.valueFilesor a FluxHelmRelease'sspec.chart.spec.valuesFiles— has noapiVersion/kind, so the acceptance gate refused it asnon-krm-yaml. Because acceptance is all-or-nothing, that one file took the whole folder down with it: the co-located release and a perfectly validClusterIssuer.This reads the path-valued field from the release document the store already parses and treats the file it names as read-only context — understood, never written, and never a refusal for its folder. It implements Move 1 of
values-file-projection.md§2 (andacceptance-precision.md§1c). Move 2 (projecting the file as an editable object) remains design.How
valuefiles.go:helmValueFileRefsdecodes both release kinds through onereleaseDoctype (dispatched by group+kind) and resolves the named path through one ordered candidate set — Argo's repo-root$values/…form, a whole-repo scan, and co-located subtree resolution — to a non-KRM file only.ManifestStore.ValueFileRefs;acceptance.gosuppresses thenon-krm-yamlrefusal for it andscan_repo.gostops counting it as non-KRM noise.FilesByPath, so it can never be written or swept. Mirrors the existing kustomize-patch "read-only build context" retention.platform/cert-managerflips refused → accepted (2/2/1 → 2/2/0) insupport-today.md, rescuing itsClusterIssuer.Testing
Both the Argo and Flux spellings are covered at every layer:
scan-repofixturessupported/argocd-external-helm-valuesandsupported/flux-helmrelease-values.pkg/manifestanalyzerfolder scan.Validation:
task lint,task test(coverage held at 76.5%), andtask test-e2e(58/58 passed) all green.🤖 Generated with Claude Code
Summary by CodeRabbit
values.yamlfiles are now treated as read-only context for Argo CD Applications and Flux HelmReleases.