Implement Git files discovery #45

rtnpro · 2020-09-09T16:35:33Z

Fixes #29

dgoodwin · 2020-09-16T12:55:35Z

Looks like the implementation for this PR isn't complete yet but so far looks fine.

Needs a conflict resolved.

rtnpro · 2020-09-24T15:27:03Z

@OmerKahani @dgoodwin One of the design decision I am faced with is if we should implement our own git utility for fetching the git repo and scan it's paths for finding the config json files.

Initially, I was leaning on using the argocd reposerver apiclient to fetch all the paths from the target repository, but it seems that ArgoCD repo server does not allow listing paths, but only applications. I am not sure if it will be possible or optimal to try to propose adding an API endpoint in ArgoCD server to list all paths in a git repository.

So, for the time being, I am thinking to just hit the ArgoCD reposerver to fetch the creds needed to clone a git repo like here:
https://github.com/argoproj/argo-cd/blob/89ee234634c10312d2fcb74c30e9d590fe01d4ff/server/application/application.go#L1279

and initialize a git client to clone the git repo. This sounds like a duplicate effort as ArgoCD reposerver already has a cache of the git repos, but I see the only way forward, for now, is to implement our little git client.

What do you think?

OmerKahani · 2020-09-25T12:02:19Z

We had the same issue in the git directory generator. I think we need to add the functionality to the repo service, but this can take more time.

I did add a repo_service facade, you can add the needed functionality to it, and then we can work on migrating the code to the argocd repo service. This way it will be easy to refactor the code once we have it in the argocd repo service.

@alexec @xianlubird what do you think?

xianlubird · 2020-09-27T03:31:02Z

@OmerKahani agree

rtnpro · 2020-10-20T13:49:51Z

I was away for 2 weeks, as I have become a father. I am back now, and have resumed work on this pull request.

OmerKahani · 2020-10-20T14:02:48Z

I was away for 2 weeks, as I have become a father. I am back now, and have resumed work on this pull request.

Mazal Tov!

rtnpro

@OmerKahani I got the Git files discovery generator working. I need a review here before I start working on the writing tests. Also, I am a bit confused on what tests to add for repo_service.go as I have added almost negligible logic in our code, but using the functions exposed by ArgoCD's git util.

May be, we can mock out the git helpers we added: GetPaths, GetFileContent, and do some functional testing in git_test.go.

rtnpro · 2020-10-25T18:27:58Z

pkg/services/repo_service.go

-	repositoriesDB 		RepositoryDB
-	repoClientset 		apiclient.Clientset
+	repositoriesDB RepositoryDB
+	repoClientset  apiclient.Clientset
 }

 type Apps interface {


I think we should rename this interface to Repos instead of Apps, as we are doing more than just fetch App data now. We also call the reference of this interfaces instance as repos.

+1 for that

rtnpro · 2020-10-25T18:29:46Z

pkg/services/repo_service.go

@@ -66,3 +72,76 @@ func (a *argoCDService) GetApps(ctx context.Context, repoURL string, revision st

 	return res, nil
 }
+
+func (a *argoCDService) checkoutRepo(gitClient git.Client, revision string) error {


I am not sure how best to write unit tests for these. We are not doing anything extra on our side, but just call the ArgoCD git util, which is unit tested.

I don't think we should aim to 100% coverage,

why does the function get's a git client and not creating one based on the repo?

I was hoping to mock the gitClient for testing. But, just sending the repo object makes more sense. Actually, the gitClient name is a bit misleading, as the gitClient object is already tied to a repo.

gitClient is actually tied to a repo instance, and it's not a generic git client like the name suggests. I sent gitClient with the hope of mocking it out for testing. But, there's not much to test here anyways, as the git client is already tested in ArgoCD codebase. I agree with you that sending repo instance makes more sense.

@OmerKahani I gave it a lot of thought to pass repo object to checkoutRepo func, but there is a use case where gitClient is used later, e.g., https://github.com/argoproj-labs/applicationset/pull/45/files#diff-004c4dc22d5fb77bff9d0d6fd29d999d0e068fc99b996bf91a75213585a040efR93. It did not make sense to initialize gitClient multiple times.

So, what I did here: e4e1e5d is rename gitClient to getRepoClient which explicitly states the role of the variable. The git client instance is tied to a repo.

OK, sounds good

OmerKahani · 2020-10-28T18:16:01Z

pkg/services/repo_service.go

+	return nil
+}
+
+func (a *argoCDService) GetPaths(ctx context.Context, repoURL string, revision string, pattern string) ([]string, error) {


Can we use GetPathes to get the applications? Does get path "*" will return all the top level directories and files?

GetPaths uses git ls-files under the hood, and it returns all the paths in the directory tree, matching the specified pattern, and not just the top level files or directories. So, it will need some extra work to retrieve applications using ls-files. We will also need to check if a directory is a valid app directory, by checking if it contains helm or kustomize artifacts. Since, ArgoCD repo server already returns the apps present in a repo, I think we should just piggy back on that functionality.

An application can be plain k8s objects (without Helm / Kustomize). Currently, the repo server will not return such directories, that's was the reason I asked. We should think about it after you merge this code.

OmerKahani · 2020-10-28T18:25:37Z

pkg/generators/git.go


 	return res, nil
 }

-func (g *GitGenerator) filter(Directories []argoprojiov1alpha1.GitDirectoryGeneratorItem, allApps []string) []string {
+func (g *GitGenerator) generateParamsForGitFiles(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator) ([]map[string]string, error) {
+	allPaths := []string{}


This function is going to create a new git client for every file. I am not sure if this is a costly code (does it uses network / IO for it?), but if so, we should probably initialize it only once

The git checkout call creates a local tmp directory, and it gets reused for subsequent calls, until the tmp directory is deleted.

Every git fetch call will involve network I/O to the git servers, but it will not pull the repo from scratch. So, I think we are good here.

OmerKahani · 2020-10-28T18:26:56Z

pkg/generators/git.go

+		content, err := g.repos.GetFileContent(context.TODO(), appSetGenerator.Git.RepoURL, appSetGenerator.Git.Revision, path)
+		if err != nil {
+			return nil, err
+		}
+
+		config := make(map[string]interface{})
+		err = json.Unmarshal(content, &config)
+		if err != nil {
+			return nil, err
+		}
+
+		flat, err := flatten.Flatten(config, "", flatten.DotStyle)
+		if err != nil {
+			return nil, err
+		}
+		params := make(map[string]string)
+		for k, v := range flat {
+			params[k] = v.(string)
+		}
+
+		res = append(res, params)


It might be easier, for testing, to extract this to a new function.

rtnpro · 2020-11-18T16:39:39Z

pkg/generators/git_test.go

 	}

 	for _, c := range cases {
 		cc := c
 		t.Run(cc.name, func(t *testing.T) {
 			argoCDServiceMock := argoCDServiceMock{}
-			argoCDServiceMock.On("GetApps", mock.Anything, mock.Anything, mock.Anything).Return(c.repoApps, c.repoError)
+			mockCalls := argoCDServiceMock.


@OmerKahani I started writing test cases for git file generator scenario, however I am not able to use testify.Mock properly. What I am trying to do is setup multiple expectations per test case:

One for GetPaths

Many for GetFileContent

I tried to use Mock chaining as mentioned here: https://godoc.org/github.com/stretchr/testify/mock#Call.On, but still could not get it working. The test is failing with error below:

WARNING: Package "github.com/golang/protobuf/protoc-gen-go/generator" is deprecated. A future release of golang/protobuf will delete this package, which has long been excluded from the compatibility promise. time="2020-11-18T21:58:55+05:30" level=info msg="applications result from the repo service" allAps="[app1 app2 p1/app3]" repoURL=RepoURL revision=Revision total=3 time="2020-11-18T21:58:55+05:30" level=info msg="applications result from the repo service" allAps="[app1 p1/app2 p1/p2/app3 p1/p2/p3/app4]" repoURL=RepoURL revision=Revision total=4 time="2020-11-18T21:58:55+05:30" level=info msg="applications result from the repo service" allAps="[]" repoURL=RepoURL revision=Revision total=0 --- FAIL: TestGitGenerateParams (0.00s) --- FAIL: TestGitGenerateParams/happy_flow_-_created_apps (0.00s) git_test.go:220: PASS: GetApps(string,string,string) git_test.go:220: FAIL: GetPaths(string,string,string,string) at: [git_test.go:186] git_test.go:220: FAIL: 1 out of 2 expectation(s) were met. The code you are testing needs to make 1 more call(s). at: [git_test.go:220] --- FAIL: TestGitGenerateParams/It_filters_application_according_to_the_paths (0.00s) git_test.go:220: PASS: GetApps(string,string,string) git_test.go:220: FAIL: GetPaths(string,string,string,string) at: [git_test.go:186] git_test.go:220: FAIL: 1 out of 2 expectation(s) were met. The code you are testing needs to make 1 more call(s). at: [git_test.go:220] --- FAIL: TestGitGenerateParams/handles_empty_response_from_repo_server (0.00s) git_test.go:220: PASS: GetApps(string,string,string) git_test.go:220: FAIL: GetPaths(string,string,string,string) at: [git_test.go:186] git_test.go:220: FAIL: 1 out of 2 expectation(s) were met. The code you are testing needs to make 1 more call(s). at: [git_test.go:220] --- FAIL: TestGitGenerateParams/handles_error_from_repo_server (0.00s) git_test.go:220: PASS: GetApps(string,string,string) git_test.go:220: FAIL: GetPaths(string,string,string,string) at: [git_test.go:186] git_test.go:220: FAIL: 1 out of 2 expectation(s) were met. The code you are testing needs to make 1 more call(s). at: [git_test.go:220] --- FAIL: TestGitGenerateParams/handles_error_from_repo_server#01 (0.00s) git_test.go:220: PASS: GetApps(string,string,string) git_test.go:220: FAIL: GetPaths(string,string,string,string) at: [git_test.go:186] git_test.go:220: FAIL: 1 out of 2 expectation(s) were met. The code you are testing needs to make 1 more call(s). at: [git_test.go:220] FAIL FAIL command-line-arguments 1.222s FAIL

Removing or commenting out the new ArgoCDServiceMock.On calls allows the test to pass.

@dgoodwin I need some help here.

@OmerKahani ^^

Hi, sorry for the delay it was a busy week.

The problem is with line 186: https://github.com/argoproj-labs/applicationset/pull/45/files/1f6c6234c6514ba4d0fbf5693699c3c5ece966cc#diff-c1129e2ddfe8072c3267b62d8ac75bcd5dfa92143c40c478527853c08752b7eeR186

I think it will be best to create a new TestGitGenerateParams so there will be two functions:
TestGitGenerateParamsFromDirectory
TestGitGenerateParamsFromFile

everyone will have its own test cases and mock only one of the functions in the argocd service

Does it sound reasonable to you?

Yes. Created separate test function for testing Git file generator.

jgwest · 2020-12-07T20:42:43Z

pkg/services/repo_service.go

+		return nil, err
+	}
+
+	paths, err := gitRepoClient.LsFiles(pattern)


The LsFiles implementation from Argo CD's git client does not handle recursive filters (eg**/*.json to recursively match all JSON files in a repo)

LsFiles is a wrapper around git ls-files --full-name -- (filter), which treats **/*.json as equivalent to */*.json (only matching from root folder)

And related: realized that since Argo CD's util/git utility calls to git directly, git needs to be included in the Dockerfile

@jgwest I ran git ls-files --full-name -- **/*.json on https://github.com/rtnpro/argocd-applicationset-examples/ and it returned

cluster-config/engineering/local/config.json

So, it supports searching for matching files in nested directories.

@jgwest I have added more .json files to the repo: https://github.com/rtnpro/argocd-applicationset-examples/ to validate the output of git ls-files --full-name -- <pattern> and it seems to work as expected.

cc @OmerKahani

@rtnpro Agreed, thanks for investigating, it appears to work as expected for me as well... I'm not able to reproduce with the information I originally provided, and I'm not able to reproduce now (and no longer have the environment I originally used), so this can be resolved.

jgwest · 2020-12-07T20:45:13Z

pkg/generators/git.go

+	}
+
+	config := make(map[string]interface{})
+	err = json.Unmarshal(content, &config)


It looks like this only handles JSON data from the git repo, whereas the proposal notes that "referenced files would contain a json/yaml list of arbitrary structured objects"."

jgwest · 2020-12-07T20:45:54Z

pkg/generators/git.go

+		if err != nil {
+			return nil, err
+		}
+		allPaths = append(allPaths, paths...)


I'm guessing we may want to remove any duplicate files from the list, in the case where multiple path patterns are specified and they happen to (partially) overlap between one another.

@jgwest Fixed by 69a891a

rtnpro · 2020-12-22T17:21:07Z

@OmerKahani @jgwest

Apart from agreeing on whether to use git ls-files ... to find matching paths in a git repo, I have addressed all other review requests. I am confident that we are ready with this pull request.

Regarding the shortcomings mentioned by @jgwest in using git ls-files ... to list file paths matching a pattern, I tried reproducing the issues reported, but could not do so. Ref: #45 (comment)

I am using git version 2.28.0.

jgwest

Thanks @rtnpro, LGTM! Any outstanding items that arise can be handled under separate issues/PRs as needed.

OmerKahani

@rtnpro sorry for all of the delays. This was a great effort from your side and a huge contribution to the project.

rtnpro changed the title ~~Implement Git files discovery~~ [WIP] Implement Git files discovery Sep 9, 2020

rtnpro force-pushed the git-files-generator branch from 663e525 to 68d0d51 Compare October 25, 2020 18:25

rtnpro changed the title ~~[WIP] Implement Git files discovery~~ Implement Git files discovery Oct 25, 2020

rtnpro commented Oct 25, 2020

View reviewed changes

OmerKahani reviewed Oct 28, 2020

View reviewed changes

rtnpro commented Nov 18, 2020

View reviewed changes

jgwest suggested changes Dec 7, 2020

View reviewed changes

jgwest mentioned this pull request Dec 7, 2020

ApplicationSet which generates Applications with matching names should report an error and return #68

Closed

jgwest mentioned this pull request Dec 18, 2020

Graduating ApplicationSets, and shipping it with ArgoCD #86

Open

rtnpro added 2 commits December 22, 2020 22:12

Implement git files discovery generator.

ba7fc08

Rename services.Apps interface to services.Repos

c739d6f

rtnpro force-pushed the git-files-generator branch from b9f62da to eefbbe6 Compare December 22, 2020 16:57

rtnpro added 5 commits December 22, 2020 22:46

Name gitClient in repo_service as gitRepoClient.

e9ee694

Separate function for getting params from a git file.

8416613

Add tests for git file generator.

a9a6316

Handle duplicate paths during generating params from git files.

d734e88

Install git binary in Docker image.

8f1da25

rtnpro force-pushed the git-files-generator branch from 874c593 to 8f1da25 Compare December 22, 2020 17:16

jgwest approved these changes Jan 7, 2021

View reviewed changes

OmerKahani approved these changes Jan 8, 2021

View reviewed changes

xianlubird approved these changes Jan 8, 2021

View reviewed changes

xianlubird merged commit e57aae6 into argoproj:master Jan 8, 2021

jgwest mentioned this pull request Jan 20, 2021

Implement Git files generator E2E tests #104

Closed

jgwest mentioned this pull request Jan 26, 2021

Git files generator: add support for YAML files #106

Closed

Implement Git files discovery #45

Implement Git files discovery #45

Conversation

rtnpro commented Sep 9, 2020

dgoodwin commented Sep 16, 2020

rtnpro commented Sep 24, 2020

OmerKahani commented Sep 25, 2020 • edited

xianlubird commented Sep 27, 2020

rtnpro commented Oct 20, 2020 • edited

OmerKahani commented Oct 20, 2020

rtnpro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtnpro Nov 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtnpro commented Dec 22, 2020

jgwest left a comment

Choose a reason for hiding this comment

OmerKahani left a comment

Choose a reason for hiding this comment

OmerKahani commented Sep 25, 2020 •

edited

rtnpro commented Oct 20, 2020 •

edited

rtnpro Nov 2, 2020 •

edited