From b122fb297fb1d9b7e4b71046f13db05ccaf8dc50 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 28 Feb 2026 05:06:22 +0000 Subject: [PATCH] Refactor reconcilers to use Knative ReconcileKind pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the key-splitting, resource-fetching, and not-found handling into a generic internal.NewReconciler wrapper so that each reconciler only implements ReconcileKind(ctx, *TypedResource) — matching the Knative generated reconciler convention. The controller setup wraps the reconciler with internal.NewReconciler, which bridges Reconcile(ctx, key) to ReconcileKind(ctx, *T). This removes duplicated boilerplate from all three reconcilers (push, sync, resolver), eliminates their Reconcile/LeaderAware methods, and makes splitKey an unexported implementation detail. https://claude.ai/code/session_01Eu3LyxX3G1KCcw4aDzmHvp --- pkg/reconciler/internal/key.go | 4 +- pkg/reconciler/internal/key_test.go | 2 +- pkg/reconciler/internal/reconcile.go | 67 +++++++++++++++++++++++++++ pkg/reconciler/push/controller.go | 10 +++- pkg/reconciler/push/push.go | 39 ++-------------- pkg/reconciler/resolver/controller.go | 9 +++- pkg/reconciler/resolver/resolver.go | 31 ++----------- pkg/reconciler/sync/controller.go | 9 +++- pkg/reconciler/sync/sync.go | 31 ++----------- 9 files changed, 106 insertions(+), 96 deletions(-) create mode 100644 pkg/reconciler/internal/reconcile.go diff --git a/pkg/reconciler/internal/key.go b/pkg/reconciler/internal/key.go index ec50510..527e6b5 100644 --- a/pkg/reconciler/internal/key.go +++ b/pkg/reconciler/internal/key.go @@ -2,9 +2,9 @@ package internal import "strings" -// SplitKey splits a "namespace/name" key into its components. +// splitKey splits a "namespace/name" key into its components. // If the key has no slash, the namespace is empty and the whole key is the name. -func SplitKey(key string) (namespace, name string) { +func splitKey(key string) (namespace, name string) { namespace, name, _ = strings.Cut(key, "/") if name == "" { // No slash found: Cut returns (key, "", false). The key is the name. diff --git a/pkg/reconciler/internal/key_test.go b/pkg/reconciler/internal/key_test.go index 6f59a6a..a0dff52 100644 --- a/pkg/reconciler/internal/key_test.go +++ b/pkg/reconciler/internal/key_test.go @@ -39,7 +39,7 @@ func TestSplitKey(t *testing.T) { for _, tt := range tests { t.Run(tt.key, func(t *testing.T) { - ns, name := SplitKey(tt.key) + ns, name := splitKey(tt.key) if ns != tt.wantNS { t.Errorf("namespace = %q, want %q", ns, tt.wantNS) } diff --git a/pkg/reconciler/internal/reconcile.go b/pkg/reconciler/internal/reconcile.go new file mode 100644 index 0000000..76efc89 --- /dev/null +++ b/pkg/reconciler/internal/reconcile.go @@ -0,0 +1,67 @@ +package internal + +import ( + "context" + "fmt" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + "knative.dev/pkg/logging" + "knative.dev/pkg/reconciler" +) + +// KindReconciler is the interface that a developer implements. +// It mirrors the Knative generated reconciler pattern: the resource +// has already been fetched and the developer only provides business logic. +type KindReconciler[T any] interface { + ReconcileKind(ctx context.Context, o *T) reconciler.Event +} + +// reconcilerImpl bridges between the controller's Reconcile(ctx, key) interface +// and the developer's ReconcileKind(ctx, *T) method. It handles splitting the +// workqueue key, fetching the typed resource, and ignoring not-found (deleted) +// resources — exactly what Knative's generated reconcilers do. +type reconcilerImpl[T any] struct { + get func(ctx context.Context, namespace, name string) (*T, error) + inner KindReconciler[T] +} + +// Verify interface conformance. +var _ reconciler.LeaderAware = (*reconcilerImpl[any])(nil) + +// NewReconciler wraps a KindReconciler into a controller-compatible reconciler +// that implements Reconcile(ctx, key) error. The get function fetches the typed +// resource by namespace and name. +func NewReconciler[T any]( + get func(ctx context.Context, namespace, name string) (*T, error), + inner KindReconciler[T], +) *reconcilerImpl[T] { + return &reconcilerImpl[T]{get: get, inner: inner} +} + +// Reconcile implements the controller.Reconciler interface. +func (r *reconcilerImpl[T]) Reconcile(ctx context.Context, key string) error { + logger := logging.FromContext(ctx) + + namespace, name := splitKey(key) + + o, err := r.get(ctx, namespace, name) + if apierrors.IsNotFound(err) { + logger.Debugf("%s/%s no longer exists", namespace, name) + return nil + } + if err != nil { + return fmt.Errorf("getting %s/%s: %w", namespace, name, err) + } + + return r.inner.ReconcileKind(ctx, o) +} + +// Promote implements reconciler.LeaderAware. +func (r *reconcilerImpl[T]) Promote(bkt reconciler.Bucket, enq func(reconciler.Bucket, types.NamespacedName)) error { + return nil +} + +// Demote implements reconciler.LeaderAware. +func (r *reconcilerImpl[T]) Demote(bkt reconciler.Bucket) { +} diff --git a/pkg/reconciler/push/controller.go b/pkg/reconciler/push/controller.go index 371576c..61abccd 100644 --- a/pkg/reconciler/push/controller.go +++ b/pkg/reconciler/push/controller.go @@ -4,6 +4,7 @@ import ( "context" "time" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/dynamic" "k8s.io/client-go/dynamic/dynamicinformer" @@ -15,6 +16,7 @@ import ( gitv1alpha1 "github.com/imjasonh/git-k8s/pkg/apis/git/v1alpha1" gitclient "github.com/imjasonh/git-k8s/pkg/client" + "github.com/imjasonh/git-k8s/pkg/reconciler/internal" ) var pushTransactionGVR = schema.GroupVersionResource{ @@ -45,7 +47,12 @@ func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl gitClient: gitClient, } - impl := controller.NewContext(ctx, r, controller.ControllerOptions{ + impl := controller.NewContext(ctx, internal.NewReconciler( + func(ctx context.Context, namespace, name string) (*gitv1alpha1.GitPushTransaction, error) { + return gitClient.GitPushTransactions(namespace).Get(ctx, name, metav1.GetOptions{}) + }, + r, + ), controller.ControllerOptions{ WorkQueueName: "GitPushTransactions", Logger: logger, }) @@ -67,4 +74,3 @@ func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl return impl } - diff --git a/pkg/reconciler/push/push.go b/pkg/reconciler/push/push.go index 2010539..da29072 100644 --- a/pkg/reconciler/push/push.go +++ b/pkg/reconciler/push/push.go @@ -11,17 +11,14 @@ import ( "github.com/go-git/go-git/v5/plumbing/transport/http" "github.com/go-git/go-git/v5/storage/memory" corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/dynamic" "knative.dev/pkg/logging" "knative.dev/pkg/reconciler" gitv1alpha1 "github.com/imjasonh/git-k8s/pkg/apis/git/v1alpha1" gitclient "github.com/imjasonh/git-k8s/pkg/client" - "github.com/imjasonh/git-k8s/pkg/reconciler/internal" ) // Reconciler implements the reconcile logic for GitPushTransaction. @@ -31,20 +28,9 @@ type Reconciler struct { } // ReconcileKind processes a single GitPushTransaction resource. -func (r *Reconciler) ReconcileKind(ctx context.Context, key string) reconciler.Event { +func (r *Reconciler) ReconcileKind(ctx context.Context, txn *gitv1alpha1.GitPushTransaction) reconciler.Event { logger := logging.FromContext(ctx) - - namespace, name := internal.SplitKey(key) - - // Fetch the GitPushTransaction. - txn, err := r.gitClient.GitPushTransactions(namespace).Get(ctx, name, metav1.GetOptions{}) - if apierrors.IsNotFound(err) { - logger.Debugf("GitPushTransaction %s/%s no longer exists", namespace, name) - return nil - } - if err != nil { - return fmt.Errorf("getting GitPushTransaction %s/%s: %w", namespace, name, err) - } + namespace := txn.Namespace // Skip if already terminal. if txn.Status.Phase == gitv1alpha1.TransactionPhaseSucceeded || @@ -57,7 +43,7 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, key string) reconciler.E now := metav1.Now() txn.Status.Phase = gitv1alpha1.TransactionPhaseInProgress txn.Status.StartTime = &now - txn, err = r.gitClient.GitPushTransactions(namespace).UpdateStatus(ctx, txn, metav1.UpdateOptions{}) + txn, err := r.gitClient.GitPushTransactions(namespace).UpdateStatus(ctx, txn, metav1.UpdateOptions{}) if err != nil { return fmt.Errorf("updating status to InProgress: %w", err) } @@ -95,7 +81,7 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, key string) reconciler.E logger.Warnf("Failed to update branch CRs after push: %v", err) } - logger.Infof("Successfully pushed transaction %s/%s", namespace, name) + logger.Infof("Successfully pushed transaction %s/%s", namespace, txn.Name) return nil } @@ -224,20 +210,3 @@ func (r *Reconciler) updateBranches(ctx context.Context, namespace string, txn * } return nil } - -// Ensure Reconciler implements the reconciler interface. -var _ reconciler.LeaderAware = (*Reconciler)(nil) - -// Promote implements LeaderAware. -func (r *Reconciler) Promote(bkt reconciler.Bucket, enq func(reconciler.Bucket, types.NamespacedName)) error { - return nil -} - -// Demote implements LeaderAware. -func (r *Reconciler) Demote(bkt reconciler.Bucket) { -} - -// Reconcile implements the controller.Reconciler interface. -func (r *Reconciler) Reconcile(ctx context.Context, key string) error { - return r.ReconcileKind(ctx, key) -} diff --git a/pkg/reconciler/resolver/controller.go b/pkg/reconciler/resolver/controller.go index c2878a9..ce1cc67 100644 --- a/pkg/reconciler/resolver/controller.go +++ b/pkg/reconciler/resolver/controller.go @@ -4,6 +4,7 @@ import ( "context" "time" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/dynamic" @@ -16,6 +17,7 @@ import ( gitv1alpha1 "github.com/imjasonh/git-k8s/pkg/apis/git/v1alpha1" gitclient "github.com/imjasonh/git-k8s/pkg/client" + "github.com/imjasonh/git-k8s/pkg/reconciler/internal" ) var repoSyncGVR = schema.GroupVersionResource{ @@ -44,7 +46,12 @@ func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl gitClient: gitClient, } - impl := controller.NewContext(ctx, r, controller.ControllerOptions{ + impl := controller.NewContext(ctx, internal.NewReconciler( + func(ctx context.Context, namespace, name string) (*gitv1alpha1.GitRepoSync, error) { + return gitClient.GitRepoSyncs(namespace).Get(ctx, name, metav1.GetOptions{}) + }, + r, + ), controller.ControllerOptions{ WorkQueueName: "GitConflictResolver", Logger: logger, }) diff --git a/pkg/reconciler/resolver/resolver.go b/pkg/reconciler/resolver/resolver.go index 74efb57..b639818 100644 --- a/pkg/reconciler/resolver/resolver.go +++ b/pkg/reconciler/resolver/resolver.go @@ -9,16 +9,13 @@ import ( "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/object" "github.com/go-git/go-git/v5/storage/memory" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/dynamic" "knative.dev/pkg/logging" "knative.dev/pkg/reconciler" gitv1alpha1 "github.com/imjasonh/git-k8s/pkg/apis/git/v1alpha1" gitclient "github.com/imjasonh/git-k8s/pkg/client" - "github.com/imjasonh/git-k8s/pkg/reconciler/internal" ) // Reconciler implements the reconcile logic for resolving conflicted GitRepoSyncs. @@ -27,28 +24,17 @@ type Reconciler struct { gitClient *gitclient.GitV1alpha1Client } -// Reconcile implements the controller.Reconciler interface. -func (r *Reconciler) Reconcile(ctx context.Context, key string) error { +// ReconcileKind processes a single GitRepoSync resource that is in the Conflicted phase. +func (r *Reconciler) ReconcileKind(ctx context.Context, syncObj *gitv1alpha1.GitRepoSync) reconciler.Event { logger := logging.FromContext(ctx) - - namespace, name := internal.SplitKey(key) - - // Fetch the GitRepoSync. - syncObj, err := r.gitClient.GitRepoSyncs(namespace).Get(ctx, name, metav1.GetOptions{}) - if apierrors.IsNotFound(err) { - logger.Debugf("GitRepoSync %s/%s no longer exists", namespace, name) - return nil - } - if err != nil { - return fmt.Errorf("getting GitRepoSync %s/%s: %w", namespace, name, err) - } + namespace := syncObj.Namespace // Only process if still conflicted. if syncObj.Status.Phase != gitv1alpha1.SyncPhaseConflicted { return nil } - logger.Infof("Attempting to resolve conflict for %s/%s", namespace, name) + logger.Infof("Attempting to resolve conflict for %s/%s", namespace, syncObj.Name) // Get the repo URLs. repoA, err := r.gitClient.GitRepositories(namespace).Get(ctx, syncObj.Spec.RepoA.Name, metav1.GetOptions{}) @@ -348,12 +334,3 @@ func (r *Reconciler) markManualIntervention(ctx context.Context, syncObj *gitv1a _, err := r.gitClient.GitRepoSyncs(syncObj.Namespace).UpdateStatus(ctx, syncObj, metav1.UpdateOptions{}) return err } - -var _ reconciler.LeaderAware = (*Reconciler)(nil) - -func (r *Reconciler) Promote(bkt reconciler.Bucket, enq func(reconciler.Bucket, types.NamespacedName)) error { - return nil -} - -func (r *Reconciler) Demote(bkt reconciler.Bucket) { -} diff --git a/pkg/reconciler/sync/controller.go b/pkg/reconciler/sync/controller.go index d9a8f87..2e8bee7 100644 --- a/pkg/reconciler/sync/controller.go +++ b/pkg/reconciler/sync/controller.go @@ -4,6 +4,7 @@ import ( "context" "time" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/dynamic" @@ -16,6 +17,7 @@ import ( gitv1alpha1 "github.com/imjasonh/git-k8s/pkg/apis/git/v1alpha1" gitclient "github.com/imjasonh/git-k8s/pkg/client" + "github.com/imjasonh/git-k8s/pkg/reconciler/internal" ) var ( @@ -52,7 +54,12 @@ func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl gitClient: gitClient, } - impl := controller.NewContext(ctx, r, controller.ControllerOptions{ + impl := controller.NewContext(ctx, internal.NewReconciler( + func(ctx context.Context, namespace, name string) (*gitv1alpha1.GitRepoSync, error) { + return gitClient.GitRepoSyncs(namespace).Get(ctx, name, metav1.GetOptions{}) + }, + r, + ), controller.ControllerOptions{ WorkQueueName: "GitRepoSyncs", Logger: logger, }) diff --git a/pkg/reconciler/sync/sync.go b/pkg/reconciler/sync/sync.go index b49ee21..0c98a48 100644 --- a/pkg/reconciler/sync/sync.go +++ b/pkg/reconciler/sync/sync.go @@ -7,16 +7,13 @@ import ( "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/storage/memory" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/dynamic" "knative.dev/pkg/logging" "knative.dev/pkg/reconciler" gitv1alpha1 "github.com/imjasonh/git-k8s/pkg/apis/git/v1alpha1" gitclient "github.com/imjasonh/git-k8s/pkg/client" - "github.com/imjasonh/git-k8s/pkg/reconciler/internal" ) // Reconciler implements the reconcile logic for GitRepoSync. @@ -25,21 +22,11 @@ type Reconciler struct { gitClient *gitclient.GitV1alpha1Client } -// Reconcile implements the controller.Reconciler interface. -func (r *Reconciler) Reconcile(ctx context.Context, key string) error { +// ReconcileKind processes a single GitRepoSync resource. +func (r *Reconciler) ReconcileKind(ctx context.Context, syncObj *gitv1alpha1.GitRepoSync) reconciler.Event { logger := logging.FromContext(ctx) - - namespace, name := internal.SplitKey(key) - - // Fetch the GitRepoSync resource. - syncObj, err := r.gitClient.GitRepoSyncs(namespace).Get(ctx, name, metav1.GetOptions{}) - if apierrors.IsNotFound(err) { - logger.Debugf("GitRepoSync %s/%s no longer exists", namespace, name) - return nil - } - if err != nil { - return fmt.Errorf("getting GitRepoSync %s/%s: %w", namespace, name, err) - } + namespace := syncObj.Namespace + name := syncObj.Name // Get the branches for both repos. branchA, err := r.findBranch(ctx, namespace, syncObj.Spec.RepoA.Name, syncObj.Spec.BranchName) @@ -211,13 +198,3 @@ func (r *Reconciler) updateSyncStatus(ctx context.Context, syncObj *gitv1alpha1. _, err := r.gitClient.GitRepoSyncs(syncObj.Namespace).UpdateStatus(ctx, syncObj, metav1.UpdateOptions{}) return err } - -// Ensure Reconciler implements the reconciler interface. -var _ reconciler.LeaderAware = (*Reconciler)(nil) - -func (r *Reconciler) Promote(bkt reconciler.Bucket, enq func(reconciler.Bucket, types.NamespacedName)) error { - return nil -} - -func (r *Reconciler) Demote(bkt reconciler.Bucket) { -}