refactor: extract helpers for dupl clones, and tighten the task-oriented docs plan

.docs-lint-scope

@@ -35,3 +35,12 @@ docs/demo/commit-window.md
 docs/facts/kubernetes-watch-options.md
 docs/spec/watch-event-ordering-and-attribution-grace.md
 docs/bi-directional.md
+docs/README.md
+docs/github-setup-guide.md
+docs/attribution-setup-guide.md
+docs/audit-setup/cluster/readme.md
+docs/sops-age-guide.md
+docs/commit-signing.md
+docs/commit-messages.md
+docs/definitions.md
+charts/gitops-reverser/README.md

internal/controller/applierclaim_controller.go

@@ -20,7 +20,6 @@ import (
 	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/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
@@ -174,20 +173,21 @@ func (r *ApplierClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request
 	st := beginStatus(r.Client, r.Recorder, claim)
 	switch t := claim.(type) {
 	case *configbutleraiv1alpha3.FluxClaim:
-		return r.reconcileFlux(ctx, st, t)
+		return reconcileHeldClaim(ctx, r, st, t, r.holdFlux)
 	case *configbutleraiv1alpha3.ArgoCDClaim:
-		return r.reconcileArgoCD(ctx, st, t)
+		return reconcileHeldClaim(ctx, r, st, t, r.holdArgoCD)
 	}
 	return ctrl.Result{}, fmt.Errorf("no applier claim kind %q", r.Kind)
 }
 
-// reconcileFlux is Reconcile for a FluxClaim: one being deleted, or that no eligible GitTarget
-// references any more, hands its Kustomization back; any other holds it while it is Configured.
-func (r *ApplierClaimReconciler) reconcileFlux(
-	ctx context.Context, st *reconcileStatus, claim *configbutleraiv1alpha3.FluxClaim,
+// reconcileHeldClaim is Reconcile for one applier claim: one being deleted, or that no eligible GitTarget
+// references any more, hands its applier back; any other holds it with hold while it is Configured.
+func reconcileHeldClaim[C configbutleraiv1alpha3.ApplierClaim](
+	ctx context.Context, r *ApplierClaimReconciler, st *reconcileStatus, claim C,
+	hold func(context.Context, *reconcileStatus, C, claimCheck) (conditionValue, error),
 ) (ctrl.Result, error) {
-	if !claim.DeletionTimestamp.IsZero() {
-		if !controllerutil.ContainsFinalizer(claim, configbutleraiv1alpha3.FluxClaimFinalizer) {
+	if !claim.GetDeletionTimestamp().IsZero() {
+		if !applier.Holding(claim) {
 			return ctrl.Result{}, nil
 		}
 		return r.retire(ctx, st, claim, nil)
@@ -197,7 +197,7 @@ func (r *ApplierClaimReconciler) reconcileFlux(
 	if check.retiring(claim) {
 		return r.retire(ctx, st, claim, &check)
 	}
-	inControl, err := r.holdFlux(ctx, st, claim, check)
+	inControl, err := hold(ctx, st, claim, check)
 	if err != nil {
 		return ctrl.Result{}, err
 	}

internal/controller/argocdclaim_hold.go

@@ -11,8 +11,6 @@ import (
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8stypes "k8s.io/apimachinery/pkg/types"
-	ctrl "sigs.k8s.io/controller-runtime"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3"
 	"github.com/ConfigButler/gitops-reverser/internal/applier"
@@ -41,32 +39,6 @@ import (
 // held off by GitOps Reverser, or kept off by the switch's owner (the message says which).
 const ReasonAutomatedSyncOff = "AutomatedSyncOff"
 
-// reconcileArgoCD is Reconcile for an ArgoCDClaim: one being deleted, or that no eligible
-// GitTarget references any more, hands its Application's automated sync back; any other holds it
-// off while it is Configured.
-func (r *ApplierClaimReconciler) reconcileArgoCD(
-	ctx context.Context, st *reconcileStatus, claim *configbutleraiv1alpha3.ArgoCDClaim,
-) (ctrl.Result, error) {
-	if !claim.DeletionTimestamp.IsZero() {
-		if !controllerutil.ContainsFinalizer(claim, configbutleraiv1alpha3.ArgoCDClaimFinalizer) {
-			return ctrl.Result{}, nil
-		}
-		return r.retire(ctx, st, claim, nil)
-	}
-	check := r.configuredVerdict(ctx, claim)
-	publishCheck(st, claim, check)
-	if check.retiring(claim) {
-		return r.retire(ctx, st, claim, &check)
-	}
-	inControl, err := r.holdArgoCD(ctx, st, claim, check)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-	st.setValue(ApplierClaimConditionInControl, inControl)
-	return r.commitClaim(ctx, st, r.claimReadiness(claim, check.configured, &inControl), &inControl,
-		heldClaimSteadyInterval)
-}
-
 // holdArgoCD returns an ArgoCDClaim's InControl condition. A Configured applier claim holds its
 // Application's automated sync off. One that is not Configured holds nothing, and one in line
 // behind another holder lets go of a finalizer it carries, handing nothing back.

internal/controller/dependency_watches.go

@@ -8,6 +8,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"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"
@@ -96,3 +97,22 @@ func ruleDependencyWatches(
 		Watches(&corev1.Namespace{}, handler.EnqueueRequestsFromMapFunc(namespaces),
 			builder.WithPredicates(predicate.LabelChangedPredicate{}))
 }
+
+// specOrStatusChanged passes create and delete, and an update that changed the spec (generation) or
+// what summary reads of the status: the status-only update GenerationChangedPredicate drops. An
+// update whose objects are not a T is let through.
+func specOrStatusChanged[T client.Object, S comparable](summary func(T) S) predicate.Predicate {
+	return predicate.Funcs{
+		CreateFunc:  func(event.CreateEvent) bool { return true },
+		DeleteFunc:  func(event.DeleteEvent) bool { return true },
+		GenericFunc: func(event.GenericEvent) bool { return false },
+		UpdateFunc: func(e event.UpdateEvent) bool {
+			before, ok1 := e.ObjectOld.(T)
+			after, ok2 := e.ObjectNew.(T)
+			if !ok1 || !ok2 {
+				return true
+			}
+			return before.GetGeneration() != after.GetGeneration() || summary(before) != summary(after)
+		},
+	}
+}

internal/controller/gittarget_controller.go

@@ -1735,43 +1735,17 @@ func kubeProviderReadyStatus(cp *configbutleraiv1alpha3.KubeProvider) metav1.Con
 // controller: fire on create/delete, and on an update when the SPEC changed (generation) OR the
 // Ready condition status changed — the latter a status-only update GenerationChangedPredicate drops.
 func kubeProviderReadyOrSpecChanged() predicate.Predicate {
-	return predicate.Funcs{
-		CreateFunc:  func(event.CreateEvent) bool { return true },
-		DeleteFunc:  func(event.DeleteEvent) bool { return true },
-		GenericFunc: func(event.GenericEvent) bool { return false },
-		UpdateFunc: func(e event.UpdateEvent) bool {
-			oldKP, ok1 := e.ObjectOld.(*configbutleraiv1alpha3.KubeProvider)
-			newKP, ok2 := e.ObjectNew.(*configbutleraiv1alpha3.KubeProvider)
-			if !ok1 || !ok2 {
-				return true
-			}
-			return oldKP.Generation != newKP.Generation ||
-				kubeProviderReadyStatus(oldKP) != kubeProviderReadyStatus(newKP)
-		},
-	}
+	return specOrStatusChanged(kubeProviderReadyStatus)
 }
 
 // gitBranchReadyOrSpecChanged is the GitBranch watch predicate for the GitTarget controller: fire on
 // create/delete, and on an update when the spec changed or the Ready condition's status or reason
 // did. A status.remote write alone is filtered.
 func gitBranchReadyOrSpecChanged() predicate.Predicate {
-	readyOf := func(b *configbutleraiv1alpha3.GitBranch) string {
+	return specOrStatusChanged(func(b *configbutleraiv1alpha3.GitBranch) string {
 		if c := findCondition(b.Status.Conditions, ConditionTypeReady); c != nil {
 			return string(c.Status) + "/" + c.Reason
 		}
 		return ""
-	}
-	return predicate.Funcs{
-		CreateFunc:  func(event.CreateEvent) bool { return true },
-		DeleteFunc:  func(event.DeleteEvent) bool { return true },
-		GenericFunc: func(event.GenericEvent) bool { return false },
-		UpdateFunc: func(e event.UpdateEvent) bool {
-			oldB, ok1 := e.ObjectOld.(*configbutleraiv1alpha3.GitBranch)
-			newB, ok2 := e.ObjectNew.(*configbutleraiv1alpha3.GitBranch)
-			if !ok1 || !ok2 {
-				return true
-			}
-			return oldB.Generation != newB.Generation || readyOf(oldB) != readyOf(newB)
-		},
-	}
+	})
 }

internal/controller/gittarget_dependency_status.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
 	configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3"
@@ -74,23 +73,13 @@ func gitTargetReadyCondition(target configbutleraiv1alpha3.GitTarget) conditionV
 // reports the same reason as a stall or as progress, so a target can flip Stalled while its mirrored
 // Status and Reason stay put, and the rule judges its stall from that verdict (see ruleReadiness).
 func gitTargetReadyProjectionChanged() predicate.Predicate {
-	return predicate.Funcs{
-		CreateFunc:  func(event.CreateEvent) bool { return true },
-		DeleteFunc:  func(event.DeleteEvent) bool { return true },
-		GenericFunc: func(event.GenericEvent) bool { return false },
-		UpdateFunc: func(e event.UpdateEvent) bool {
-			oldTarget, ok1 := e.ObjectOld.(*configbutleraiv1alpha3.GitTarget)
-			newTarget, ok2 := e.ObjectNew.(*configbutleraiv1alpha3.GitTarget)
-			if !ok1 || !ok2 {
-				return true
-			}
-			if oldTarget.Generation != newTarget.Generation {
-				return true
-			}
-			before := gitTargetReadyCondition(*oldTarget)
-			after := gitTargetReadyCondition(*newTarget)
-			return before.Status != after.Status || before.Reason != after.Reason ||
-				gitTargetStalled(*oldTarget) != gitTargetStalled(*newTarget)
-		},
+	type projection struct {
+		status  metav1.ConditionStatus
+		reason  string
+		stalled bool
 	}
+	return specOrStatusChanged(func(target *configbutleraiv1alpha3.GitTarget) projection {
+		ready := gitTargetReadyCondition(*target)
+		return projection{status: ready.Status, reason: ready.Reason, stalled: gitTargetStalled(*target)}
+	})
 }

internal/queue/fact_index_store.go

@@ -170,23 +170,14 @@ func newScopeFacts() *scopeFacts {
 
 // putExact stores the immutable (uid, rv) fact.
 func (s *scopeFacts) putExact(uid, rv string, entry *indexedFact) {
-	key := exactFactKey{uid: uid, rv: rv}
-	if _, ok := s.exact[key]; !ok {
-		s.count++
-	}
-	s.exact[key] = entry
-	s.order = append(s.order, factRef{kind: factKindExact, uid: uid, rv: rv, seq: entry.seq})
+	putKeyed(s, s.exact, exactFactKey{uid: uid, rv: rv}, entry, factRef{kind: factKindExact, uid: uid, rv: rv})
 }
 
 // putLatest stores the last-writer-wins pointer for an object. Entries are applied in delivery
 // order, which is what makes "last writer" mean the last fact appended rather than whichever
 // goroutine got there first.
 func (s *scopeFacts) putLatest(uid string, entry *indexedFact) {
-	if _, ok := s.latest[uid]; !ok {
-		s.count++
-	}
-	s.latest[uid] = entry
-	s.order = append(s.order, factRef{kind: factKindLatest, uid: uid, seq: entry.seq})
+	putKeyed(s, s.latest, uid, entry, factRef{kind: factKindLatest, uid: uid})
 }
 
 // putRemoval stores the sticky removal pointer for an object: the fact that it was DELETED, and by
@@ -203,32 +194,32 @@ func (s *scopeFacts) putLatest(uid string, entry *indexedFact) {
 //
 // A later removal fact may replace it, because that is a statement about the same question.
 func (s *scopeFacts) putRemoval(uid string, entry *indexedFact) {
-	if _, ok := s.removals[uid]; !ok {
-		s.count++
-	}
-	s.removals[uid] = entry
-	s.order = append(s.order, factRef{kind: factKindRemoval, uid: uid, seq: entry.seq})
+	putKeyed(s, s.removals, uid, entry, factRef{kind: factKindRemoval, uid: uid})
 }
 
 // putRV stores the rv-only escape hatch.
 func (s *scopeFacts) putRV(rv string, entry *indexedFact) {
-	if _, ok := s.rvOnly[rv]; !ok {
-		s.count++
-	}
-	s.rvOnly[rv] = entry
-	s.order = append(s.order, factRef{kind: factKindRV, rv: rv, seq: entry.seq})
+	putKeyed(s, s.rvOnly, rv, entry, factRef{kind: factKindRV, rv: rv})
 }
 
 // putName stores the (namespace, name) floor. Like putLatest it is last-writer-wins: two writes to
 // one name inside the TTL leave the later actor, which is the same answer the uid tier would give
 // for the same pair of writes.
 func (s *scopeFacts) putName(namespace, name string, entry *indexedFact) {
-	key := nameFactKey{namespace: namespace, name: name}
-	if _, ok := s.byName[key]; !ok {
+	putKeyed(s, s.byName, nameFactKey{namespace: namespace, name: name}, entry,
+		factRef{kind: factKindName, namespace: namespace, name: name})
+}
+
+// putKeyed stores entry under key in one keyed tier, and its reference last in order. It is counted
+// only when the key is new: the entry it overwrites leaves a stale reference in order, which remove
+// skips.
+func putKeyed[K comparable](s *scopeFacts, tier map[K]*indexedFact, key K, entry *indexedFact, ref factRef) {
+	if _, ok := tier[key]; !ok {
 		s.count++
 	}
-	s.byName[key] = entry
-	s.order = append(s.order, factRef{kind: factKindName, namespace: namespace, name: name, seq: entry.seq})
+	tier[key] = entry
+	ref.seq = entry.seq
+	s.order = append(s.order, ref)
 }
 
 // putCollection appends one collection fact. Collection facts are not keyed one per namespace:

internal/typeset/observe.go

@@ -95,23 +95,9 @@ func observationFromEntry(
 // (gvk-not-unique). Exact duplicates (same GVR) collapse, so a doubly-listed resource
 // is not mistaken for a conflict.
 func distinctGVRsByGVK(entries []Entry) map[schema.GroupVersionKind][]schema.GroupVersionResource {
-	seen := map[schema.GroupVersionKind]map[schema.GroupVersionResource]struct{}{}
-	for _, e := range entries {
-		if e.Subresource {
-			continue
-		}
-		if seen[e.GVK] == nil {
-			seen[e.GVK] = map[schema.GroupVersionResource]struct{}{}
-		}
-		seen[e.GVK][e.GVR] = struct{}{}
-	}
-	out := make(map[schema.GroupVersionKind][]schema.GroupVersionResource, len(seen))
-	for gvk, set := range seen {
-		for gvr := range set {
-			out[gvk] = append(out[gvk], gvr)
-		}
-	}
-	return out
+	return distinctByKey(entries, func(e Entry) (schema.GroupVersionKind, schema.GroupVersionResource) {
+		return e.GVK, e.GVR
+	})
 }
 
 // distinctGVKsByGVR indexes each resource to the distinct kinds it resolves to, so a
@@ -121,20 +107,29 @@ func distinctGVRsByGVK(entries []Entry) map[schema.GroupVersionKind][]schema.Gro
 // hand-crafted (snapshot) surface; modelling it keeps the bijection honest in both
 // directions rather than silently picking a winner.
 func distinctGVKsByGVR(entries []Entry) map[schema.GroupVersionResource][]schema.GroupVersionKind {
-	seen := map[schema.GroupVersionResource]map[schema.GroupVersionKind]struct{}{}
+	return distinctByKey(entries, func(e Entry) (schema.GroupVersionResource, schema.GroupVersionKind) {
+		return e.GVR, e.GVK
+	})
+}
+
+// distinctByKey indexes the top-level entries by the key pair returns, to the distinct values it
+// returns for that key. Subresources are skipped, and the order of each value list is unspecified.
+func distinctByKey[K, V comparable](entries []Entry, pair func(Entry) (K, V)) map[K][]V {
+	seen := map[K]map[V]struct{}{}
 	for _, e := range entries {
 		if e.Subresource {
 			continue
 		}
-		if seen[e.GVR] == nil {
-			seen[e.GVR] = map[schema.GroupVersionKind]struct{}{}
+		key, value := pair(e)
+		if seen[key] == nil {
+			seen[key] = map[V]struct{}{}
 		}
-		seen[e.GVR][e.GVK] = struct{}{}
+		seen[key][value] = struct{}{}
 	}
-	out := make(map[schema.GroupVersionResource][]schema.GroupVersionKind, len(seen))
-	for gvr, set := range seen {
-		for gvk := range set {
-			out[gvr] = append(out[gvr], gvk)
+	out := make(map[K][]V, len(seen))
+	for key, set := range seen {
+		for value := range set {
+			out[key] = append(out[key], value)
 		}
 	}
 	return out