From 0413b21e763703929f5dd0124eea3418f4d4d91f Mon Sep 17 00:00:00 2001 From: Nicacio Oliveira Date: Fri, 22 May 2026 13:38:40 -0300 Subject: [PATCH 1/2] feat(decofile): cascade-delete via Revision ownerReference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DecofileReconciler now adds the Knative Revision matching a Decofile's deploymentId as an ownerReference (controller=false) on the Decofile itself. When the Revision is later garbage-collected — either by Knative GC (maxNonActiveRevisions) or by the cluster's knative-clean-revisions CronJob — Kubernetes garbage collection cascades through Revision -> Decofile -> ConfigMap, eliminating orphan accumulation. Mechanism: * Reconcile path: after fetching the Decofile, syncRevisionOwnerRefs lists Revisions in the same namespace whose app.deco/deploymentId label matches the Decofile's deploymentId (or metadata.name when spec.deploymentId is empty), and appends any not-yet-present Revision as an ownerReference. Refetch-before-update avoids optimistic concurrency conflicts with the status update that happens later in the same Reconcile. Failures are logged but non-fatal — the operator continues without owner refs rather than blocking ConfigMap creation. * Watch path: Revision Create events enqueue any Decofile in the same namespace whose effective deploymentId matches the new Revision's label, so newly-created Revisions trigger ownerRef sync even when the corresponding Decofile already existed (the common case where the admin/build pipeline creates the Decofile slightly before the KSvc). Edge cases covered by the implementation and the unit tests: * No matching Revision yet: nothing is added; reconcile completes. * Revision in DeletionTimestamp: skipped (don't link to dying owners). * Multiple Revisions with same deploymentId (rollback): both become owners. Kubernetes GC waits for ALL owners to be deleted before reclaiming the Decofile. * Re-running Reconcile is idempotent — existing UIDs are detected and not duplicated. * Explicit spec.deploymentId is respected; decoys matching only the Decofile name are ignored. Defense-in-depth: the controller-cluster CronJob (PR decocms/infra_applications#92, deferred) remains useful as a fallback for legacy Decofiles created before this change, and for the rare case where the Revision is deleted before the operator manages to patch the ownerRef. Those orphans are still detectable by the script in infra_applications. Backfill of the existing fleet (~2276 Decofiles cluster-wide): the new operator will retroactively patch ownerRefs to any Decofile that still has a matching Revision, the next time each one is reconciled. Decofiles whose Revisions are already gone (likely the majority of the backlog) won't gain an owner — those require a one-off cleanup. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../clusterrole-operator-manager-role.yaml | 1 + config/rbac/role.yaml | 1 + internal/controller/decofile_controller.go | 155 +++++++++ internal/controller/decofile_owner_test.go | 300 ++++++++++++++++++ 4 files changed, 457 insertions(+) create mode 100644 internal/controller/decofile_owner_test.go diff --git a/chart/templates/clusterrole-operator-manager-role.yaml b/chart/templates/clusterrole-operator-manager-role.yaml index f0cd39e..e723350 100644 --- a/chart/templates/clusterrole-operator-manager-role.yaml +++ b/chart/templates/clusterrole-operator-manager-role.yaml @@ -121,6 +121,7 @@ rules: - apiGroups: - serving.knative.dev resources: + - revisions - services verbs: - get diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 84ecaaf..2fa3d19 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -122,6 +122,7 @@ rules: - apiGroups: - serving.knative.dev resources: + - revisions - services verbs: - get diff --git a/internal/controller/decofile_controller.go b/internal/controller/decofile_controller.go index 5a54d61..33461d6 100644 --- a/internal/controller/decofile_controller.go +++ b/internal/controller/decofile_controller.go @@ -27,17 +27,27 @@ import ( "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" + servingv1 "knative.dev/serving/pkg/apis/serving/v1" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" decositesv1alpha1 "github.com/deco-sites/decofile-operator/api/v1alpha1" ) const condTypePodsNotified = "PodsNotified" +// deploymentIdLabel is declared in notifier.go (same package). + // DecofileReconciler reconciles a Decofile object type DecofileReconciler struct { client.Client @@ -53,6 +63,7 @@ type DecofileReconciler struct { // +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=pods,verbs=get;list;watch +// +kubebuilder:rbac:groups=serving.knative.dev,resources=revisions,verbs=get;list;watch // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. @@ -80,6 +91,13 @@ func (r *DecofileReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c log.V(1).Info("Fetched Decofile", "duration", time.Since(fetchStart)) + // Sync Revision ownerReferences so deletion cascades when the Knative + // Revision referencing this Decofile is garbage-collected. + // Non-fatal: failures are logged but don't block reconcile. + if err := r.syncRevisionOwnerRefs(ctx, decofile); err != nil { + log.Error(err, "Failed to sync Revision ownerReferences (non-fatal)") + } + // Define the ConfigMap name configMapName := decofile.ConfigMapName() @@ -384,11 +402,148 @@ func updateCondition(decofile *decositesv1alpha1.Decofile, newCondition metav1.C decofile.Status.Conditions = append(decofile.Status.Conditions, newCondition) } +// syncRevisionOwnerRefs ensures the Decofile carries an ownerReference for +// every Knative Revision in the same namespace that targets it (matched via +// the app.deco/deploymentId label). When the Revision is later deleted — +// either by Knative GC (maxNonActiveRevisions) or by the knative-clean-* +// CronJobs — Kubernetes garbage collection cascades through Revision -> +// Decofile -> ConfigMap, so we never accumulate orphaned configuration. +// +// Multiple matching Revisions (e.g. during a rollback that reuses a +// deploymentId) all become owners; the Decofile is only GC'd once every +// owner Revision is gone. Stale UIDs are cleaned by Kubernetes GC on its +// own — we never remove ownerReferences here. +func (r *DecofileReconciler) syncRevisionOwnerRefs(ctx context.Context, decofile *decositesv1alpha1.Decofile) error { + log := logf.FromContext(ctx) + + deploymentId := decofile.Spec.DeploymentId + if deploymentId == "" { + deploymentId = decofile.Name + } + + revs := &servingv1.RevisionList{} + if err := r.List(ctx, revs, + client.InNamespace(decofile.Namespace), + client.MatchingLabels{deploymentIdLabel: deploymentId}, + ); err != nil { + return fmt.Errorf("list revisions for deploymentId=%s: %w", deploymentId, err) + } + if len(revs.Items) == 0 { + return nil + } + + existingUIDs := make(map[types.UID]bool, len(decofile.OwnerReferences)) + for _, or := range decofile.OwnerReferences { + existingUIDs[or.UID] = true + } + + var toAdd []metav1.OwnerReference + for i := range revs.Items { + rev := &revs.Items[i] + if rev.DeletionTimestamp != nil { + continue + } + if existingUIDs[rev.UID] { + continue + } + toAdd = append(toAdd, metav1.OwnerReference{ + APIVersion: servingv1.SchemeGroupVersion.String(), + Kind: "Revision", + Name: rev.Name, + UID: rev.UID, + Controller: ptr.To(false), + BlockOwnerDeletion: ptr.To(false), + }) + } + if len(toAdd) == 0 { + return nil + } + + // Refetch + re-diff against the latest version to avoid optimistic + // concurrency conflicts with status updates that happen later in + // the reconcile loop. + fresh := &decositesv1alpha1.Decofile{} + if err := r.Get(ctx, client.ObjectKey{Name: decofile.Name, Namespace: decofile.Namespace}, fresh); err != nil { + return fmt.Errorf("refetch decofile: %w", err) + } + freshUIDs := make(map[types.UID]bool, len(fresh.OwnerReferences)) + for _, or := range fresh.OwnerReferences { + freshUIDs[or.UID] = true + } + var actualAdd []metav1.OwnerReference + for _, or := range toAdd { + if !freshUIDs[or.UID] { + actualAdd = append(actualAdd, or) + } + } + if len(actualAdd) == 0 { + return nil + } + fresh.OwnerReferences = append(fresh.OwnerReferences, actualAdd...) + if err := r.Update(ctx, fresh); err != nil { + return fmt.Errorf("update decofile owner refs: %w", err) + } + + for _, or := range actualAdd { + log.Info("Added Revision as Decofile owner", + "decofile", decofile.Name, "revision", or.Name, "uid", or.UID) + } + return nil +} + +// mapRevisionToDecofile maps a Revision event to the Decofile that should be +// reconciled because of it. Used by the Revision watch so that new Revisions +// (created after their Decofile) still trigger ownerReference sync. +func (r *DecofileReconciler) mapRevisionToDecofile(ctx context.Context, obj client.Object) []reconcile.Request { + rev, ok := obj.(*servingv1.Revision) + if !ok { + return nil + } + deploymentId := rev.Labels[deploymentIdLabel] + if deploymentId == "" { + return nil + } + decofiles := &decositesv1alpha1.DecofileList{} + if err := r.List(ctx, decofiles, client.InNamespace(rev.Namespace)); err != nil { + return nil + } + var reqs []reconcile.Request + for i := range decofiles.Items { + df := &decofiles.Items[i] + depId := df.Spec.DeploymentId + if depId == "" { + depId = df.Name + } + if depId == deploymentId { + reqs = append(reqs, reconcile.Request{ + NamespacedName: client.ObjectKey{Namespace: df.Namespace, Name: df.Name}, + }) + } + } + return reqs +} + // SetupWithManager sets up the controller with the Manager. func (r *DecofileReconciler) SetupWithManager(mgr ctrl.Manager) error { + // Only react to Revision Create — Updates and Deletes don't add new + // ownerRef linkage information (Kubernetes GC already handles deletes + // via existing ownerReferences). Skipping them keeps the controller + // from spinning on Status churn of running Revisions. + revisionCreateOnly := predicate.Funcs{ + CreateFunc: func(_ event.CreateEvent) bool { return true }, + UpdateFunc: func(_ event.UpdateEvent) bool { return false }, + DeleteFunc: func(_ event.DeleteEvent) bool { return false }, + GenericFunc: func(_ event.GenericEvent) bool { return false }, + } + return ctrl.NewControllerManagedBy(mgr). For(&decositesv1alpha1.Decofile{}). Owns(&corev1.ConfigMap{}). + Watches( + &servingv1.Revision{}, + handler.EnqueueRequestsFromMapFunc(r.mapRevisionToDecofile), + builder.WithPredicates(revisionCreateOnly), + ). Named("decofile"). WithOptions(controller.Options{ MaxConcurrentReconciles: 8, // Allow 8 parallel reconciliations diff --git a/internal/controller/decofile_owner_test.go b/internal/controller/decofile_owner_test.go new file mode 100644 index 0000000..dda4ac5 --- /dev/null +++ b/internal/controller/decofile_owner_test.go @@ -0,0 +1,300 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + servingv1 "knative.dev/serving/pkg/apis/serving/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + decositesv1alpha1 "github.com/deco-sites/decofile-operator/api/v1alpha1" +) + +func newOwnerTestScheme(t *testing.T) *runtime.Scheme { + t.Helper() + s := runtime.NewScheme() + if err := decositesv1alpha1.AddToScheme(s); err != nil { + t.Fatalf("add decosites scheme: %v", err) + } + if err := servingv1.AddToScheme(s); err != nil { + t.Fatalf("add servingv1 scheme: %v", err) + } + return s +} + +func makeRevision(name, namespace, deploymentId string, uid types.UID) *servingv1.Revision { + return &servingv1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + UID: uid, + Labels: map[string]string{deploymentIdLabel: deploymentId}, + }, + } +} + +func makeDecofile(name, namespace, deploymentId string) *decositesv1alpha1.Decofile { + df := &decositesv1alpha1.Decofile{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + } + if deploymentId != "" { + df.Spec.DeploymentId = deploymentId + } + return df +} + +func TestSyncRevisionOwnerRefs_AddsOwnerWhenRevisionExists(t *testing.T) { + ctx := context.Background() + scheme := newOwnerTestScheme(t) + + df := makeDecofile("mhsygflbgo", "sites-foo", "") + rev := makeRevision("foo-site-mhsygflbgo", "sites-foo", "mhsygflbgo", "rev-uid-1") + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() + r := &DecofileReconciler{Client: c, Scheme: scheme} + + if err := r.syncRevisionOwnerRefs(ctx, df); err != nil { + t.Fatalf("sync failed: %v", err) + } + + got := &decositesv1alpha1.Decofile{} + if err := c.Get(ctx, client.ObjectKey{Name: "mhsygflbgo", Namespace: "sites-foo"}, got); err != nil { + t.Fatalf("get decofile: %v", err) + } + if len(got.OwnerReferences) != 1 { + t.Fatalf("want 1 owner, got %d", len(got.OwnerReferences)) + } + or := got.OwnerReferences[0] + if or.Kind != "Revision" || or.Name != "foo-site-mhsygflbgo" || or.UID != "rev-uid-1" { + t.Errorf("unexpected ownerRef: %+v", or) + } + if or.Controller == nil || *or.Controller { + t.Errorf("controller should be false, got %v", or.Controller) + } + if or.BlockOwnerDeletion == nil || *or.BlockOwnerDeletion { + t.Errorf("blockOwnerDeletion should be false, got %v", or.BlockOwnerDeletion) + } +} + +func TestSyncRevisionOwnerRefs_NoMatchingRevisionDoesNothing(t *testing.T) { + ctx := context.Background() + scheme := newOwnerTestScheme(t) + + df := makeDecofile("orphan", "sites-foo", "") + // Revision with DIFFERENT deploymentId — must not be picked up. + rev := makeRevision("foo-site-other", "sites-foo", "other", "rev-uid-2") + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() + r := &DecofileReconciler{Client: c, Scheme: scheme} + + if err := r.syncRevisionOwnerRefs(ctx, df); err != nil { + t.Fatalf("sync failed: %v", err) + } + + got := &decositesv1alpha1.Decofile{} + if err := c.Get(ctx, client.ObjectKey{Name: "orphan", Namespace: "sites-foo"}, got); err != nil { + t.Fatalf("get decofile: %v", err) + } + if len(got.OwnerReferences) != 0 { + t.Fatalf("want 0 owners for orphan, got %d", len(got.OwnerReferences)) + } +} + +func TestSyncRevisionOwnerRefs_IsIdempotent(t *testing.T) { + ctx := context.Background() + scheme := newOwnerTestScheme(t) + + df := makeDecofile("mhsygflbgo", "sites-foo", "") + rev := makeRevision("foo-site-mhsygflbgo", "sites-foo", "mhsygflbgo", "rev-uid-3") + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() + r := &DecofileReconciler{Client: c, Scheme: scheme} + + for i := 0; i < 3; i++ { + if err := r.syncRevisionOwnerRefs(ctx, df); err != nil { + t.Fatalf("sync %d failed: %v", i, err) + } + // Refresh local df to mirror what a real reconcile loop would see. + if err := c.Get(ctx, client.ObjectKey{Name: df.Name, Namespace: df.Namespace}, df); err != nil { + t.Fatalf("refresh: %v", err) + } + } + + if len(df.OwnerReferences) != 1 { + t.Fatalf("want exactly 1 ownerRef after 3 syncs, got %d", len(df.OwnerReferences)) + } +} + +func TestSyncRevisionOwnerRefs_SkipsRevisionBeingDeleted(t *testing.T) { + ctx := context.Background() + scheme := newOwnerTestScheme(t) + + df := makeDecofile("mhsygflbgo", "sites-foo", "") + rev := makeRevision("foo-site-mhsygflbgo", "sites-foo", "mhsygflbgo", "rev-uid-4") + now := metav1.Now() + rev.DeletionTimestamp = &now + rev.Finalizers = []string{"keep-alive-for-test"} + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() + r := &DecofileReconciler{Client: c, Scheme: scheme} + + if err := r.syncRevisionOwnerRefs(ctx, df); err != nil { + t.Fatalf("sync failed: %v", err) + } + + got := &decositesv1alpha1.Decofile{} + if err := c.Get(ctx, client.ObjectKey{Name: df.Name, Namespace: df.Namespace}, got); err != nil { + t.Fatalf("get decofile: %v", err) + } + if len(got.OwnerReferences) != 0 { + t.Fatalf("want 0 owners (terminating Revision skipped), got %d", len(got.OwnerReferences)) + } +} + +func TestSyncRevisionOwnerRefs_MultipleRevisionsBecomeMultipleOwners(t *testing.T) { + ctx := context.Background() + scheme := newOwnerTestScheme(t) + + df := makeDecofile("mhsygflbgo", "sites-foo", "") + // Two Revisions with the same deploymentId (rollback scenario). + rev1 := makeRevision("foo-site-mhsygflbgo-old", "sites-foo", "mhsygflbgo", "uid-old") + rev2 := makeRevision("foo-site-mhsygflbgo-new", "sites-foo", "mhsygflbgo", "uid-new") + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev1, rev2).Build() + r := &DecofileReconciler{Client: c, Scheme: scheme} + + if err := r.syncRevisionOwnerRefs(ctx, df); err != nil { + t.Fatalf("sync failed: %v", err) + } + + got := &decositesv1alpha1.Decofile{} + if err := c.Get(ctx, client.ObjectKey{Name: df.Name, Namespace: df.Namespace}, got); err != nil { + t.Fatalf("get decofile: %v", err) + } + if len(got.OwnerReferences) != 2 { + t.Fatalf("want 2 ownerRefs (both Revisions keep the Decofile alive), got %d", len(got.OwnerReferences)) + } + uids := map[types.UID]bool{} + for _, or := range got.OwnerReferences { + uids[or.UID] = true + } + if !uids["uid-old"] || !uids["uid-new"] { + t.Errorf("missing one of the expected UIDs: %v", uids) + } +} + +func TestSyncRevisionOwnerRefs_RespectsExplicitDeploymentId(t *testing.T) { + ctx := context.Background() + scheme := newOwnerTestScheme(t) + + // Decofile name and explicit deploymentId differ — the explicit one wins. + df := makeDecofile("any-name", "sites-foo", "explicit-dep") + rev := makeRevision("rev-by-explicit", "sites-foo", "explicit-dep", "uid-explicit") + // Decoy Revision with deploymentId matching the Decofile name — must NOT + // be picked up because spec.deploymentId is set. + decoy := makeRevision("rev-decoy", "sites-foo", "any-name", "uid-decoy") + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev, decoy).Build() + r := &DecofileReconciler{Client: c, Scheme: scheme} + + if err := r.syncRevisionOwnerRefs(ctx, df); err != nil { + t.Fatalf("sync failed: %v", err) + } + + got := &decositesv1alpha1.Decofile{} + if err := c.Get(ctx, client.ObjectKey{Name: df.Name, Namespace: df.Namespace}, got); err != nil { + t.Fatalf("get decofile: %v", err) + } + if len(got.OwnerReferences) != 1 { + t.Fatalf("want 1 ownerRef, got %d", len(got.OwnerReferences)) + } + if got.OwnerReferences[0].UID != "uid-explicit" { + t.Errorf("want uid-explicit, got %s", got.OwnerReferences[0].UID) + } +} + +func TestMapRevisionToDecofile_FindsByDefaultedDeploymentId(t *testing.T) { + ctx := context.Background() + scheme := newOwnerTestScheme(t) + + // Decofile uses metadata.name as effective deploymentId (spec.deploymentId empty). + df := makeDecofile("mhsygflbgo", "sites-foo", "") + rev := makeRevision("foo-site-mhsygflbgo", "sites-foo", "mhsygflbgo", "uid") + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() + r := &DecofileReconciler{Client: c, Scheme: scheme} + + reqs := r.mapRevisionToDecofile(ctx, rev) + if len(reqs) != 1 { + t.Fatalf("want 1 request, got %d", len(reqs)) + } + want := reconcile.Request{NamespacedName: client.ObjectKey{Namespace: "sites-foo", Name: "mhsygflbgo"}} + if reqs[0] != want { + t.Errorf("want %v, got %v", want, reqs[0]) + } +} + +func TestMapRevisionToDecofile_IgnoresRevisionWithoutLabel(t *testing.T) { + ctx := context.Background() + scheme := newOwnerTestScheme(t) + + df := makeDecofile("mhsygflbgo", "sites-foo", "") + rev := &servingv1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "no-label", + Namespace: "sites-foo", + UID: "uid", + // Note: no deploymentIdLabel. + }, + } + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() + r := &DecofileReconciler{Client: c, Scheme: scheme} + + reqs := r.mapRevisionToDecofile(ctx, rev) + if len(reqs) != 0 { + t.Fatalf("want 0 requests, got %d", len(reqs)) + } +} + +func TestMapRevisionToDecofile_FindsByExplicitDeploymentId(t *testing.T) { + ctx := context.Background() + scheme := newOwnerTestScheme(t) + + df := makeDecofile("any-name", "sites-foo", "explicit-dep") + rev := makeRevision("rev", "sites-foo", "explicit-dep", "uid") + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() + r := &DecofileReconciler{Client: c, Scheme: scheme} + + reqs := r.mapRevisionToDecofile(ctx, rev) + if len(reqs) != 1 { + t.Fatalf("want 1 request, got %d", len(reqs)) + } + if reqs[0].Name != "any-name" { + t.Errorf("want any-name, got %s", reqs[0].Name) + } +} From 2585560758afc81a906c5211aaa45095b3b1847c Mon Sep 17 00:00:00 2001 From: Nicacio Oliveira Date: Fri, 22 May 2026 16:03:26 -0300 Subject: [PATCH 2/2] fix(decofile): satisfy golangci-lint (prealloc, unparam) - prealloc: pre-size toAdd slice to len(revs.Items) - unparam: drop unused namespace param from test helpers (always "sites-foo") --- internal/controller/decofile_controller.go | 2 +- internal/controller/decofile_owner_test.go | 48 +++++++++++----------- 2 files changed, 26 insertions(+), 24 deletions(-) diff --git a/internal/controller/decofile_controller.go b/internal/controller/decofile_controller.go index 33461d6..ca7162d 100644 --- a/internal/controller/decofile_controller.go +++ b/internal/controller/decofile_controller.go @@ -437,7 +437,7 @@ func (r *DecofileReconciler) syncRevisionOwnerRefs(ctx context.Context, decofile existingUIDs[or.UID] = true } - var toAdd []metav1.OwnerReference + toAdd := make([]metav1.OwnerReference, 0, len(revs.Items)) for i := range revs.Items { rev := &revs.Items[i] if rev.DeletionTimestamp != nil { diff --git a/internal/controller/decofile_owner_test.go b/internal/controller/decofile_owner_test.go index dda4ac5..8e030bd 100644 --- a/internal/controller/decofile_owner_test.go +++ b/internal/controller/decofile_owner_test.go @@ -43,22 +43,24 @@ func newOwnerTestScheme(t *testing.T) *runtime.Scheme { return s } -func makeRevision(name, namespace, deploymentId string, uid types.UID) *servingv1.Revision { +const testNamespace = "sites-foo" + +func makeRevision(name, deploymentId string, uid types.UID) *servingv1.Revision { return &servingv1.Revision{ ObjectMeta: metav1.ObjectMeta{ Name: name, - Namespace: namespace, + Namespace: testNamespace, UID: uid, Labels: map[string]string{deploymentIdLabel: deploymentId}, }, } } -func makeDecofile(name, namespace, deploymentId string) *decositesv1alpha1.Decofile { +func makeDecofile(name, deploymentId string) *decositesv1alpha1.Decofile { df := &decositesv1alpha1.Decofile{ ObjectMeta: metav1.ObjectMeta{ Name: name, - Namespace: namespace, + Namespace: testNamespace, }, } if deploymentId != "" { @@ -71,8 +73,8 @@ func TestSyncRevisionOwnerRefs_AddsOwnerWhenRevisionExists(t *testing.T) { ctx := context.Background() scheme := newOwnerTestScheme(t) - df := makeDecofile("mhsygflbgo", "sites-foo", "") - rev := makeRevision("foo-site-mhsygflbgo", "sites-foo", "mhsygflbgo", "rev-uid-1") + df := makeDecofile("mhsygflbgo", "") + rev := makeRevision("foo-site-mhsygflbgo", "mhsygflbgo", "rev-uid-1") c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() r := &DecofileReconciler{Client: c, Scheme: scheme} @@ -104,9 +106,9 @@ func TestSyncRevisionOwnerRefs_NoMatchingRevisionDoesNothing(t *testing.T) { ctx := context.Background() scheme := newOwnerTestScheme(t) - df := makeDecofile("orphan", "sites-foo", "") + df := makeDecofile("orphan", "") // Revision with DIFFERENT deploymentId — must not be picked up. - rev := makeRevision("foo-site-other", "sites-foo", "other", "rev-uid-2") + rev := makeRevision("foo-site-other", "other", "rev-uid-2") c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() r := &DecofileReconciler{Client: c, Scheme: scheme} @@ -128,8 +130,8 @@ func TestSyncRevisionOwnerRefs_IsIdempotent(t *testing.T) { ctx := context.Background() scheme := newOwnerTestScheme(t) - df := makeDecofile("mhsygflbgo", "sites-foo", "") - rev := makeRevision("foo-site-mhsygflbgo", "sites-foo", "mhsygflbgo", "rev-uid-3") + df := makeDecofile("mhsygflbgo", "") + rev := makeRevision("foo-site-mhsygflbgo", "mhsygflbgo", "rev-uid-3") c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() r := &DecofileReconciler{Client: c, Scheme: scheme} @@ -152,8 +154,8 @@ func TestSyncRevisionOwnerRefs_SkipsRevisionBeingDeleted(t *testing.T) { ctx := context.Background() scheme := newOwnerTestScheme(t) - df := makeDecofile("mhsygflbgo", "sites-foo", "") - rev := makeRevision("foo-site-mhsygflbgo", "sites-foo", "mhsygflbgo", "rev-uid-4") + df := makeDecofile("mhsygflbgo", "") + rev := makeRevision("foo-site-mhsygflbgo", "mhsygflbgo", "rev-uid-4") now := metav1.Now() rev.DeletionTimestamp = &now rev.Finalizers = []string{"keep-alive-for-test"} @@ -178,10 +180,10 @@ func TestSyncRevisionOwnerRefs_MultipleRevisionsBecomeMultipleOwners(t *testing. ctx := context.Background() scheme := newOwnerTestScheme(t) - df := makeDecofile("mhsygflbgo", "sites-foo", "") + df := makeDecofile("mhsygflbgo", "") // Two Revisions with the same deploymentId (rollback scenario). - rev1 := makeRevision("foo-site-mhsygflbgo-old", "sites-foo", "mhsygflbgo", "uid-old") - rev2 := makeRevision("foo-site-mhsygflbgo-new", "sites-foo", "mhsygflbgo", "uid-new") + rev1 := makeRevision("foo-site-mhsygflbgo-old", "mhsygflbgo", "uid-old") + rev2 := makeRevision("foo-site-mhsygflbgo-new", "mhsygflbgo", "uid-new") c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev1, rev2).Build() r := &DecofileReconciler{Client: c, Scheme: scheme} @@ -211,11 +213,11 @@ func TestSyncRevisionOwnerRefs_RespectsExplicitDeploymentId(t *testing.T) { scheme := newOwnerTestScheme(t) // Decofile name and explicit deploymentId differ — the explicit one wins. - df := makeDecofile("any-name", "sites-foo", "explicit-dep") - rev := makeRevision("rev-by-explicit", "sites-foo", "explicit-dep", "uid-explicit") + df := makeDecofile("any-name", "explicit-dep") + rev := makeRevision("rev-by-explicit", "explicit-dep", "uid-explicit") // Decoy Revision with deploymentId matching the Decofile name — must NOT // be picked up because spec.deploymentId is set. - decoy := makeRevision("rev-decoy", "sites-foo", "any-name", "uid-decoy") + decoy := makeRevision("rev-decoy", "any-name", "uid-decoy") c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev, decoy).Build() r := &DecofileReconciler{Client: c, Scheme: scheme} @@ -241,8 +243,8 @@ func TestMapRevisionToDecofile_FindsByDefaultedDeploymentId(t *testing.T) { scheme := newOwnerTestScheme(t) // Decofile uses metadata.name as effective deploymentId (spec.deploymentId empty). - df := makeDecofile("mhsygflbgo", "sites-foo", "") - rev := makeRevision("foo-site-mhsygflbgo", "sites-foo", "mhsygflbgo", "uid") + df := makeDecofile("mhsygflbgo", "") + rev := makeRevision("foo-site-mhsygflbgo", "mhsygflbgo", "uid") c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() r := &DecofileReconciler{Client: c, Scheme: scheme} @@ -261,7 +263,7 @@ func TestMapRevisionToDecofile_IgnoresRevisionWithoutLabel(t *testing.T) { ctx := context.Background() scheme := newOwnerTestScheme(t) - df := makeDecofile("mhsygflbgo", "sites-foo", "") + df := makeDecofile("mhsygflbgo", "") rev := &servingv1.Revision{ ObjectMeta: metav1.ObjectMeta{ Name: "no-label", @@ -284,8 +286,8 @@ func TestMapRevisionToDecofile_FindsByExplicitDeploymentId(t *testing.T) { ctx := context.Background() scheme := newOwnerTestScheme(t) - df := makeDecofile("any-name", "sites-foo", "explicit-dep") - rev := makeRevision("rev", "sites-foo", "explicit-dep", "uid") + df := makeDecofile("any-name", "explicit-dep") + rev := makeRevision("rev", "explicit-dep", "uid") c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() r := &DecofileReconciler{Client: c, Scheme: scheme}