feat(gitops-api): F7 — higher-level KRM objects as first-class documents#203
feat(gitops-api): F7 — higher-level KRM objects as first-class documents#203sunib wants to merge 2 commits into
Conversation
…cuments Flux HelmRelease, Argo CD Application, and KRO resources already mirror and edit through the kind-agnostic pipeline exactly like core resources (no per-kind allowlist; only a small deny list of noisy built-ins). F7 makes that guarantee load-bearing rather than assumed. No product code changes. - manifestedit corpus: helmrelease.yaml, argocd-application.yaml, kro-podinfoapp.yaml flow through the existing globbed round-trip (byte-identical) and convergence (perturb-then-settle) gates. - e2e (manager,f7-higher-level-krm): a Flux HelmRelease is mirrored to its canonical path and a live chart-version bump round-trips in place, preserving a hand-authored comment. Uses the Flux CRDs already in the base e2e cluster; spec.suspend keeps the helm-controller inert. - docs: f7 design doc + user guide installing-apps-as-krm.md (install an app = add a KRM document; the chart-inflation boundary stays refused); README ladder marks F7 implemented. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds design and user documentation for "F7: Higher-level KRM objects as first-class documents," updates the GitOps API README to mark F7 as implemented, adds three manifestedit test corpus fixtures (Argo CD, HelmRelease, KRO), and introduces a new e2e test with templates validating HelmRelease mirroring and in-place comment-preserving edits. ChangesF7 Higher-Level KRM Documents
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test
participant K8s as Kubernetes API
participant Operator as Manager Operator
participant Git as Gitea Repository
Test->>K8s: Apply WatchRule for HelmReleases
Test->>K8s: Apply HelmRelease (podinfo, version 6.5.0)
Operator->>K8s: Watch HelmRelease
Operator->>Git: Mirror HelmRelease to canonical path (no status)
Test->>Git: Seed head comment into mirrored file and push
Test->>K8s: Patch HelmRelease chart version
Operator->>Git: Edit mirrored file in place, preserving comment
Test->>Git: Assert new version present, comment preserved, old version removed
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e/helmrelease_mirror_edit_e2e_test.go`:
- Around line 155-167: The fallback path in this test helper destroys the
checkout and then immediately tries to read the committed file again, which
makes the later os.ReadFile in the seeding block fail misleadingly. Update the
orphan-branch handling in the same helper that uses gitRun/mustGit so it does
not assume relPath still exists after checkout --orphan and rm -rf ., or bail
out early with a clear error before attempting to seed the comment. Keep the
file-read and write step only for the successful origin/main checkout path, or
make the fallback create the target file explicitly before reading it.
🪄 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: ffb9bb2c-c77d-4b53-bc26-74abacfd24ea
📒 Files selected for processing (10)
docs/README.mddocs/design/gitops-api/README.mddocs/design/gitops-api/f7-higher-level-krm-documents.mddocs/installing-apps-as-krm.mdinternal/git/manifestedit/testdata/corpus/argocd-application.yamlinternal/git/manifestedit/testdata/corpus/helmrelease.yamlinternal/git/manifestedit/testdata/corpus/kro-podinfoapp.yamltest/e2e/helmrelease_mirror_edit_e2e_test.gotest/e2e/templates/manager/helmrelease.tmpltest/e2e/templates/manager/watchrule-helmrelease.tmpl
| if _, err := gitRun(repo.CheckoutDir, "fetch", "origin", "main"); err == nil { | ||
| mustGit("checkout", "-B", "main", "origin/main") | ||
| mustGit("reset", "--hard", "origin/main") | ||
| } else { | ||
| mustGit("checkout", "--orphan", "main") | ||
| _, _ = gitRun(repo.CheckoutDir, "rm", "-rf", ".") | ||
| } | ||
|
|
||
| full := filepath.Join(repo.CheckoutDir, relPath) | ||
| content, readErr := os.ReadFile(full) | ||
| Expect(readErr).NotTo(HaveOccurred(), "committed file must exist before seeding a comment") | ||
| seeded := comment + "\n" + string(content) | ||
| Expect(os.WriteFile(full, []byte(seeded), 0o600)).To(Succeed()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fallback branch destroys the file it later needs.
If git fetch origin main fails, the else-branch does checkout --orphan main and rm -rf . — but line 164 then unconditionally reads the pre-existing committed file at relPath and asserts it exists ("committed file must exist before seeding a comment"). If this branch is ever hit (e.g. a transient fetch error, not just a genuinely-empty repo), the local checkout is wiped and the subsequent os.ReadFile fails, producing a confusing/misleading test failure instead of a clear one.
In practice this branch is likely unreachable here (the earlier Eventually block already confirms the file exists on origin/main), but the fallback logic is inconsistent with the function's precondition and worth tightening.
🐛 Proposed fix
- if _, err := gitRun(repo.CheckoutDir, "fetch", "origin", "main"); err == nil {
- mustGit("checkout", "-B", "main", "origin/main")
- mustGit("reset", "--hard", "origin/main")
- } else {
- mustGit("checkout", "--orphan", "main")
- _, _ = gitRun(repo.CheckoutDir, "rm", "-rf", ".")
- }
+ fetchOut, fetchErr := gitRun(repo.CheckoutDir, "fetch", "origin", "main")
+ Expect(fetchErr).NotTo(HaveOccurred(), fmt.Sprintf("git fetch origin main: %s", fetchOut))
+ mustGit("checkout", "-B", "main", "origin/main")
+ mustGit("reset", "--hard", "origin/main")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if _, err := gitRun(repo.CheckoutDir, "fetch", "origin", "main"); err == nil { | |
| mustGit("checkout", "-B", "main", "origin/main") | |
| mustGit("reset", "--hard", "origin/main") | |
| } else { | |
| mustGit("checkout", "--orphan", "main") | |
| _, _ = gitRun(repo.CheckoutDir, "rm", "-rf", ".") | |
| } | |
| full := filepath.Join(repo.CheckoutDir, relPath) | |
| content, readErr := os.ReadFile(full) | |
| Expect(readErr).NotTo(HaveOccurred(), "committed file must exist before seeding a comment") | |
| seeded := comment + "\n" + string(content) | |
| Expect(os.WriteFile(full, []byte(seeded), 0o600)).To(Succeed()) | |
| fetchOut, fetchErr := gitRun(repo.CheckoutDir, "fetch", "origin", "main") | |
| Expect(fetchErr).NotTo(HaveOccurred(), fmt.Sprintf("git fetch origin main: %s", fetchOut)) | |
| mustGit("checkout", "-B", "main", "origin/main") | |
| mustGit("reset", "--hard", "origin/main") | |
| full := filepath.Join(repo.CheckoutDir, relPath) | |
| content, readErr := os.ReadFile(full) | |
| Expect(readErr).NotTo(HaveOccurred(), "committed file must exist before seeding a comment") | |
| seeded := comment + "\n" + string(content) | |
| Expect(os.WriteFile(full, []byte(seeded), 0o600)).To(Succeed()) |
🤖 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 `@test/e2e/helmrelease_mirror_edit_e2e_test.go` around lines 155 - 167, The
fallback path in this test helper destroys the checkout and then immediately
tries to read the committed file again, which makes the later os.ReadFile in the
seeding block fail misleadingly. Update the orphan-branch handling in the same
helper that uses gitRun/mustGit so it does not assume relPath still exists after
checkout --orphan and rm -rf ., or bail out early with a clear error before
attempting to seed the comment. Keep the file-read and write step only for the
successful origin/main checkout path, or make the fallback create the target
file explicitly before reading it.
The install-apps guide explained that the operator won't *render* charts, but not the bigger configuration discipline: users must deliberately scope capture to authored intent (HelmRelease/Application/KRO documents) and keep the objects those controllers render (Deployments, Services, ConfigMaps, ReplicaSets, Pods) out of Git. Add a "capture intent, not the rendered output" section that explains why mirroring rendered output breaks round-trippability (controller-owned objects have no single writable destination), that the operator cannot tell an authored Deployment from a rendered one (only a few purely-runtime kinds are excluded by default), and how to draw the line: select intent kinds not workload kinds, separate intent from runtime by namespace, and mind overlaps. Frame it as the same principle as the chart-inflation boundary, applied at runtime. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
Implements F7 — Higher-level KRM objects as first-class documents from the
gitops-api feature ladder (
docs/design/gitops-api/README.md), the nextlaunch-critical item after F1/F4.
Higher-level control-plane custom resources — Flux
HelmRelease, Argo CDApplication, KRO resources — already mirror and edit through the operator'skind-agnostic pipeline exactly like core resources (no per-kind allowlist;
only a small deny list of noisy built-ins). F7 does not change product code;
it makes that guarantee load-bearing with a corpus + e2e so a future
special-case cannot silently regress it, and teaches "install an app = add a KRM
document" in user docs.
What's in it
docs/design/gitops-api/f7-higher-level-krm-documents.md(premise, why-prove-it, coverage split, what it deliberately doesn't do).
helmrelease.yaml,argocd-application.yaml,kro-podinfoapp.yamladded tointernal/git/manifestedit/testdata/corpus/.They flow through the two existing globbed gates with no new test code:
TestCorpusRoundTrip_ByteIdentical(re-encode byte-for-byte) andTestConvergence_Corpus(perturb-then-settle).test/e2e/helmrelease_mirror_edit_e2e_test.go(
manager,f7-higher-level-krm): a FluxHelmReleaseis mirrored to itscanonical path
<ns>/helm.toolkit.fluxcd.io/helmreleases/<name>.yaml, then alive
spec.chart.spec.versionbump round-trips in place, preserving ahand-authored comment. Uses the Flux CRDs already installed in the base e2e
cluster;
spec.suspend: truekeeps the helm-controller inert so the objectbehaves like any inert document. (The generic-CRD case is already pinned by
crd_lifecycle_e2e_test.go; this pins a real, named higher-level type.)docs/installing-apps-as-krm.md: installing an app is adding aKRM document; the chart-inflation boundary (
helmCharts:) stays refused.Validation
task lint— 0 issues.task test— pass; unit coverage steady at baseline (no production codechanged). New manifestedit corpus documents pass round-trip + convergence.
task test-e2e— the newf7-higher-level-krmspec passes (HelmReleasemirror + version-bump + comment-preservation, live), and 54 of 55 run specs
pass. The one failure is a pre-existing, unrelated flake in
Manager Unsupported Folder Refusal > refuses a hard-Kustomize path(the racy
GitPathAccepted/refusal projection — refusal recorded on the dataplane doesn't promptly re-enqueue the GitTarget, so the WatchRule
GitTargetReady=Falseprojection can lag past the 90s gate). It reproduces inisolation on this environment and is independent of this PR: no production Go
is changed here (the only
.gochange is the new e2e test), so the deployedoperator binary is behaviourally identical to
main.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation