feat(gitops-api): treat higher-level KRM objects (HelmRelease, Argo Application, KRO) as first-class documents#203
Merged
Merged
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>
📝 WalkthroughWalkthroughThis PR adds F7 design and user documentation, new manifestedit corpus fixtures, a HelmRelease mirror/edit e2e test with templates, and a release workflow job that closes stale release-please PRs after publishing. ChangesF7 Higher-Level KRM Documents
Release Workflow Cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 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 |
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
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>
release-please builds the next release PR in the same invocation that cuts the draft release for the just-merged one, before publish-release flips that draft to published. A draft release is invisible to release-please's "latest release" lookup, so the next PR is computed against the full history and proposes a bogus "release everything" version (it cannot see the release just cut) — e.g. #206 proposed 0.31.0 with a 168-line changelog re-listing work shipped releases ago. Add a close-stale-release-pr job after publish-release: once the release is published, if main has no commits past its tag, close the open 'autorelease: pending' / 'release-please--' PR. release-please opens a fresh, correct one on the next push with releasable commits. Deterministic cleanup; leaves the draft/asset-attachment flow (required for immutable releases) untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/installing-apps-as-krm.md (1)
149-152: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify the watchability requirement here.
This reads like every watched kind needs a CRD, but core kinds and aggregated APIs are also watchable. Rephrase this to require that the resource be served, and mention CRDs only for CRD-backed kinds.
Proposed fix
-The kind's CRD must be installed and served in the cluster — the operator -resolves what to watch from the live API surface, so any served kind (core, CRD, -or aggregated) is watchable. +The resource must be served in the cluster — for CRD-backed kinds, that means +the CRD must be installed. The operator resolves what to watch from the live +API surface, so any served kind (core, CRD-backed, or aggregated) is watchable.🤖 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 `@docs/installing-apps-as-krm.md` around lines 149 - 152, Clarify the watchability wording in the installation docs so it does not imply every watched kind needs a CRD; update the sentence in the section describing the operator’s live API discovery to say the resource must be served in the cluster, and mention CRDs only for CRD-backed kinds. Keep the references to the watch model by preserving the link to configuration.md and the GitProvider / GitTarget / WatchRule terminology.
🧹 Nitpick comments (2)
.github/workflows/release.yml (2)
385-392: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider resilience for the
gh api comparecall.A single transient GitHub API error here fails the whole job with no retry, silently skipping the cleanup for this release (low impact, since it's non-blocking for the release itself, but requires manual intervention or waiting for the next release cycle to self-heal).
🤖 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 @.github/workflows/release.yml around lines 385 - 392, The release workflow’s `gh api repos/${REPO}/compare/${TAG_NAME}...main` check in the cleanup step is brittle because a transient API failure aborts the job and skips pruning. Update the logic around the compare call to be resilient by adding a retry/backoff or a safe fallback that preserves the job flow when GitHub API requests fail, and keep the behavior localized to the cleanup block that computes `ahead` and decides whether to exit early.
359-404: 📐 Maintainability & Code Quality | 🔵 TrivialSolid, well-guarded cleanup job.
Compare direction (
${TAG_NAME}...main), label/branch-prefix filtering, and theahead_by != 0fail-safe are all correct. Permissions are scoped to least privilege for this job's needs.As per coding guidelines, remember to run
task lint-actionslocally before pushing this workflow change to catch anyactionlintissues not visible in this review.🤖 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 @.github/workflows/release.yml around lines 359 - 404, Add a local validation step for this workflow change by running task lint-actions before pushing. This comment is a reminder to verify the release-please cleanup job in the workflow, especially the close-stale-release-pr step and its gh usage, with actionlint so any workflow syntax or permissions issues are caught early.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.
Outside diff comments:
In `@docs/installing-apps-as-krm.md`:
- Around line 149-152: Clarify the watchability wording in the installation docs
so it does not imply every watched kind needs a CRD; update the sentence in the
section describing the operator’s live API discovery to say the resource must be
served in the cluster, and mention CRDs only for CRD-backed kinds. Keep the
references to the watch model by preserving the link to configuration.md and the
GitProvider / GitTarget / WatchRule terminology.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 385-392: The release workflow’s `gh api
repos/${REPO}/compare/${TAG_NAME}...main` check in the cleanup step is brittle
because a transient API failure aborts the job and skips pruning. Update the
logic around the compare call to be resilient by adding a retry/backoff or a
safe fallback that preserves the job flow when GitHub API requests fail, and
keep the behavior localized to the cleanup block that computes `ahead` and
decides whether to exit early.
- Around line 359-404: Add a local validation step for this workflow change by
running task lint-actions before pushing. This comment is a reminder to verify
the release-please cleanup job in the workflow, especially the
close-stale-release-pr step and its gh usage, with actionlint so any workflow
syntax or permissions issues are caught early.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9fe8596b-218e-47a4-b4b0-8faecd9ed4a5
📒 Files selected for processing (2)
.github/workflows/release.ymldocs/installing-apps-as-krm.md
Fix the spurious "release everything" PR at its root instead of janitoring it. release-please cut the draft release AND built the next PR in one invocation; the draft is invisible to its own "latest release" lookup, so the PR was computed against full history (e.g. #206 -> 0.31.0, 168-line changelog). Split the action into two invocations: - release-please job: skip-github-pull-request=true — cut the (draft) release only, never open a PR, so no spurious PR is ever created. - release-please-pr job: skip-github-release=true — open/refresh the next release PR *after* publish-release makes the release visible, so it anchors to the published release. The PR job runs whenever CI + release-please succeeded and either no release was created (publish-release skipped) or publish-release succeeded, so a failed publish (release still a draft) never opens a PR against the draft. Keeps immutable-releases + the draft/asset-attach flow untouched; replaces the close-stale-release-pr janitor added in the previous commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reflect the release.yml change in the release docs: release-please now runs as two passes — cut the (draft) release with skip-github-pull-request, then open/refresh the next PR with skip-github-release after publish makes the release visible. Explains why (a draft is invisible to release-please's latest-release lookup, so a single-pass run proposed a full-history "release everything" PR). - docs/ci-overview.md: third "worth calling out" property covering the split. - .github/RELEASES.md: two-pass flow in "What Happens When You Push", draft → publish note, diagram note. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sunib
changed the title
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 this delivers
Two independent changes:
1. Higher-level KRM objects as first-class documents
Control-plane custom resources — Flux
HelmRelease, Argo CDApplication, KROresources — already mirror and edit in place through the operator's
kind-agnostic pipeline, exactly like core resources (no per-kind allowlist; only
a small deny list of noisy built-ins). This change makes that guarantee
load-bearing so a future special-case can't silently regress it, and documents
the "install an app = commit a KRM document" workflow for users.
No operator code changes — it's a test corpus, one e2e pin, and docs:
helmrelease.yaml,argocd-application.yaml,kro-podinfoapp.yamlunderinternal/git/manifestedit/testdata/corpus/. Theyflow 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: a live FluxHelmReleaseis mirrored to its canonical path<ns>/helm.toolkit.fluxcd.io/helmreleases/<name>.yaml, then a livespec.chart.spec.versionbump round-trips in place, preserving ahand-authored comment.
spec.suspend: truekeeps the helm-controller inert sothe object behaves like any other inert document.
docs/installing-apps-as-krm.md: installing an app iscommitting a KRM document; the chart-inflation boundary (
helmCharts:)stays refused.
2. Fix the recurring "release everything" release-please PR
After each release, release-please kept opening a spurious PR proposing a giant,
full-history changelog (it proposed
0.31.0re-listing work shipped releasesago). Root cause: it cut the GitHub release as a draft and opened the next
PR in the same run — and a draft is invisible to release-please's own "latest
release" lookup, so that PR was computed against the entire history.
Fixed by splitting the action into two passes:
release-pleasejob →skip-github-pull-request: cut the (draft) releaseonly, never open a PR.
release-please-prjob →skip-github-release: open/refresh the next PRafter
publish-releasemakes the release public and visible.Keeps immutable releases + the draft/asset-attach flow intact. Release docs
updated (
docs/ci-overview.md,.github/RELEASES.md).Validation
task lint(incl.actionlint) — clean.task test— pass; unit coverage steady at baseline (no production codechanged).
task test-e2e— the newHelmReleasemirror + version-bump +comment-preservation spec passes live. The only failure is a pre-existing,
unrelated flake in the "Unsupported Folder Refusal" spec (a known racy
GitPathAccepted/refusal status projection) — independent of this PR, whichchanges no operator Go, so the deployed binary is behaviourally identical to
main.🤖 Generated with Claude Code