Provisioning: unit test and bug fixes go-git repository (#104390)

* Add unit test for unimplemented methods

* Add unit test for GoGitRepo_Read

* Add tests for Delete

* Add more tests

* Add unit test for GoGitRepo_Push

* Add unit test for ReadTree

pkg/registry/apis/provisioning/repository/go-git/repository_mock.go

@@ -0,0 +1,84 @@
+// Code generated by mockery v2.52.4. DO NOT EDIT.
+
+package gogit
+
+import (
+	context "context"
+
+	git "github.com/go-git/go-git/v5"
+	mock "github.com/stretchr/testify/mock"
+)
+
+// MockRepository is an autogenerated mock type for the Repository type
+type MockRepository struct {
+	mock.Mock
+}
+
+type MockRepository_Expecter struct {
+	mock *mock.Mock
+}
+
+func (_m *MockRepository) EXPECT() *MockRepository_Expecter {
+	return &MockRepository_Expecter{mock: &_m.Mock}
+}
+
+// PushContext provides a mock function with given fields: ctx, o
+func (_m *MockRepository) PushContext(ctx context.Context, o *git.PushOptions) error {
+	ret := _m.Called(ctx, o)
+
+	if len(ret) == 0 {
+		panic("no return value specified for PushContext")
+	}
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(context.Context, *git.PushOptions) error); ok {
+		r0 = rf(ctx, o)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
+// MockRepository_PushContext_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'PushContext'
+type MockRepository_PushContext_Call struct {
+	*mock.Call
+}
+
+// PushContext is a helper method to define mock.On call
+//   - ctx context.Context
+//   - o *git.PushOptions
+func (_e *MockRepository_Expecter) PushContext(ctx interface{}, o interface{}) *MockRepository_PushContext_Call {
+	return &MockRepository_PushContext_Call{Call: _e.mock.On("PushContext", ctx, o)}
+}
+
+func (_c *MockRepository_PushContext_Call) Run(run func(ctx context.Context, o *git.PushOptions)) *MockRepository_PushContext_Call {
+	_c.Call.Run(func(args mock.Arguments) {
+		run(args[0].(context.Context), args[1].(*git.PushOptions))
+	})
+	return _c
+}
+
+func (_c *MockRepository_PushContext_Call) Return(_a0 error) *MockRepository_PushContext_Call {
+	_c.Call.Return(_a0)
+	return _c
+}
+
+func (_c *MockRepository_PushContext_Call) RunAndReturn(run func(context.Context, *git.PushOptions) error) *MockRepository_PushContext_Call {
+	_c.Call.Return(run)
+	return _c
+}
+
+// NewMockRepository creates a new instance of MockRepository. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations.
+// The first argument is typically a *testing.T value.
+func NewMockRepository(t interface {
+	mock.TestingT
+	Cleanup(func())
+}) *MockRepository {
+	mock := &MockRepository{}
+	mock.Mock.Test(t)
+
+	t.Cleanup(func() { mock.AssertExpectations(t) })
+
+	return mock
+}

pkg/registry/apis/provisioning/repository/go-git/worktree_mock.go

@@ -0,0 +1,261 @@
+// Code generated by mockery v2.52.4. DO NOT EDIT.
+
+package gogit
+
+import (
+	billy "github.com/go-git/go-billy/v5"
+	git "github.com/go-git/go-git/v5"
+
+	mock "github.com/stretchr/testify/mock"
+
+	plumbing "github.com/go-git/go-git/v5/plumbing"
+)
+
+// MockWorktree is an autogenerated mock type for the Worktree type
+type MockWorktree struct {
+	mock.Mock
+}
+
+type MockWorktree_Expecter struct {
+	mock *mock.Mock
+}
+
+func (_m *MockWorktree) EXPECT() *MockWorktree_Expecter {
+	return &MockWorktree_Expecter{mock: &_m.Mock}
+}
+
+// Add provides a mock function with given fields: path
+func (_m *MockWorktree) Add(path string) (plumbing.Hash, error) {
+	ret := _m.Called(path)
+
+	if len(ret) == 0 {
+		panic("no return value specified for Add")
+	}
+
+	var r0 plumbing.Hash
+	var r1 error
+	if rf, ok := ret.Get(0).(func(string) (plumbing.Hash, error)); ok {
+		return rf(path)
+	}
+	if rf, ok := ret.Get(0).(func(string) plumbing.Hash); ok {
+		r0 = rf(path)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(plumbing.Hash)
+		}
+	}
+
+	if rf, ok := ret.Get(1).(func(string) error); ok {
+		r1 = rf(path)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
+// MockWorktree_Add_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Add'
+type MockWorktree_Add_Call struct {
+	*mock.Call
+}
+
+// Add is a helper method to define mock.On call
+//   - path string
+func (_e *MockWorktree_Expecter) Add(path interface{}) *MockWorktree_Add_Call {
+	return &MockWorktree_Add_Call{Call: _e.mock.On("Add", path)}
+}
+
+func (_c *MockWorktree_Add_Call) Run(run func(path string)) *MockWorktree_Add_Call {
+	_c.Call.Run(func(args mock.Arguments) {
+		run(args[0].(string))
+	})
+	return _c
+}
+
+func (_c *MockWorktree_Add_Call) Return(_a0 plumbing.Hash, _a1 error) *MockWorktree_Add_Call {
+	_c.Call.Return(_a0, _a1)
+	return _c
+}
+
+func (_c *MockWorktree_Add_Call) RunAndReturn(run func(string) (plumbing.Hash, error)) *MockWorktree_Add_Call {
+	_c.Call.Return(run)
+	return _c
+}
+
+// Commit provides a mock function with given fields: message, opts
+func (_m *MockWorktree) Commit(message string, opts *git.CommitOptions) (plumbing.Hash, error) {
+	ret := _m.Called(message, opts)
+
+	if len(ret) == 0 {
+		panic("no return value specified for Commit")
+	}
+
+	var r0 plumbing.Hash
+	var r1 error
+	if rf, ok := ret.Get(0).(func(string, *git.CommitOptions) (plumbing.Hash, error)); ok {
+		return rf(message, opts)
+	}
+	if rf, ok := ret.Get(0).(func(string, *git.CommitOptions) plumbing.Hash); ok {
+		r0 = rf(message, opts)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(plumbing.Hash)
+		}
+	}
+
+	if rf, ok := ret.Get(1).(func(string, *git.CommitOptions) error); ok {
+		r1 = rf(message, opts)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
+// MockWorktree_Commit_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Commit'
+type MockWorktree_Commit_Call struct {
+	*mock.Call
+}
+
+// Commit is a helper method to define mock.On call
+//   - message string
+//   - opts *git.CommitOptions
+func (_e *MockWorktree_Expecter) Commit(message interface{}, opts interface{}) *MockWorktree_Commit_Call {
+	return &MockWorktree_Commit_Call{Call: _e.mock.On("Commit", message, opts)}
+}
+
+func (_c *MockWorktree_Commit_Call) Run(run func(message string, opts *git.CommitOptions)) *MockWorktree_Commit_Call {
+	_c.Call.Run(func(args mock.Arguments) {
+		run(args[0].(string), args[1].(*git.CommitOptions))
+	})
+	return _c
+}
+
+func (_c *MockWorktree_Commit_Call) Return(_a0 plumbing.Hash, _a1 error) *MockWorktree_Commit_Call {
+	_c.Call.Return(_a0, _a1)
+	return _c
+}
+
+func (_c *MockWorktree_Commit_Call) RunAndReturn(run func(string, *git.CommitOptions) (plumbing.Hash, error)) *MockWorktree_Commit_Call {
+	_c.Call.Return(run)
+	return _c
+}
+
+// Filesystem provides a mock function with no fields
+func (_m *MockWorktree) Filesystem() billy.Filesystem {
+	ret := _m.Called()
+
+	if len(ret) == 0 {
+		panic("no return value specified for Filesystem")
+	}
+
+	var r0 billy.Filesystem
+	if rf, ok := ret.Get(0).(func() billy.Filesystem); ok {
+		r0 = rf()
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(billy.Filesystem)
+		}
+	}
+
+	return r0
+}
+
+// MockWorktree_Filesystem_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Filesystem'
+type MockWorktree_Filesystem_Call struct {
+	*mock.Call
+}
+
+// Filesystem is a helper method to define mock.On call
+func (_e *MockWorktree_Expecter) Filesystem() *MockWorktree_Filesystem_Call {
+	return &MockWorktree_Filesystem_Call{Call: _e.mock.On("Filesystem")}
+}
+
+func (_c *MockWorktree_Filesystem_Call) Run(run func()) *MockWorktree_Filesystem_Call {
+	_c.Call.Run(func(args mock.Arguments) {
+		run()
+	})
+	return _c
+}
+
+func (_c *MockWorktree_Filesystem_Call) Return(_a0 billy.Filesystem) *MockWorktree_Filesystem_Call {
+	_c.Call.Return(_a0)
+	return _c
+}
+
+func (_c *MockWorktree_Filesystem_Call) RunAndReturn(run func() billy.Filesystem) *MockWorktree_Filesystem_Call {
+	_c.Call.Return(run)
+	return _c
+}
+
+// Remove provides a mock function with given fields: path
+func (_m *MockWorktree) Remove(path string) (plumbing.Hash, error) {
+	ret := _m.Called(path)
+
+	if len(ret) == 0 {
+		panic("no return value specified for Remove")
+	}
+
+	var r0 plumbing.Hash
+	var r1 error
+	if rf, ok := ret.Get(0).(func(string) (plumbing.Hash, error)); ok {
+		return rf(path)
+	}
+	if rf, ok := ret.Get(0).(func(string) plumbing.Hash); ok {
+		r0 = rf(path)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(plumbing.Hash)
+		}
+	}
+
+	if rf, ok := ret.Get(1).(func(string) error); ok {
+		r1 = rf(path)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
+// MockWorktree_Remove_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Remove'
+type MockWorktree_Remove_Call struct {
+	*mock.Call
+}
+
+// Remove is a helper method to define mock.On call
+//   - path string
+func (_e *MockWorktree_Expecter) Remove(path interface{}) *MockWorktree_Remove_Call {
+	return &MockWorktree_Remove_Call{Call: _e.mock.On("Remove", path)}
+}
+
+func (_c *MockWorktree_Remove_Call) Run(run func(path string)) *MockWorktree_Remove_Call {
+	_c.Call.Run(func(args mock.Arguments) {
+		run(args[0].(string))
+	})
+	return _c
+}
+
+func (_c *MockWorktree_Remove_Call) Return(_a0 plumbing.Hash, _a1 error) *MockWorktree_Remove_Call {
+	_c.Call.Return(_a0, _a1)
+	return _c
+}
+
+func (_c *MockWorktree_Remove_Call) RunAndReturn(run func(string) (plumbing.Hash, error)) *MockWorktree_Remove_Call {
+	_c.Call.Return(run)
+	return _c
+}
+
+// NewMockWorktree creates a new instance of MockWorktree. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations.
+// The first argument is typically a *testing.T value.
+func NewMockWorktree(t interface {
+	mock.TestingT
+	Cleanup(func())
+}) *MockWorktree {
+	mock := &MockWorktree{}
+	mock.Mock.Test(t)
+
+	t.Cleanup(func() { mock.AssertExpectations(t) })
+
+	return mock
+}

pkg/registry/apis/provisioning/repository/go-git/wrapper.go

@@ -11,6 +11,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/go-git/go-billy/v5"
 	"github.com/go-git/go-billy/v5/util"
 	"github.com/go-git/go-git/v5"
 	"github.com/go-git/go-git/v5/plumbing"
@@ -43,6 +44,27 @@ func init() {
 	client.InstallProtocol("http", httpClient)
 }
 
+//go:generate mockery --name=Worktree --output=mocks --inpackage --filename=worktree_mock.go --with-expecter
+type Worktree interface {
+	Commit(message string, opts *git.CommitOptions) (plumbing.Hash, error)
+	Remove(path string) (plumbing.Hash, error)
+	Add(path string) (plumbing.Hash, error)
+	Filesystem() billy.Filesystem
+}
+
+type worktree struct {
+	*git.Worktree
+}
+
+//go:generate mockery --name=Repository --output=mocks --inpackage --filename=repository_mock.go --with-expecter
+type Repository interface {
+	PushContext(ctx context.Context, o *git.PushOptions) error
+}
+
+func (w *worktree) Filesystem() billy.Filesystem {
+	return w.Worktree.Filesystem
+}
+
 var _ repository.Repository = (*GoGitRepo)(nil)
 
 type GoGitRepo struct {
@@ -50,8 +72,8 @@ type GoGitRepo struct {
 	decryptedPassword string
 	opts              repository.CloneOptions
 
-	repo *git.Repository
-	tree *git.Worktree
+	repo Repository
+	tree Worktree
 	dir  string // file path to worktree root (necessary? should use billy)
 }
 
@@ -101,7 +123,7 @@ func Clone(
 		progress = io.Discard
 	}
 
-	repo, worktree, err := clone(ctx, config, opts, decrypted, dir, progress)
+	repo, tree, err := clone(ctx, config, opts, decrypted, dir, progress)
 	if err != nil {
 		if err := os.RemoveAll(dir); err != nil {
 			return nil, fmt.Errorf("remove temp clone dir after clone failed: %w", err)
@@ -112,7 +134,7 @@ func Clone(
 
 	return &GoGitRepo{
 		config:            config,
-		tree:              worktree,
+		tree:              &worktree{Worktree: tree},
 		opts:              opts,
 		decryptedPassword: string(decrypted),
 		repo:              repo,
@@ -254,13 +276,13 @@ func (g *GoGitRepo) ReadTree(ctx context.Context, ref string) ([]repository.File
 	treePath = safepath.Clean(treePath)
 
 	entries := make([]repository.FileTreeEntry, 0, 100)
-	err := util.Walk(g.tree.Filesystem, treePath, func(path string, info fs.FileInfo, err error) error {
+	err := util.Walk(g.tree.Filesystem(), treePath, func(path string, info fs.FileInfo, err error) error {
 		// We already have an error, just pass it onwards.
 		if err != nil ||
 			// This is the root of the repository (or should pretend to be)
 			safepath.Clean(path) == "" || path == treePath ||
 			// This is the Git data
-			(treePath == "" && strings.HasPrefix(path, ".git/")) {
+			(treePath == "" && (strings.HasPrefix(path, ".git/") || path == ".git")) {
 			return err
 		}
 		if treePath != "" {
@@ -280,9 +302,9 @@ func (g *GoGitRepo) ReadTree(ctx context.Context, ref string) ([]repository.File
 		return err
 	})
 	if errors.Is(err, fs.ErrNotExist) {
-		// We intentionally ignore this case, as
+		// We intentionally ignore this case, as it is expected
 	} else if err != nil {
-		return nil, fmt.Errorf("failed to walk tree for ref '%s': %w", ref, err)
+		return nil, fmt.Errorf("walk tree for ref '%s': %w", ref, err)
 	}
 	return entries, nil
 }
@@ -300,30 +322,33 @@ func (g *GoGitRepo) Update(ctx context.Context, path string, ref string, data []
 
 // Create implements repository.Repository.
 func (g *GoGitRepo) Create(ctx context.Context, path string, ref string, data []byte, message string) error {
+	// FIXME: this means we would override files
 	return g.Write(ctx, path, ref, data, message)
 }
 
 // Write implements repository.Repository.
 func (g *GoGitRepo) Write(ctx context.Context, fpath string, ref string, data []byte, message string) error {
-	fpath = safepath.Join(g.config.Spec.GitHub.Path, fpath)
 	if err := verifyPathWithoutRef(fpath, ref); err != nil {
 		return err
 	}
+	fpath = safepath.Join(g.config.Spec.GitHub.Path, fpath)
 
+	// FIXME: this means that won't export empty folders
+	// should we create them with a .keep file?
 	// For folders, just create the folder and ignore the commit
 	if safepath.IsDir(fpath) {
-		return g.tree.Filesystem.MkdirAll(fpath, 0750)
+		return g.tree.Filesystem().MkdirAll(fpath, 0750)
 	}
 
 	dir := safepath.Dir(fpath)
 	if dir != "" {
-		err := g.tree.Filesystem.MkdirAll(dir, 0750)
+		err := g.tree.Filesystem().MkdirAll(dir, 0750)
 		if err != nil {
 			return err
 		}
 	}
 
-	file, err := g.tree.Filesystem.Create(fpath)
+	file, err := g.tree.Filesystem().Create(fpath)
 	if err != nil {
 		return err
 	}
@@ -363,11 +388,16 @@ func (g *GoGitRepo) maybeCommit(ctx context.Context, message string) error {
 
 // Delete implements repository.Repository.
 func (g *GoGitRepo) Delete(ctx context.Context, fpath string, ref string, message string) error {
-	fpath = safepath.Join(g.config.Spec.GitHub.Path, fpath)
 	if err := verifyPathWithoutRef(fpath, ref); err != nil {
 		return err
 	}
+
+	fpath = safepath.Join(g.config.Spec.GitHub.Path, fpath)
 	if _, err := g.tree.Remove(fpath); err != nil {
+		if errors.Is(err, fs.ErrNotExist) {
+			return repository.ErrFileNotFound
+		}
+
 		return err
 	}
 	return g.maybeCommit(ctx, message)
@@ -379,11 +409,11 @@ func (g *GoGitRepo) Read(ctx context.Context, path string, ref string) (*reposit
 		return nil, err
 	}
 	readPath := safepath.Join(g.config.Spec.GitHub.Path, path)
-	stat, err := g.tree.Filesystem.Lstat(readPath)
+	stat, err := g.tree.Filesystem().Lstat(readPath)
 	if errors.Is(err, fs.ErrNotExist) {
 		return nil, repository.ErrFileNotFound
 	} else if err != nil {
-		return nil, fmt.Errorf("failed to stat path '%s': %w", readPath, err)
+		return nil, fmt.Errorf("stat path '%s': %w", readPath, err)
 	}
 	info := &repository.FileInfo{
 		Path: path,
@@ -392,13 +422,13 @@ func (g *GoGitRepo) Read(ctx context.Context, path string, ref string) (*reposit
 		},
 	}
 	if !stat.IsDir() {
-		f, err := g.tree.Filesystem.Open(readPath)
+		f, err := g.tree.Filesystem().Open(readPath)
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("open file '%s': %w", readPath, err)
 		}
 		info.Data, err = io.ReadAll(f)
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("read file '%s': %w", readPath, err)
 		}
 	}
 	return info, err