Repository: argoproj-labs/applicationset
Pull request #355 (2021-08-26 05:03:27)

fix: improve git files generator performance
Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

Closes https://github.com/argoproj-labs/applicationset/issues/354

PR changes repo services implementation: replaces `GetFilePaths` and `GetFileContent` methods with `GetFiles` that checkout/fetch repo once and return content of discovered files.


Patch:
diff --git a/docs/Generators-SCM-Provider.md b/docs/Generators-SCM-Provider.md
index eefc48e..45899a0 100644
--- a/docs/Generators-SCM-Provider.md
+++ b/docs/Generators-SCM-Provider.md
@@ -88,7 +88,7 @@ spec:
 * `group`: Required name of the base Gitlab group to scan. If you have multiple base groups, use multiple generators.
 * `api`: If using GitHub Enterprise, the URL to access it.
 * `allBranches`: By default (false) the template will only be evaluated for the default branch of each repo. If this is true, every branch of every repository will be passed to the filters. If using this flag, you likely want to use a `branchMatch` filter.
-* `allBranches`: By default (false) the controller will only search for repos directly in the base group. If this is true, it will recurse through all the subgroups searching for repos to scan.
+* `includeSubgroups`: By default (false) the controller will only search for repos directly in the base group. If this is true, it will recurse through all the subgroups searching for repos to scan.
 * `tokenRef`: A `Secret` name and key containing the Gitlab access token to use for requests. If not specified, will make anonymous requests which have a lower rate limit and can only see public repositories.
 
 For label filtering, the repository tags are used.
diff --git a/docs/Use-Cases.md b/docs/Use-Cases.md
index 9148762..57db8e5 100644
--- a/docs/Use-Cases.md
+++ b/docs/Use-Cases.md
@@ -83,7 +83,7 @@ spec:
       source:
         # developers may customize app details using JSON files from above repo URL
         repoURL: {{app.source}}
-        targetRevision: {app.revision}}
+        targetRevision: {{app.revision}}
         path: {{app.path}}
       destination:
         name: production-cluster # cluster is restricted
diff --git a/pkg/generators/git.go b/pkg/generators/git.go
index 6316d25..83211f6 100644
--- a/pkg/generators/git.go
+++ b/pkg/generators/git.go
@@ -92,22 +92,22 @@ func (g *GitGenerator) generateParamsForGitDirectories(appSetGenerator *argoproj
 
 func (g *GitGenerator) generateParamsForGitFiles(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator) ([]map[string]string, error) {
 
-	// Get all paths that match the requested path string, removing duplicates
-	allPathsMap := make(map[string]bool)
+	// Get all files that match the requested path string, removing duplicates
+	allFiles := make(map[string][]byte)
 	for _, requestedPath := range appSetGenerator.Git.Files {
-		paths, err := g.repos.GetFilePaths(context.TODO(), appSetGenerator.Git.RepoURL, appSetGenerator.Git.Revision, requestedPath.Path)
+		files, err := g.repos.GetFiles(context.TODO(), appSetGenerator.Git.RepoURL, appSetGenerator.Git.Revision, requestedPath.Path)
 		if err != nil {
 			return nil, err
 		}
-		for _, path := range paths {
-			allPathsMap[path] = true
+		for filePath, content := range files {
+			allFiles[filePath] = content
 		}
 	}
 
 	// Extract the unduplicated map into a list, and sort by path to ensure a deterministic
 	// processing order in the subsequent step
 	allPaths := []string{}
-	for path := range allPathsMap {
+	for path := range allFiles {
 		allPaths = append(allPaths, path)
 	}
 	sort.Strings(allPaths)
@@ -117,7 +117,7 @@ func (g *GitGenerator) generateParamsForGitFiles(appSetGenerator *argoprojiov1al
 	for _, path := range allPaths {
 
 		// A JSON / YAML file path can contain multiple sets of parameters (ie it is an array)
-		paramsArray, err := g.generateParamsFromGitFile(appSetGenerator, path)
+		paramsArray, err := g.generateParamsFromGitFile(path, allFiles[path])
 		if err != nil {
 			return nil, fmt.Errorf("unable to process file '%s': %v", path, err)
 		}
@@ -129,17 +129,11 @@ func (g *GitGenerator) generateParamsForGitFiles(appSetGenerator *argoprojiov1al
 	return res, nil
 }
 
-func (g *GitGenerator) generateParamsFromGitFile(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator, filePath string) ([]map[string]string, error) {
-
-	fileContent, err := g.repos.GetFileContent(context.TODO(), appSetGenerator.Git.RepoURL, appSetGenerator.Git.Revision, filePath)
-	if err != nil {
-		return nil, err
-	}
-
+func (g *GitGenerator) generateParamsFromGitFile(filePath string, fileContent []byte) ([]map[string]string, error) {
 	objectsFound := []map[string]interface{}{}
 
 	// First, we attempt to parse as an array
-	err = yaml.Unmarshal(fileContent, &objectsFound)
+	err := yaml.Unmarshal(fileContent, &objectsFound)
 	if err != nil {
 		// If unable to parse as an array, attempt to parse as a single object
 		singleObj := make(map[string]interface{})
diff --git a/pkg/services/repo_service.go b/pkg/services/repo_service.go
index 9b54380..1922631 100644
--- a/pkg/services/repo_service.go
+++ b/pkg/services/repo_service.go
@@ -24,14 +24,11 @@ type argoCDService struct {
 
 type Repos interface {
 
-	// GetFilePaths returns a list of files (not directories) within the target repo
-	GetFilePaths(ctx context.Context, repoURL string, revision string, pattern string) ([]string, error)
+	// GetFiles returns content of files (not directories) within the target repo
+	GetFiles(ctx context.Context, repoURL string, revision string, pattern string) (map[string][]byte, error)
 
 	// GetDirectories returns a list of directories (not files) within the target repo
 	GetDirectories(ctx context.Context, repoURL string, revision string) ([]string, error)
-
-	// GetFileContent returns the contents of a particular repository file
-	GetFileContent(ctx context.Context, repoURL string, revision string, path string) ([]byte, error)
 }
 
 func NewArgoCDService(db db.ArgoDB, repoServerAddress string) Repos {
@@ -41,7 +38,7 @@ func NewArgoCDService(db db.ArgoDB, repoServerAddress string) Repos {
 	}
 }
 
-func (a *argoCDService) GetFilePaths(ctx context.Context, repoURL string, revision string, pattern string) ([]string, error) {
+func (a *argoCDService) GetFiles(ctx context.Context, repoURL string, revision string, pattern string) (map[string][]byte, error) {
 	repo, err := a.repositoriesDB.GetRepository(ctx, repoURL)
 	if err != nil {
 		return nil, errors.Wrap(err, "Error in GetRepository")
@@ -63,7 +60,16 @@ func (a *argoCDService) GetFilePaths(ctx context.Context, repoURL string, revisi
 		return nil, errors.Wrap(err, "Error during listing files of local repo")
 	}
 
-	return paths, nil
+	res := map[string][]byte{}
+	for _, filePath := range paths {
+		bytes, err := os.ReadFile(filepath.Join(gitRepoClient.Root(), filePath))
+		if err != nil {
+			return nil, err
+		}
+		res[filePath] = bytes
+	}
+
+	return res, nil
 }
 
 func (a *argoCDService) GetDirectories(ctx context.Context, repoURL string, revision string) ([]string, error) {
@@ -120,31 +126,6 @@ func (a *argoCDService) GetDirectories(ctx context.Context, repoURL string, revi
 
 }
 
-func (a *argoCDService) GetFileContent(ctx context.Context, repoURL string, revision string, path string) ([]byte, error) {
-	repo, err := a.repositoriesDB.GetRepository(ctx, repoURL)
-	if err != nil {
-		return nil, errors.Wrap(err, "Error in GetRepository")
-	}
-
-	gitRepoClient, err := git.NewClient(repo.Repo, repo.GetGitCreds(), repo.IsInsecure(), repo.IsLFSEnabled(), repo.Proxy)
-
-	if err != nil {
-		return nil, err
-	}
-
-	err = checkoutRepo(gitRepoClient, revision)
-	if err != nil {
-		return nil, err
-	}
-
-	bytes, err := os.ReadFile(filepath.Join(gitRepoClient.Root(), path))
-	if err != nil {
-		return nil, err
-	}
-
-	return bytes, nil
-}
-
 func checkoutRepo(gitRepoClient git.Client, revision string) error {
 	err := gitRepoClient.Init()
 	if err != nil {


Test patch:
diff --git a/pkg/generators/git_test.go b/pkg/generators/git_test.go
index 0c3c293..296cac0 100644
--- a/pkg/generators/git_test.go
+++ b/pkg/generators/git_test.go
@@ -29,10 +29,10 @@ func (a argoCDServiceMock) GetApps(ctx context.Context, repoURL string, revision
 	return args.Get(0).([]string), args.Error(1)
 }
 
-func (a argoCDServiceMock) GetFilePaths(ctx context.Context, repoURL string, revision string, pattern string) ([]string, error) {
+func (a argoCDServiceMock) GetFiles(ctx context.Context, repoURL string, revision string, pattern string) (map[string][]byte, error) {
 	args := a.mock.Called(ctx, repoURL, revision, pattern)
 
-	return args.Get(0).([]string), args.Error(1)
+	return args.Get(0).(map[string][]byte), args.Error(1)
 }
 
 func (a argoCDServiceMock) GetFileContent(ctx context.Context, repoURL string, revision string, path string) ([]byte, error) {
@@ -185,24 +185,16 @@ func TestGitGenerateParamsFromFiles(t *testing.T) {
 		name string
 		// files is the list of paths/globs to match
 		files []argoprojiov1alpha1.GitFileGeneratorItem
-		// repoPaths is the list of matching paths in the simulated git repository
-		repoPaths []string
 		// repoFileContents maps repo path to the literal contents of that path
 		repoFileContents map[string][]byte
 		// if repoPathsError is non-nil, the call to GetPaths(...) will return this error value
 		repoPathsError error
-		// if repoFileContentsErrors contains a path key, the error value will be returned on the call to GetFileContents(...)
-		repoFileContentsErrors map[string]error
-		expected               []map[string]string
-		expectedError          error
+		expected       []map[string]string
+		expectedError  error
 	}{
 		{
 			name:  "happy flow: create params from git files",
 			files: []argoprojiov1alpha1.GitFileGeneratorItem{{Path: "**/config.json"}},
-			repoPaths: []string{
-				"cluster-config/production/config.json",
-				"cluster-config/staging/config.json",
-			},
 			repoFileContents: map[string][]byte{
 				"cluster-config/production/config.json": []byte(`{
    "cluster": {
@@ -227,8 +219,7 @@ func TestGitGenerateParamsFromFiles(t *testing.T) {
    }
 }`),
 			},
-			repoPathsError:         nil,
-			repoFileContentsErrors: nil,
+			repoPathsError: nil,
 			expected: []map[string]string{
 				{
 					"cluster.owner":        "john.doe@example.com",
@@ -252,60 +243,26 @@ func TestGitGenerateParamsFromFiles(t *testing.T) {
 			expectedError: nil,
 		},
 		{
-			name:                   "handles error during getting repo paths",
-			files:                  []argoprojiov1alpha1.GitFileGeneratorItem{{Path: "**/config.json"}},
-			repoPaths:              []string{},
-			repoFileContents:       map[string][]byte{},
-			repoPathsError:         fmt.Errorf("paths error"),
-			repoFileContentsErrors: nil,
-			expected:               []map[string]string{},
-			expectedError:          fmt.Errorf("paths error"),
-		},
-		{
-			name:  "handles error during getting repo file contents",
-			files: []argoprojiov1alpha1.GitFileGeneratorItem{{Path: "**/config.json"}},
-			repoPaths: []string{
-				"cluster-config/production/config.json",
-				"cluster-config/staging/config.json",
-			},
-			repoFileContents: map[string][]byte{
-				"cluster-config/production/config.json": []byte(`{
-   "cluster": {
-       "owner": "john.doe@example.com",
-       "name": "production",
-       "address": "https://kubernetes.default.svc"
-   }
-}`),
-				"cluster-config/staging/config.json": nil,
-			},
-			repoPathsError: nil,
-			repoFileContentsErrors: map[string]error{
-				"cluster-config/production/config.json": nil,
-				"cluster-config/staging/config.json":    fmt.Errorf("staging config file get content error"),
-			},
-			expected:      []map[string]string{},
-			expectedError: fmt.Errorf("unable to process file 'cluster-config/staging/config.json': staging config file get content error"),
+			name:             "handles error during getting repo paths",
+			files:            []argoprojiov1alpha1.GitFileGeneratorItem{{Path: "**/config.json"}},
+			repoFileContents: map[string][]byte{},
+			repoPathsError:   fmt.Errorf("paths error"),
+			expected:         []map[string]string{},
+			expectedError:    fmt.Errorf("paths error"),
 		},
 		{
 			name:  "test invalid JSON file returns error",
 			files: []argoprojiov1alpha1.GitFileGeneratorItem{{Path: "**/config.json"}},
-			repoPaths: []string{
-				"cluster-config/production/config.json",
-			},
 			repoFileContents: map[string][]byte{
 				"cluster-config/production/config.json": []byte(`invalid json file`),
 			},
-			repoPathsError:         nil,
-			repoFileContentsErrors: map[string]error{},
-			expected:               []map[string]string{},
-			expectedError:          fmt.Errorf("unable to process file 'cluster-config/production/config.json': unable to parse file: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go value of type map[string]interface {}"),
+			repoPathsError: nil,
+			expected:       []map[string]string{},
+			expectedError:  fmt.Errorf("unable to process file 'cluster-config/production/config.json': unable to parse file: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go value of type map[string]interface {}"),
 		},
 		{
 			name:  "test JSON array",
 			files: []argoprojiov1alpha1.GitFileGeneratorItem{{Path: "**/config.json"}},
-			repoPaths: []string{
-				"cluster-config/production/config.json",
-			},
 			repoFileContents: map[string][]byte{
 				"cluster-config/production/config.json": []byte(`
 [
@@ -328,8 +285,7 @@ func TestGitGenerateParamsFromFiles(t *testing.T) {
 	}
 ]`),
 			},
-			repoPathsError:         nil,
-			repoFileContentsErrors: map[string]error{},
+			repoPathsError: nil,
 			expected: []map[string]string{
 				{
 					"cluster.owner":     "john.doe@example.com",
@@ -352,10 +308,6 @@ func TestGitGenerateParamsFromFiles(t *testing.T) {
 		{
 			name:  "Test YAML flow",
 			files: []argoprojiov1alpha1.GitFileGeneratorItem{{Path: "**/config.yaml"}},
-			repoPaths: []string{
-				"cluster-config/production/config.yaml",
-				"cluster-config/staging/config.yaml",
-			},
 			repoFileContents: map[string][]byte{
 				"cluster-config/production/config.yaml": []byte(`
 cluster:
@@ -375,8 +327,7 @@ cluster:
   address: https://kubernetes.default.svc
 `),
 			},
-			repoPathsError:         nil,
-			repoFileContentsErrors: nil,
+			repoPathsError: nil,
 			expected: []map[string]string{
 				{
 					"cluster.owner":        "john.doe@example.com",
@@ -401,9 +352,6 @@ cluster:
 		{
 			name:  "test YAML array",
 			files: []argoprojiov1alpha1.GitFileGeneratorItem{{Path: "**/config.yaml"}},
-			repoPaths: []string{
-				"cluster-config/production/config.yaml",
-			},
 			repoFileContents: map[string][]byte{
 				"cluster-config/production/config.yaml": []byte(`
 - cluster:
@@ -417,8 +365,7 @@ cluster:
     name: staging
     address: https://kubernetes.default.svc`),
 			},
-			repoPathsError:         nil,
-			repoFileContentsErrors: map[string]error{},
+			repoPathsError: nil,
 			expected: []map[string]string{
 				{
 					"cluster.owner":     "john.doe@example.com",
@@ -444,14 +391,8 @@ cluster:
 		cc := c
 		t.Run(cc.name, func(t *testing.T) {
 			argoCDServiceMock := argoCDServiceMock{mock: &mock.Mock{}}
-			argoCDServiceMock.mock.On("GetFilePaths", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
-				Return(c.repoPaths, c.repoPathsError)
-			if c.repoPaths != nil {
-				for _, repoPath := range c.repoPaths {
-					argoCDServiceMock.mock.On("GetFileContent", mock.Anything, mock.Anything, mock.Anything, repoPath).
-						Return(c.repoFileContents[repoPath], c.repoFileContentsErrors[repoPath]).Once()
-				}
-			}
+			argoCDServiceMock.mock.On("GetFiles", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
+				Return(c.repoFileContents, c.repoPathsError)
 
 			var gitGenerator = NewGitGenerator(argoCDServiceMock)
 			applicationSetInfo := argoprojiov1alpha1.ApplicationSet{
diff --git a/pkg/services/repo_service_test.go b/pkg/services/repo_service_test.go
index 90718ce..077036e 100644
--- a/pkg/services/repo_service_test.go
+++ b/pkg/services/repo_service_test.go
@@ -3,10 +3,7 @@ package services
 import (
 	"context"
 	"errors"
-	"os"
-	"path/filepath"
 	"sort"
-	"strings"
 	"testing"
 
 	"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
@@ -104,7 +101,7 @@ func TestGetDirectories(t *testing.T) {
 	}
 }
 
-func TestGetPaths(t *testing.T) {
+func TestGetFiles(t *testing.T) {
 
 	// Hardcode a specific commit, so that changes to argoproj/argocd-example-apps/ don't break our tests
 	// "chore: downgrade kustomize guestbook image tag (#73)"
@@ -214,7 +211,7 @@ func TestGetPaths(t *testing.T) {
 				repositoriesDB: argocdRepositoryMock,
 			}
 
-			getPathsRes, err := argocd.GetFilePaths(context.Background(), cc.repoURL, cc.revision, cc.pattern)
+			getPathsRes, err := argocd.GetFiles(context.Background(), cc.repoURL, cc.revision, cc.pattern)
 
 			if cc.expectedError == nil {
 
@@ -232,103 +229,4 @@ func TestGetPaths(t *testing.T) {
 			}
 		})
 	}
-
-}
-
-func TestGetFileContent(t *testing.T) {
-
-	// Hardcode a specific commit, so that changes to argoproj/argocd-example-apps/ don't break our tests
-	// "chore: downgrade kustomize guestbook image tag (#73)"
-	commitID := "08f72e2a309beab929d9fd14626071b1a61a47f9"
-
-	tests := []struct {
-		name     string
-		repoURL  string
-		revision string
-		path     string
-		repoRes  *v1alpha1.Repository
-		repoErr  error
-
-		expectedFileContentsSubstring string
-		expectedError                 error
-	}{
-		{
-			name: "pull a specific path of a revision of example apps",
-			repoRes: &v1alpha1.Repository{
-				Insecure:              true,
-				InsecureIgnoreHostKey: true,
-				Repo:                  "https://github.com/argoproj/argocd-example-apps/",
-			},
-			repoURL:                       "https://github.com/argoproj/argocd-example-apps/",
-			revision:                      commitID,
-			path:                          "/README.md",
-			expectedFileContentsSubstring: "This repository contains example applications for demoing ArgoCD functionality",
-		},
-		{
-			name: "pull another specific path of a revision of example apps",
-			repoRes: &v1alpha1.Repository{
-				Insecure:              true,
-				InsecureIgnoreHostKey: true,
-				Repo:                  "https://github.com/argoproj/argocd-example-apps/",
-			},
-			repoURL:                       "https://github.com/argoproj/argocd-example-apps/",
-			revision:                      commitID,
-			path:                          "/helm-guestbook/Chart.yaml",
-			expectedFileContentsSubstring: "A Helm chart for Kubernetes",
-		},
-
-		{
-			name: "pull an invalid revison",
-			repoRes: &v1alpha1.Repository{
-				Insecure:              true,
-				InsecureIgnoreHostKey: true,
-				Repo:                  "https://github.com/argoproj/argocd-example-apps/",
-			},
-			repoURL:       "https://github.com/argoproj/argocd-example-apps/",
-			revision:      "this-tag-does-not-exist",
-			path:          "/README.md",
-			expectedError: errors.New("Error during fetching repo: `git fetch origin this-tag-does-not-exist --tags --force` failed exit status 128: fatal: couldn't find remote ref this-tag-does-not-exist"),
-		},
-		{
-			name: "pull an invalid file",
-			repoRes: &v1alpha1.Repository{
-				Insecure:              true,
-				InsecureIgnoreHostKey: true,
-				Repo:                  "https://github.com/argoproj/argocd-example-apps/",
-			},
-			repoURL:       "https://github.com/argoproj/argocd-example-apps/",
-			revision:      commitID,
-			path:          "/this-file-does-not-exist.md",
-			expectedError: errors.New("open " + filepath.Join(os.TempDir()+"/https___github.com_argoproj_argocd-example-apps_/this-file-does-not-exist.md"+": no such file or directory")),
-		},
-	}
-
-	for _, cc := range tests {
-
-		// Get the file contents of a specific path and revision, and confirm it has the expected value (or the expected error is returned.)
-		t.Run(cc.name, func(t *testing.T) {
-
-			argocdRepositoryMock := ArgocdRepositoryMock{mock: &mock.Mock{}}
-
-			argocd := argoCDService{
-				repositoriesDB: argocdRepositoryMock,
-			}
-
-			argocdRepositoryMock.mock.On("GetRepository", mock.Anything, cc.repoURL).Return(cc.repoRes, cc.repoErr)
-
-			fileContentRes, err := argocd.GetFileContent(context.Background(), cc.repoURL, cc.revision, cc.path)
-
-			if cc.expectedError == nil {
-
-				// File contents should contain the expected string
-				assert.True(t, strings.Contains(string(fileContentRes), cc.expectedFileContentsSubstring))
-				assert.NoError(t, err)
-
-			} else {
-				assert.EqualError(t, err, cc.expectedError.Error())
-			}
-
-		})
-	}
-
 }