Skip to content
This repository was archived by the owner on Jul 18, 2025. It is now read-only.

Commit c3944e3

Browse files
committed
adding more refactoring and unit tests
1 parent ed0c1cd commit c3944e3

File tree

4 files changed

+66
-168
lines changed

4 files changed

+66
-168
lines changed

Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,7 @@ CAPI_KIND_CLUSTER_NAME ?= capi-test
231231
# It is set by Prow GIT_TAG, a git-based tag of the form vYYYYMMDD-hash, e.g., v20210120-v0.3.10-308-gc61521971
232232

233233
# Next release is: v1.0.0-preview
234-
TAG ?= v1.0.0-preview
234+
TAG ?= v1.0.0-preview.1
235235
ARCH ?= $(shell go env GOARCH)
236236
ALL_ARCH = amd64 arm arm64
237237

controllers/cdk8sappproxy/cdk8sappproxy_git_operator.go

Lines changed: 9 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -2,66 +2,39 @@ package cdk8sappproxy
22

33
import (
44
"bytes"
5-
"context"
65
addonsv1alpha1 "github.com/PatrickLaabs/cluster-api-addon-provider-cdk8s/api/v1alpha1"
76
gitoperator "github.com/PatrickLaabs/cluster-api-addon-provider-cdk8s/controllers/cdk8sappproxy/git"
87
"github.com/go-logr/logr"
9-
"github.com/pkg/errors"
10-
"k8s.io/apimachinery/pkg/types"
11-
"os"
128
"path/filepath"
139
)
1410

15-
func (r *Reconciler) prepareSource(ctx context.Context, cdk8sAppProxy *addonsv1alpha1.Cdk8sAppProxy, proxyNamespacedName types.NamespacedName, logger logr.Logger, operation string) (appSourcePath string, currentCommitHash string, err error) {
11+
func (r *Reconciler) prepareSource(cdk8sAppProxy *addonsv1alpha1.Cdk8sAppProxy, logger logr.Logger, operation string) (appSourcePath string, currentCommitHash string, err error) {
1612
gitImpl := &gitoperator.GitImplementer{}
1713
var buf bytes.Buffer
1814
gitSpec := cdk8sAppProxy.Spec.GitRepository
1915

20-
switch {
21-
case cdk8sAppProxy.Spec.GitRepository != nil && cdk8sAppProxy.Spec.GitRepository.URL != "":
22-
logger.Info("Determining git repository URL.")
16+
if cdk8sAppProxy.Spec.GitRepository != nil && cdk8sAppProxy.Spec.GitRepository.URL != "" {
2317

24-
tempDirPattern := "cdk8s-git-clone-"
25-
if operation == OperationDeletion {
26-
tempDirPattern = "cdk8s-git-delete-"
27-
}
28-
tempDir, err := os.MkdirTemp("", tempDirPattern)
29-
if err != nil {
30-
return "", "", err
31-
}
32-
33-
err = gitImpl.Clone(gitSpec.URL, tempDir, &buf)
18+
directory, err := gitImpl.Clone(gitSpec.URL, &buf)
3419
if err != nil {
3520
logger.Error(err, addonsv1alpha1.GitCloneFailedCondition, "Failed to clone git repository")
3621
}
3722

38-
retrieveCommitHash, err := gitImpl.Hash(tempDir, cdk8sAppProxy.Spec.GitRepository.Reference)
23+
retrieveCommitHash, err := gitImpl.Hash(directory, cdk8sAppProxy.Spec.GitRepository.Reference)
3924
if err != nil {
4025
logger.Error(err, addonsv1alpha1.GitHashFailureReason, "Failed to get local git hash")
4126
}
4227

43-
if operation == OperationNormal {
44-
currentCommitHash = retrieveCommitHash
45-
if currentCommitHash != "" && cdk8sAppProxy != nil {
46-
cdk8sAppProxy.Status.LastRemoteGitHash = currentCommitHash
47-
logger.Info("Updated cdk8sAppProxy.Status.LastRemoteGitHash with the latest commit hash from remote", "lastRemoteGitHash", currentCommitHash)
48-
}
28+
currentCommitHash = retrieveCommitHash
29+
if currentCommitHash != "" && cdk8sAppProxy != nil {
30+
cdk8sAppProxy.Status.LastRemoteGitHash = currentCommitHash
31+
logger.Info("Updated cdk8sAppProxy.Status.LastRemoteGitHash with the latest commit hash from remote", "lastRemoteGitHash", currentCommitHash)
4932
}
5033

51-
appSourcePath = tempDir
5234
if gitSpec.Path != "" {
53-
appSourcePath = filepath.Join(tempDir, gitSpec.Path)
35+
appSourcePath = filepath.Join(directory, gitSpec.Path)
5436
logger.Info("Adjusted appSourcePath for repository subpath", "subPath", gitSpec.Path, "finalPath", appSourcePath, "operation", operation)
5537
}
56-
default:
57-
err := errors.New("no source specified")
58-
if operation == OperationNormal {
59-
if cdk8sAppProxy != nil {
60-
_ = r.updateStatusWithError(ctx, cdk8sAppProxy, addonsv1alpha1.EmptyGitRepositoryReason, "No source specified during "+operation, err, false)
61-
}
62-
} else if operation == OperationDeletion && cdk8sAppProxy != nil {
63-
_ = r.updateStatusWithError(ctx, cdk8sAppProxy, addonsv1alpha1.EmptyGitRepositoryReason, "No source specified, cannot determine resources to delete during "+operation, err, true)
64-
}
6538

6639
return appSourcePath, currentCommitHash, err
6740
}

controllers/cdk8sappproxy/git/git.go

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,12 @@ import (
88
"github.com/go-git/go-git/v5/config"
99
"github.com/go-git/go-git/v5/plumbing"
1010
"net/url"
11+
"os"
1112
)
1213

1314
// GitOperator defines the interface for git operations.
1415
type GitOperator interface {
15-
Clone(repoUrl string, directory string, writer *bytes.Buffer) (err error)
16+
Clone(repoUrl string, writer *bytes.Buffer) (directory string, err error)
1617
Poll(repo string, branch string, directory string, writer *bytes.Buffer) (changes bool, err error)
1718
Hash(repo string, branch string) (hash string, err error)
1819
}
@@ -21,12 +22,19 @@ type GitOperator interface {
2122
type GitImplementer struct{}
2223

2324
// Clone clones the given repository to a local directory.
24-
func (g *GitImplementer) Clone(repoUrl string, directory string, writer *bytes.Buffer) (err error) {
25+
func (g *GitImplementer) Clone(repoUrl string, writer *bytes.Buffer) (directory string, err error) {
26+
tempDirPattern := "cdk8s-git-clone-"
27+
28+
directory, err = os.MkdirTemp("", tempDirPattern)
29+
if err != nil {
30+
return directory, fmt.Errorf("failed to create temporary directory: %v", err)
31+
}
32+
2533
// Check if repo and directory are empty.
2634
if empty(repoUrl, directory) {
2735
fmt.Fprintf(writer, "%s", addonsv1alpha1.EmptyGitRepositoryReason)
2836

29-
return fmt.Errorf("%s", addonsv1alpha1.EmptyGitRepositoryReason)
37+
return directory, fmt.Errorf("%s", addonsv1alpha1.EmptyGitRepositoryReason)
3038
}
3139

3240
_, err = git.PlainClone(directory, false, &git.CloneOptions{
@@ -35,10 +43,10 @@ func (g *GitImplementer) Clone(repoUrl string, directory string, writer *bytes.B
3543
if err != nil {
3644
fmt.Fprintf(writer, addonsv1alpha1.GitCloneFailedCondition)
3745

38-
return err
46+
return directory, err
3947
}
4048

41-
return err
49+
return directory, err
4250
}
4351

4452
// Poll polls for changes for the given remote git repository. Returns true, if current local commit hash and remote hash are not equal.

controllers/cdk8sappproxy/git/git_test.go

Lines changed: 43 additions & 126 deletions
Original file line numberDiff line numberDiff line change
@@ -21,148 +21,66 @@ var (
2121
func TestClone(t *testing.T) {
2222
gitImplementer := &GitImplementer{}
2323

24-
t.Run("empty parameters - both empty", func(t *testing.T) {
25-
buffer := &bytes.Buffer{}
26-
27-
err := gitImplementer.Clone("", "", buffer)
28-
if err == nil {
29-
t.Error("Clone should have returned error for empty parameters")
30-
}
31-
expectedMessage := addonsv1alpha1.EmptyGitRepositoryReason
32-
if buffer.String() != expectedMessage {
33-
t.Errorf("Buffer should contain %q, got %q", expectedMessage, buffer.String())
34-
}
35-
if err.Error() != addonsv1alpha1.EmptyGitRepositoryReason {
36-
t.Errorf("Error message should be %q, got %q", addonsv1alpha1.EmptyGitRepositoryReason, err.Error())
37-
}
38-
})
39-
4024
t.Run("empty parameters - repo empty", func(t *testing.T) {
4125
buffer := &bytes.Buffer{}
42-
tempDir := t.TempDir()
4326

44-
err := gitImplementer.Clone("", tempDir, buffer)
27+
directory, err := gitImplementer.Clone("", buffer)
4528
if err == nil {
4629
t.Error("Clone should have returned error for empty repo")
4730
}
48-
expectedMessage := addonsv1alpha1.EmptyGitRepositoryReason
49-
if buffer.String() != expectedMessage {
50-
t.Errorf("Buffer should contain %q, got %q", expectedMessage, buffer.String())
51-
}
52-
})
53-
54-
t.Run("empty parameters - directory empty", func(t *testing.T) {
55-
buffer := &bytes.Buffer{}
56-
57-
err := gitImplementer.Clone(validRepoUrl, "", buffer)
58-
if err == nil {
59-
t.Error("Clone should have returned error for empty directory")
31+
// Directory should still be created even on error
32+
if directory == "" {
33+
t.Error("Directory should be returned even on error")
6034
}
6135
expectedMessage := addonsv1alpha1.EmptyGitRepositoryReason
6236
if buffer.String() != expectedMessage {
6337
t.Errorf("Buffer should contain %q, got %q", expectedMessage, buffer.String())
6438
}
39+
// Clean up created directory
40+
if directory != "" {
41+
os.RemoveAll(directory)
42+
}
6543
})
6644

6745
t.Run("invalid repository URL", func(t *testing.T) {
6846
buffer := &bytes.Buffer{}
69-
tempDir := t.TempDir()
7047

71-
err := gitImplementer.Clone(invalidRepoUrl, tempDir, buffer)
48+
directory, err := gitImplementer.Clone(invalidRepoUrl, buffer)
7249
if err == nil {
7350
t.Error("Clone should have returned error for invalid repository URL")
7451
}
7552
expectedMessage := addonsv1alpha1.GitCloneFailedCondition
7653
if buffer.String() != expectedMessage {
7754
t.Errorf("Buffer should contain %q, got %q", expectedMessage, buffer.String())
7855
}
56+
// Clean up created directory
57+
if directory != "" {
58+
os.RemoveAll(directory)
59+
}
7960
})
8061

8162
t.Run("malformed URL", func(t *testing.T) {
8263
buffer := &bytes.Buffer{}
83-
tempDir := t.TempDir()
8464
malformedURL := "not-a-valid-url"
8565

86-
err := gitImplementer.Clone(malformedURL, tempDir, buffer)
66+
directory, err := gitImplementer.Clone(malformedURL, buffer)
8767
if err == nil {
8868
t.Error("Clone should have returned error for malformed URL")
8969
}
9070
expectedMessage := addonsv1alpha1.GitCloneFailedCondition
9171
if buffer.String() != expectedMessage {
9272
t.Errorf("Buffer should contain %q, got %q", expectedMessage, buffer.String())
9373
}
94-
})
95-
96-
t.Run("directory is a file not directory", func(t *testing.T) {
97-
buffer := &bytes.Buffer{}
98-
99-
// Create a temporary file instead of directory
100-
tempFile, err := os.CreateTemp("", "not-a-directory")
101-
if err != nil {
102-
t.Fatalf("Failed to create temp file: %v", err)
103-
}
104-
defer os.Remove(tempFile.Name())
105-
tempFile.Close()
106-
107-
err = gitImplementer.Clone(validRepoUrl, tempFile.Name(), buffer)
108-
if err == nil {
109-
t.Error("Clone should have returned error when directory is actually a file")
110-
}
111-
expectedMessage := addonsv1alpha1.GitCloneFailedCondition
112-
if buffer.String() != expectedMessage {
113-
t.Errorf("Buffer should contain %q, got %q", expectedMessage, buffer.String())
114-
}
115-
})
116-
117-
t.Run("directory already exists and is not empty", func(t *testing.T) {
118-
buffer := &bytes.Buffer{}
119-
tempDir := t.TempDir()
120-
121-
// Create a file in the directory to make it non-empty
122-
testFile := filepath.Join(tempDir, "existing-file.txt")
123-
if err := os.WriteFile(testFile, []byte("existing content"), 0644); err != nil {
124-
t.Fatalf("Failed to create test file: %v", err)
125-
}
126-
127-
err := gitImplementer.Clone(validRepoUrl, tempDir, buffer)
128-
129-
// git.PlainClone will fail if directory is not empty, but the exact behavior
130-
// depends on the git library version and network conditions
131-
if err != nil {
132-
// Expected behavior - clone should fail for non-empty directory
133-
expectedMessage := addonsv1alpha1.GitCloneFailedCondition
134-
if buffer.String() != expectedMessage {
135-
t.Errorf("Buffer should contain %q, got %q", expectedMessage, buffer.String())
136-
}
137-
} else {
138-
// If clone somehow succeeds, just log it (network-dependent test)
139-
t.Logf("Clone unexpectedly succeeded for non-empty directory - this may be network/environment dependent")
140-
}
141-
})
142-
143-
t.Run("permission denied directory", func(t *testing.T) {
144-
buffer := &bytes.Buffer{}
145-
146-
// Try to clone to a directory with restricted permissions
147-
restrictedDir := "/root/restricted-clone" // Assuming test doesn't run as root
148-
149-
err := gitImplementer.Clone(validRepoUrl, restrictedDir, buffer)
150-
if err == nil {
151-
t.Error("Clone should have returned error for permission denied directory")
152-
}
153-
expectedMessage := addonsv1alpha1.GitCloneFailedCondition
154-
if buffer.String() != expectedMessage {
155-
t.Errorf("Buffer should contain %q, got %q", expectedMessage, buffer.String())
74+
// Clean up created directory
75+
if directory != "" {
76+
os.RemoveAll(directory)
15677
}
15778
})
15879

15980
t.Run("valid repository clone - success case", func(t *testing.T) {
16081
buffer := &bytes.Buffer{}
161-
tempDir := t.TempDir()
16282

163-
// This test may fail in CI/test environments due to network restrictions
164-
// but validates the success path
165-
err := gitImplementer.Clone(validRepoUrl, tempDir, buffer)
83+
directory, err := gitImplementer.Clone(validRepoUrl, buffer)
16684

16785
if err != nil {
16886
// Expected in most test environments due to network/auth restrictions
@@ -172,46 +90,45 @@ func TestClone(t *testing.T) {
17290
t.Errorf("Buffer should contain %q on network error, got %q", expectedMessage, buffer.String())
17391
}
17492
} else {
175-
// If clone succeeds (unlikely in test env), verify the clone was successful
93+
// If clone succeeds, verify the clone was successful
17694
if buffer.String() != "" {
17795
t.Errorf("Buffer should be empty on successful clone, got %q", buffer.String())
17896
}
17997

180-
// Verify .git directory was created
181-
gitDir := filepath.Join(tempDir, ".git")
98+
// Verify .git directory was created in returned directory
99+
gitDir := filepath.Join(directory, ".git")
182100
if _, statErr := os.Stat(gitDir); os.IsNotExist(statErr) {
183101
t.Error("Expected .git directory to exist after successful clone")
184102
}
185103
}
186-
})
187-
188-
t.Run("private repository without authentication", func(t *testing.T) {
189-
buffer := &bytes.Buffer{}
190-
tempDir := t.TempDir()
191-
privateRepoURL := "https://github.com/private-user/private-repo"
192104

193-
err := gitImplementer.Clone(privateRepoURL, tempDir, buffer)
194-
if err == nil {
195-
t.Error("Clone should have returned error for private repository without auth")
196-
}
197-
expectedMessage := addonsv1alpha1.GitCloneFailedCondition
198-
if buffer.String() != expectedMessage {
199-
t.Errorf("Buffer should contain %q, got %q", expectedMessage, buffer.String())
105+
// Clean up created directory
106+
if directory != "" {
107+
os.RemoveAll(directory)
200108
}
201109
})
202110

203-
t.Run("protocol not supported", func(t *testing.T) {
204-
buffer := &bytes.Buffer{}
205-
tempDir := t.TempDir()
206-
unsupportedURL := "ftp://example.com/repo.git"
111+
t.Run("nil buffer parameter", func(t *testing.T) {
112+
// This should not panic even with nil buffer
113+
directory, err := gitImplementer.Clone(validRepoUrl, nil)
207114

208-
err := gitImplementer.Clone(unsupportedURL, tempDir, buffer)
209-
if err == nil {
210-
t.Error("Clone should have returned error for unsupported protocol")
115+
if err != nil {
116+
// This is expected in test environments due to network restrictions
117+
t.Logf("Clone failed as expected in test environment: %v", err)
118+
} else {
119+
// If clone succeeds, verify the .git directory was created
120+
gitDir := filepath.Join(directory, ".git")
121+
if _, err := os.Stat(gitDir); os.IsNotExist(err) {
122+
t.Error("Expected .git directory to exist after successful clone")
123+
}
211124
}
212-
expectedMessage := addonsv1alpha1.GitCloneFailedCondition
213-
if buffer.String() != expectedMessage {
214-
t.Errorf("Buffer should contain %q, got %q", expectedMessage, buffer.String())
125+
126+
// Most importantly, the method should not have panicked
127+
t.Log("Method completed without panic despite nil buffer")
128+
129+
// Clean up created directory
130+
if directory != "" {
131+
os.RemoveAll(directory)
215132
}
216133
})
217134
}

0 commit comments

Comments
 (0)