imjasonh/git-k8s
1
0
Fork 0
mirror of https://github.com/imjasonh/git-k8s synced 2026-07-16 12:33:02 +00:00
Code Activity

Fix multiple code review issues: dedup splitKey, fix merge bug, remove dead code

Browse source 
- Extract triplicated splitKey into shared pkg/reconciler/internal package
  using strings.Cut and removing the unnecessary error return
- Fix resolver merge bug: attemptMerge was using treeA.Hash as the merge
  commit tree, silently dropping all of branch B's changes. Now builds a
  proper merged tree that applies B's non-conflicting changes onto A's tree
- Replace hand-rolled nestedFieldNoCopy/unstructuredNestedStringMap with
  k8s.io/apimachinery's unstructured.NestedStringMap stdlib equivalent
- Hoist branch listing out of per-refspec loop in updateBranches to avoid
  redundant API calls
- Add not-found handling in all three reconcilers so deleted resources
  don't cause infinite requeue loops
- Fix client.Get() to use checked type assertion with a clear panic message
  instead of an opaque nil-pointer dereference
- Remove dead code: unused addDefaultingFuncs wrapper, unused context
  injection in push controller setup

https://claude.ai/code/session_01634JRpHEoM9FzuBjZ7qHcY
This commit is contained in:
Claude 2026-02-28 04:39:00 +00:00
parent 9d5cd83734
commit 0cb9e4aa1d
No known key found for this signature in database
11 changed files with 166 additions and 409 deletions
Download patch file Download diff file Expand all files Collapse all files

14
pkg/reconciler/internal/key.go Normal file
View file

@ -0,0 +1,14 @@
package internal
import "strings"
// 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) {
namespace, name, _ = strings.Cut(key, "/")
if name == "" {
// No slash found: Cut returns (key, "", false). The key is the name.
return "", namespace
}
return namespace, name
}

51
pkg/reconciler/internal/key_test.go Normal file
View file

@ -0,0 +1,51 @@
package internal
import (
"testing"
)
func TestSplitKey(t *testing.T) {
tests := []struct {
key string
wantNS string
wantName string
}{
{
key: "default/my-resource",
wantNS: "default",
wantName: "my-resource",
},
{
key: "kube-system/controller",
wantNS: "kube-system",
wantName: "controller",
},
{
key: "no-namespace",
wantNS: "",
wantName: "no-namespace",
},
{
key: "ns/name/with/slashes",
wantNS: "ns",
wantName: "name/with/slashes",
},
{
key: "",
wantNS: "",
wantName: "",
},
}
for _, tt := range tests {
t.Run(tt.key, func(t *testing.T) {
ns, name := SplitKey(tt.key)
if ns != tt.wantNS {
t.Errorf("namespace = %q, want %q", ns, tt.wantNS)
}
if name != tt.wantName {
t.Errorf("name = %q, want %q", name, tt.wantName)
}
})
}
}

1
pkg/reconciler/push/controller.go
View file

@ -35,7 +35,6 @@ func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl
}
gitClient := gitclient.NewFromDynamic(dynClient)
ctx = gitclient.WithClient(ctx, gitClient)
// Create dynamic informer for GitPushTransaction.
factory := dynamicinformer.NewDynamicSharedInformerFactory(dynClient, 30*time.Second)

73
pkg/reconciler/push/push.go
View file

@ -11,14 +11,17 @@ 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/client-go/dynamic"
"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,13 +34,14 @@ type Reconciler struct {
func (r *Reconciler) ReconcileKind(ctx context.Context, key string) reconciler.Event {
logger := logging.FromContext(ctx)
namespace, name, err := splitKey(key)
if err != nil {
return err
}
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)
}
@ -158,7 +162,7 @@ func (r *Reconciler) resolveAuth(ctx context.Context, namespace string, repo *gi
return nil, fmt.Errorf("getting secret %q: %w", repo.Spec.Auth.SecretRef.Name, err)
}
data, found, err := unstructuredNestedStringMap(u.Object, "data")
data, found, err := unstructured.NestedStringMap(u.Object, "data")
if err != nil || !found {
return nil, fmt.Errorf("reading secret data: found=%v err=%v", found, err)
}
@ -181,42 +185,6 @@ func (r *Reconciler) resolveAuth(ctx context.Context, namespace string, repo *gi
}, nil
}
// unstructuredNestedStringMap extracts a map[string]string from an unstructured object path.
func unstructuredNestedStringMap(obj map[string]interface{}, fields ...string) (map[string]string, bool, error) {
val, found, err := nestedFieldNoCopy(obj, fields...)
if !found || err != nil {
return nil, found, err
}
m, ok := val.(map[string]interface{})
if !ok {
return nil, false, fmt.Errorf("expected map, got %T", val)
}
result := make(map[string]string, len(m))
for k, v := range m {
s, ok := v.(string)
if !ok {
continue
}
result[k] = s
}
return result, true, nil
}
func nestedFieldNoCopy(obj map[string]interface{}, fields ...string) (interface{}, bool, error) {
var val interface{} = obj
for _, field := range fields {
m, ok := val.(map[string]interface{})
if !ok {
return nil, false, nil
}
val, ok = m[field]
if !ok {
return nil, false, nil
}
}
return val, true, nil
}
// failTransaction marks a GitPushTransaction as Failed with the given message.
func (r *Reconciler) failTransaction(ctx context.Context, txn *gitv1alpha1.GitPushTransaction, message string) error {
logger := logging.FromContext(ctx)
@ -234,13 +202,14 @@ func (r *Reconciler) failTransaction(ctx context.Context, txn *gitv1alpha1.GitPu
// updateBranches updates GitBranch CRs after a successful push.
func (r *Reconciler) updateBranches(ctx context.Context, namespace string, txn *gitv1alpha1.GitPushTransaction) error {
// List branches once rather than per-refspec.
branches, err := r.gitClient.GitBranches(namespace).List(ctx, metav1.ListOptions{})
if err != nil {
return fmt.Errorf("listing branches: %w", err)
}
now := metav1.Now()
for _, rs := range txn.Spec.RefSpecs {
// List branches matching the destination ref.
branches, err := r.gitClient.GitBranches(namespace).List(ctx, metav1.ListOptions{})
if err != nil {
return fmt.Errorf("listing branches: %w", err)
}
for i := range branches.Items {
branch := &branches.Items[i]
destRef := fmt.Sprintf("refs/heads/%s", branch.Spec.BranchName)
@ -256,16 +225,6 @@ func (r *Reconciler) updateBranches(ctx context.Context, namespace string, txn *
return nil
}
// splitKey splits a namespace/name key.
func splitKey(key string) (string, string, error) {
for i := range key {
if key[i] == '/' {
return key[:i], key[i+1:], nil
}
}
return "", key, nil
}
// Ensure Reconciler implements the reconciler interface.
var _ reconciler.LeaderAware = (*Reconciler)(nil)

209
pkg/reconciler/push/push_test.go
View file

@ -5,215 +5,6 @@ import (
"testing"
)
func TestSplitKey(t *testing.T) {
tests := []struct {
key string
wantNS string
wantName string
}{
{
key: "default/my-resource",
wantNS: "default",
wantName: "my-resource",
},
{
key: "kube-system/controller",
wantNS: "kube-system",
wantName: "controller",
},
{
key: "no-namespace",
wantNS: "",
wantName: "no-namespace",
},
{
key: "ns/name/with/slashes",
wantNS: "ns",
wantName: "name/with/slashes",
},
{
key: "",
wantNS: "",
wantName: "",
},
}
for _, tt := range tests {
t.Run(tt.key, func(t *testing.T) {
ns, name, err := splitKey(tt.key)
if err != nil {
t.Fatalf("splitKey(%q) error = %v", tt.key, err)
}
if ns != tt.wantNS {
t.Errorf("namespace = %q, want %q", ns, tt.wantNS)
}
if name != tt.wantName {
t.Errorf("name = %q, want %q", name, tt.wantName)
}
})
}
}
func TestNestedFieldNoCopy(t *testing.T) {
tests := []struct {
name string
obj map[string]interface{}
fields []string
want interface{}
found bool
}{
{
name: "simple field",
obj: map[string]interface{}{
"key": "value",
},
fields: []string{"key"},
want: "value",
found: true,
},
{
name: "nested field",
obj: map[string]interface{}{
"level1": map[string]interface{}{
"level2": "deep-value",
},
},
fields: []string{"level1", "level2"},
want: "deep-value",
found: true,
},
{
name: "missing field",
obj: map[string]interface{}{
"key": "value",
},
fields: []string{"missing"},
want: nil,
found: false,
},
{
name: "non-map intermediate",
obj: map[string]interface{}{
"key": "not-a-map",
},
fields: []string{"key", "subkey"},
want: nil,
found: false,
},
{
name: "empty object",
obj: map[string]interface{}{},
fields: []string{"key"},
want: nil,
found: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, found, err := nestedFieldNoCopy(tt.obj, tt.fields...)
if err != nil {
t.Fatalf("nestedFieldNoCopy() error = %v", err)
}
if found != tt.found {
t.Errorf("found = %v, want %v", found, tt.found)
}
if found && got != tt.want {
t.Errorf("value = %v, want %v", got, tt.want)
}
})
}
}
func TestUnstructuredNestedStringMap(t *testing.T) {
tests := []struct {
name string
obj map[string]interface{}
fields []string
wantMap map[string]string
wantFound bool
wantErr bool
}{
{
name: "valid string map",
obj: map[string]interface{}{
"data": map[string]interface{}{
"username": base64.StdEncoding.EncodeToString([]byte("admin")),
"password": base64.StdEncoding.EncodeToString([]byte("secret")),
},
},
fields: []string{"data"},
wantMap: map[string]string{
"username": base64.StdEncoding.EncodeToString([]byte("admin")),
"password": base64.StdEncoding.EncodeToString([]byte("secret")),
},
wantFound: true,
},
{
name: "missing data field",
obj: map[string]interface{}{
"metadata": map[string]interface{}{},
},
fields: []string{"data"},
wantFound: false,
},
{
name: "non-string values are skipped",
obj: map[string]interface{}{
"data": map[string]interface{}{
"string-key": "string-value",
"int-key": 42,
"bool-key": true,
},
},
fields: []string{"data"},
wantMap: map[string]string{
"string-key": "string-value",
},
wantFound: true,
},
{
name: "not a map type",
obj: map[string]interface{}{
"data": "just-a-string",
},
fields: []string{"data"},
wantErr: true,
},
{
name: "empty map",
obj: map[string]interface{}{
"data": map[string]interface{}{},
},
fields: []string{"data"},
wantMap: map[string]string{},
wantFound: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, found, err := unstructuredNestedStringMap(tt.obj, tt.fields...)
if (err != nil) != tt.wantErr {
t.Fatalf("error = %v, wantErr %v", err, tt.wantErr)
}
if found != tt.wantFound {
t.Errorf("found = %v, want %v", found, tt.wantFound)
}
if tt.wantMap != nil {
if len(got) != len(tt.wantMap) {
t.Errorf("map length = %d, want %d", len(got), len(tt.wantMap))
}
for k, v := range tt.wantMap {
if got[k] != v {
t.Errorf("map[%q] = %q, want %q", k, got[k], v)
}
}
}
})
}
}
func TestBase64Decoding(t *testing.T) {
// Verify that values typically stored in Kubernetes secrets
// (base64-encoded via dynamic client) decode correctly.

95
pkg/reconciler/resolver/resolver.go
View file

@ -9,6 +9,7 @@ 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"
@ -17,6 +18,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"
)
// Reconciler implements the reconcile logic for resolving conflicted GitRepoSyncs.
@ -29,13 +31,14 @@ type Reconciler struct {
func (r *Reconciler) Reconcile(ctx context.Context, key string) error {
logger := logging.FromContext(ctx)
namespace, name, err := splitKey(key)
if err != nil {
return err
}
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)
}
@ -87,7 +90,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, key string) error {
// attemptMerge performs a 3-way merge in memory using go-git.
// It compares the trees of both commits against the merge base to detect conflicts.
// Returns the hash of the merge commit if the merge is clean.
// If there are no file-level conflicts (disjoint changes), it builds a merged tree
// that includes changes from both branches and creates a merge commit.
func (r *Reconciler) attemptMerge(ctx context.Context, repoURL, commitAHash, commitBHash, mergeBaseHash string) (string, error) {
logger := logging.FromContext(ctx)
@ -160,27 +164,29 @@ func (r *Reconciler) attemptMerge(ctx context.Context, repoURL, commitAHash, com
}
}
// No file-level conflicts. We can create a merge commit.
// Use tree A as the base (it has A's changes), and we need to apply B's changes.
// Since there are no overlapping files, we can use B's tree for non-conflicting merge.
// For a true merge, we'd reconstruct a merged tree. For this file-disjoint case,
// we create a merge commit with A's tree as the result (simplified).
// In production, we'd build a proper merged tree object.
logger.Info("No file-level conflicts detected, creating merge commit")
// No file-level conflicts. Build a merged tree that starts from A's tree
// (which already has A's changes) and applies B's changes on top.
logger.Info("No file-level conflicts detected, building merged tree")
mergedTreeHash, err := buildMergedTree(storer, treeA, treeB, diffB)
if err != nil {
return "", fmt.Errorf("building merged tree: %w", err)
}
now := time.Now()
mergeCommit := &object.Commit{
Author: object.Signature{
Name: "git-k8s-resolver",
Email: "resolver@git-k8s.imjasonh.com",
When: time.Now(),
When: now,
},
Committer: object.Signature{
Name: "git-k8s-resolver",
Email: "resolver@git-k8s.imjasonh.com",
When: time.Now(),
When: now,
},
Message: fmt.Sprintf("Merge %s and %s\n\nAutomated merge by git-k8s conflict resolver.", commitAHash[:8], commitBHash[:8]),
TreeHash: treeA.Hash,
TreeHash: mergedTreeHash,
ParentHashes: []plumbing.Hash{
hashA,
hashB,
@ -200,6 +206,56 @@ func (r *Reconciler) attemptMerge(ctx context.Context, repoURL, commitAHash, com
return mergeHash.String(), nil
}
// buildMergedTree constructs a tree that contains A's tree with B's non-conflicting
// changes applied. It processes diffB to apply additions, modifications, and deletions
// from branch B onto tree A.
func buildMergedTree(storer *memory.Storage, treeA, treeB *object.Tree, diffB object.Changes) (plumbing.Hash, error) {
// Start with all entries from tree A.
entries := make(map[string]object.TreeEntry)
for _, entry := range treeA.Entries {
entries[entry.Name] = entry
}
// Apply B's changes.
for _, change := range diffB {
if change.To.Name != "" {
// File added or modified in B: use B's version.
entry, err := treeB.FindEntry(change.To.Name)
if err != nil {
return plumbing.ZeroHash, fmt.Errorf("finding entry %q in tree B: %w", change.To.Name, err)
}
entries[change.To.Name] = *entry
}
if change.From.Name != "" && change.To.Name == "" {
// File deleted in B: remove from merged tree.
delete(entries, change.From.Name)
}
if change.From.Name != "" && change.To.Name != "" && change.From.Name != change.To.Name {
// File renamed in B: remove old name.
delete(entries, change.From.Name)
}
}
// Build sorted tree entries.
sortedEntries := make([]object.TreeEntry, 0, len(entries))
for _, entry := range entries {
sortedEntries = append(sortedEntries, entry)
}
mergedTree := &object.Tree{Entries: sortedEntries}
encodedObj := storer.NewEncodedObject()
if err := mergedTree.Encode(encodedObj); err != nil {
return plumbing.ZeroHash, fmt.Errorf("encoding merged tree: %w", err)
}
hash, err := storer.SetEncodedObject(encodedObj)
if err != nil {
return plumbing.ZeroHash, fmt.Errorf("storing merged tree: %w", err)
}
return hash, nil
}
// changeName extracts the file name from a tree change.
func changeName(change *object.Change) string {
if change.From.Name != "" {
@ -293,15 +349,6 @@ func (r *Reconciler) markManualIntervention(ctx context.Context, syncObj *gitv1a
return err
}
func splitKey(key string) (string, string, error) {
for i := range key {
if key[i] == '/' {
return key[:i], key[i+1:], nil
}
}
return "", key, nil
}
var _ reconciler.LeaderAware = (*Reconciler)(nil)
func (r *Reconciler) Promote(bkt reconciler.Bucket, enq func(reconciler.Bucket, types.NamespacedName)) error {

44
pkg/reconciler/resolver/resolver_test.go
View file

@ -6,50 +6,6 @@ import (
"github.com/go-git/go-git/v5/plumbing/object"
)
func TestSplitKey(t *testing.T) {
tests := []struct {
key string
wantNS string
wantName string
}{
{
key: "default/my-sync",
wantNS: "default",
wantName: "my-sync",
},
{
key: "git-system/resolver",
wantNS: "git-system",
wantName: "resolver",
},
{
key: "just-name",
wantNS: "",
wantName: "just-name",
},
{
key: "",
wantNS: "",
wantName: "",
},
}
for _, tt := range tests {
t.Run(tt.key, func(t *testing.T) {
ns, name, err := splitKey(tt.key)
if err != nil {
t.Fatalf("splitKey(%q) error = %v", tt.key, err)
}
if ns != tt.wantNS {
t.Errorf("namespace = %q, want %q", ns, tt.wantNS)
}
if name != tt.wantName {
t.Errorf("name = %q, want %q", name, tt.wantName)
}
})
}
}
func TestChangeName(t *testing.T) {
tests := []struct {
name string

22
pkg/reconciler/sync/sync.go
View file

@ -3,9 +3,11 @@ package sync
import (
"context"
"fmt"
"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"
@ -14,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"
)
// Reconciler implements the reconcile logic for GitRepoSync.
@ -26,13 +29,14 @@ type Reconciler struct {
func (r *Reconciler) Reconcile(ctx context.Context, key string) error {
logger := logging.FromContext(ctx)
namespace, name, err := splitKey(key)
if err != nil {
return err
}
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)
}
@ -208,15 +212,6 @@ func (r *Reconciler) updateSyncStatus(ctx context.Context, syncObj *gitv1alpha1.
return err
}
func splitKey(key string) (string, string, error) {
for i := range key {
if key[i] == '/' {
return key[:i], key[i+1:], nil
}
}
return "", key, nil
}
// Ensure Reconciler implements the reconciler interface.
var _ reconciler.LeaderAware = (*Reconciler)(nil)
@ -226,4 +221,3 @@ func (r *Reconciler) Promote(bkt reconciler.Bucket, enq func(reconciler.Bucket,
func (r *Reconciler) Demote(bkt reconciler.Bucket) {
}

53
pkg/reconciler/sync/sync_test.go
View file

@ -1,54 +1 @@
package sync
import (
"testing"
)
func TestSplitKey(t *testing.T) {
tests := []struct {
key string
wantNS string
wantName string
}{
{
key: "default/my-sync",
wantNS: "default",
wantName: "my-sync",
},
{
key: "git-system/sync-controller",
wantNS: "git-system",
wantName: "sync-controller",
},
{
key: "just-name",
wantNS: "",
wantName: "just-name",
},
{
key: "",
wantNS: "",
wantName: "",
},
{
key: "ns/name/extra",
wantNS: "ns",
wantName: "name/extra",
},
}
for _, tt := range tests {
t.Run(tt.key, func(t *testing.T) {
ns, name, err := splitKey(tt.key)
if err != nil {
t.Fatalf("splitKey(%q) error = %v", tt.key, err)
}
if ns != tt.wantNS {
t.Errorf("namespace = %q, want %q", ns, tt.wantNS)
}
if name != tt.wantName {
t.Errorf("name = %q, want %q", name, tt.wantName)
}
})
}
}