feat(PL-6661): implement release pruning respecting prune annotation - #35
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe operator now enables Argo CD pruning, safely deletes Applications during annotated Release removal, and manages a cleanup finalizer. Integration tests use server-side apply and verify pruning behavior. Kubernetes and related Go dependencies were updated. ChangesPruning lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant KubernetesRelease
participant ReleaseReconciler
participant ArgoCDApplication
KubernetesRelease->>ReleaseReconciler: report annotated Release deletion
ReleaseReconciler->>ArgoCDApplication: delete associated Application
ArgoCDApplication-->>ReleaseReconciler: confirm deletion or not found
ReleaseReconciler->>KubernetesRelease: remove prune-release finalizer
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
adb8d63 to
2ddd1b0
Compare
2ddd1b0 to
15e0b7c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
cmd/operator/main_test.go (3)
837-837: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the shadowing variable.
Line 837 assigns an
argocd.Applicationto a variable namedrelease, which shadows thev1alpha1.Releasefrom line 791. Line 845 then reports "expected application" while checkingrelease.DeletionTimestamp. Name itapp.♻️ Proposed change
- release, err := appsIntf.Get(t.Context(), "staging-test", metav1.GetOptions{}) + app, err := appsIntf.Get(t.Context(), "staging-test", metav1.GetOptions{}) if err != nil { if kerrors.IsNotFound(err) { return nil } return fmt.Errorf("expected app to be not found but got: %v", err) } - if release.DeletionTimestamp.IsZero() { + if app.DeletionTimestamp.IsZero() {Also applies to: 844-844
🤖 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 `@cmd/operator/main_test.go` at line 837, Rename the variable assigned by appsIntf.Get in the affected test from release to app, and update its later references, including the DeletionTimestamp check and related error message, while preserving the v1alpha1.Release variable named release.
253-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a test-specific field manager instead of
joyOperator.These applies use the same field manager as the operator. Server-side apply then treats test writes and operator writes as one owner. Two effects follow:
- Ownership conflicts between the test fixture and the operator are never reported, even with
Forceabsent.- The operator can remove fields that the test set, and the test cannot detect it.
TestMainalready uses a distinct manager at line 171 ("operator-tests"). Use the same value here.♻️ Proposed change (apply to each call site)
- metav1.ApplyOptions{FieldManager: joyOperator}, + metav1.ApplyOptions{FieldManager: "operator-tests"},Also applies to: 289-289, 311-311, 355-355, 632-632
🤖 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 `@cmd/operator/main_test.go` at line 253, Replace joyOperator with the test-specific field manager "operator-tests" in every metav1.ApplyOptions call identified in the diff, matching the manager already used by TestMain. Keep the existing apply behavior and options unchanged.
688-712: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
TestReleasePruningreuses cluster-wide names and leaves state behind.The test applies
project/test,catalog/catalogandenvironment/staging.TestHappyReconciliationsuses the same names, andTestEnvironmentSourcePatternalso appliescatalog/catalogand runshelm upgradewith a changedEnvironmentSourcePattern. All three tests share one kind cluster. The tests therefore depend on execution order and on the leftover state of earlier tests.The test also creates no cleanup. The Project, Catalog and Environment stay in the cluster after the test ends.
Add
t.Cleanupcalls that delete the applied resources, or use unique names per test.Also applies to: 720-748, 756-770
🤖 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 `@cmd/operator/main_test.go` around lines 688 - 712, Update TestReleasePruning and the related resource-application blocks to avoid shared cluster-wide names by generating unique Project, Catalog, and Environment names per test, or register t.Cleanup callbacks that delete every applied resource. Apply the same isolation to the blocks around the referenced symbols so TestHappyReconciliations and TestEnvironmentSourcePattern cannot depend on leftover state or execution order.cmd/operator/reconciler_environment.go (1)
124-124: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
retry.DefaultRetryand return conflict errors unwrapped.Two points about the retry helper:
DefaultBackoffuses 4 steps starting at 10 ms with factor 5.0.DefaultRetryis documented as the recommended retry for a conflict where multiple clients are making changes to the same resource, which matches this case, because the Argo CD application controller updates the same Application.- The client-go example states that the callback must return err itself, not wrapped inside another error, so that RetryOnConflict can identify it correctly. Lines 127 and 137 wrap the error with
fmt.Errorf. Recent apimachinery unwraps througherrors.As, so this most likely still works, but the wrapping also affects thekerrors.IsNotFound(err)check at line 142. Confirm the behavior of the vendored apimachinery version, or return the raw errors and wrap once at line 143.Also applies to: 127-127
🤖 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 `@cmd/operator/reconciler_environment.go` at line 124, Update the RetryOnConflict call in the reconciler to use retry.DefaultRetry instead of retry.DefaultBackoff. Within its callback, return conflict errors directly without fmt.Errorf wrapping, including the error paths around the referenced lines, so conflict detection and kerrors.IsNotFound remain reliable; if context is needed, wrap only once at the outer error return.
🤖 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 `@cmd/operator/main_test.go`:
- Around line 832-853: Extend the test around the release deletion to first
retrieve the Release and assert its metadata includes the
joy.nesto.ca/prune-release finalizer before calling releaseIntf.Delete. After
deletion, add an Eventually check using releaseIntf.Get that succeeds only when
the Release returns a NotFound error, while preserving the existing Argo CD
Application deletion assertion.
In `@cmd/operator/reconciler_release.go`:
- Around line 49-59: Update cmd/operator/reconciler_release.go lines 49-59 and
the finalizer writes in that reconciler to copy the cached release before
mutation, then apply a minimal object containing only apiVersion, kind,
metadata.name, metadata.namespace, and metadata.finalizers. Update
cmd/operator/reconciler_environment.go line 135 to avoid applying the full
object returned by appIntf.Get; apply only the identity fields and
spec.syncPolicy, or use a JSON patch.
- Around line 80-117: Handle deleted releases before environment/project lookups
so missing environments cannot prevent finalizer cleanup. In the deletion path,
delete the Application only when shouldPrune is true, then remove
finalizerPruneRelease whenever hasPruneReleaseFinalizer regardless of the
annotation, and return without re-applying the Application. Use
release.Namespace when constructing the Application name via appName, then keep
environment/project lookup and normal Application apply exclusively for
non-deleted releases.
In `@go.mod`:
- Around line 12-17: Update the indirect google.golang.org/protobuf dependency
in go.mod from the pseudo-version to the v1.36.12 release by running go get
google.golang.org/protobuf@latest, then commit the resulting go.mod and reduced
go.sum changes.
---
Nitpick comments:
In `@cmd/operator/main_test.go`:
- Line 837: Rename the variable assigned by appsIntf.Get in the affected test
from release to app, and update its later references, including the
DeletionTimestamp check and related error message, while preserving the
v1alpha1.Release variable named release.
- Line 253: Replace joyOperator with the test-specific field manager
"operator-tests" in every metav1.ApplyOptions call identified in the diff,
matching the manager already used by TestMain. Keep the existing apply behavior
and options unchanged.
- Around line 688-712: Update TestReleasePruning and the related
resource-application blocks to avoid shared cluster-wide names by generating
unique Project, Catalog, and Environment names per test, or register t.Cleanup
callbacks that delete every applied resource. Apply the same isolation to the
blocks around the referenced symbols so TestHappyReconciliations and
TestEnvironmentSourcePattern cannot depend on leftover state or execution order.
In `@cmd/operator/reconciler_environment.go`:
- Line 124: Update the RetryOnConflict call in the reconciler to use
retry.DefaultRetry instead of retry.DefaultBackoff. Within its callback, return
conflict errors directly without fmt.Errorf wrapping, including the error paths
around the referenced lines, so conflict detection and kerrors.IsNotFound remain
reliable; if context is needed, wrap only once at the outer error return.
🪄 Autofix
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: 10898b77-72bf-4eb7-a24f-da4801c28284
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/operator/main_test.gocmd/operator/reconciler_environment.gocmd/operator/reconciler_release.gogo.mod
| releaseIntf := k8s.TypedInterface[v1alpha1.Release]( | ||
| ctrl.Client(ctx), | ||
| schema.GroupVersionResource{Group: v1alpha1.Group, Version: v1alpha1.Version, Resource: "releases"}, | ||
| ).Namespace(event.Namespace) | ||
|
|
||
| release, err := releaseCache.Get(event.Name) | ||
| if kerrors.IsNotFound(err) { | ||
| // If the release is no longer in the cache, a deletion event happened. However, that doesn't mean that the release doesn't yet | ||
| // still exist in etcd with a deletion timestamp and finalizer. In order to handle cleanup, we need to check its live state. | ||
| release, err = releaseIntf.Get(ctx, event.Name, metav1.GetOptions{}) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Server-side apply of a full live-read object claims ownership of every field. Both reconcilers read an object with Get, mutate one field, then send the whole object through Apply with FieldManager: joyOperator and Force: true. Server-side apply treats every field present in the request body as owned by that manager, and Force: true takes fields away from their current owners. The operator then owns fields it never intended to manage, and later removals by the real owner no longer take effect.
cmd/operator/reconciler_release.go#L49-L59: for the finalizer writes at lines 90, 96 and 111, build a minimal object that carries onlyapiVersion,kind,metadata.name,metadata.namespaceandmetadata.finalizers. Also copy the object before mutatingrelease.Finalizers, becausereleaseCache.Getcan return a pointer into the shared informer cache.cmd/operator/reconciler_environment.go#L135-L135: replace the full-object apply of the value returned byappIntf.Getwith a minimal apply that carries only the identity fields andspec.syncPolicy, or use a JSON patch.
📍 Affects 2 files
cmd/operator/reconciler_release.go#L49-L59(this comment)cmd/operator/reconciler_environment.go#L135-L135
🤖 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 `@cmd/operator/reconciler_release.go` around lines 49 - 59, Update
cmd/operator/reconciler_release.go lines 49-59 and the finalizer writes in that
reconciler to copy the cached release before mutation, then apply a minimal
object containing only apiVersion, kind, metadata.name, metadata.namespace, and
metadata.finalizers. Update cmd/operator/reconciler_environment.go line 135 to
avoid applying the full object returned by appIntf.Get; apply only the identity
fields and spec.syncPolicy, or use a JSON patch.
| github.com/yokecd/yoke v0.20.25 | ||
| go.yaml.in/yaml/v3 v3.0.5 | ||
| k8s.io/api v0.36.3 | ||
| k8s.io/apiextensions-apiserver v0.36.3 | ||
| k8s.io/apimachinery v0.36.3 | ||
| k8s.io/client-go v0.36.3 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for mod in github.com/yokecd/yoke k8s.io/api k8s.io/client-go google.golang.org/protobuf; do
echo "== $mod"
curl -s "https://proxy.golang.org/${mod}/@latest" | head -c 400; echo
done
grep -n 'protobuf' go.mod go.sum | head -20
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "google.golang.org/protobuf") {
nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } }
}
}'Repository: nestoca/joy-operator
Length of output: 1942
Replace the indirect protobuf pseudo-version with a release.
google.golang.org/protobuf is pinned via an indirect requirement to v1.36.12-0.20260120151049-f2248ac996af; a go.sum entry is present, so it was not hand-edited. Run go get google.golang.org/protobuf@latest to prefer v1.36.12 and commit the reduced go.sum diff. The bumped versions exist and are outside the known advisory ranges.
🤖 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 `@go.mod` around lines 12 - 17, Update the indirect google.golang.org/protobuf
dependency in go.mod from the pseudo-version to the v1.36.12 release by running
go get google.golang.org/protobuf@latest, then commit the resulting go.mod and
reduced go.sum changes.
15e0b7c to
c68b8ef
Compare
c68b8ef to
aa0ae49
Compare
2 participants
Context: https://nestoca.atlassian.net/browse/PLT-6661
This PR implements pruning on releases because its a desirable feature, but also so that we have a mechanism via which we can remove previews from clusters.
Summary by CodeRabbit
New Features
Bug Fixes
Tests