From 16bc588193942a394b42b50fdd5c637f8473ae45 Mon Sep 17 00:00:00 2001 From: Joe Lombrozo Date: Thu, 19 Dec 2024 10:47:39 -0500 Subject: [PATCH 1/5] add more count types to result log --- pkg/affected_apps/argocd_matcher.go | 5 ++++- pkg/appdir/app_directory.go | 10 +++++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/pkg/affected_apps/argocd_matcher.go b/pkg/affected_apps/argocd_matcher.go index 935daeeb..1cd5804a 100644 --- a/pkg/affected_apps/argocd_matcher.go +++ b/pkg/affected_apps/argocd_matcher.go @@ -36,7 +36,10 @@ func logCounts(repoApps *appdir.AppDirectory) { if repoApps == nil { log.Debug().Msg("found no apps") } else { - log.Debug().Msgf("found %d apps", repoApps.Count()) + log.Debug().Int("apps", repoApps.AppsCount()). + Int("app files", repoApps.AppFilesCount()). + Int("app dirs", repoApps.AppDirsCount()). + Msg("mapped apps") } } diff --git a/pkg/appdir/app_directory.go b/pkg/appdir/app_directory.go index a915fe61..59533eed 100644 --- a/pkg/appdir/app_directory.go +++ b/pkg/appdir/app_directory.go @@ -23,10 +23,18 @@ func NewAppDirectory() *AppDirectory { } } -func (d *AppDirectory) Count() int { +func (d *AppDirectory) AppsCount() int { return len(d.appsMap) } +func (d *AppDirectory) AppFilesCount() int { + return len(d.appFiles) +} + +func (d *AppDirectory) AppDirsCount() int { + return len(d.appDirs) +} + func (d *AppDirectory) Union(other *AppDirectory) *AppDirectory { var join AppDirectory join.appsMap = mergeMaps(d.appsMap, other.appsMap, takeFirst[v1alpha1.Application]) From 418e5fc4cad88df71fda07014a8e1eea4e40d8fa Mon Sep 17 00:00:00 2001 From: Joe Lombrozo Date: Thu, 19 Dec 2024 10:50:56 -0500 Subject: [PATCH 2/5] fix func names --- pkg/app_watcher/app_watcher_test.go | 13 +++++++------ pkg/appdir/vcstoargomap_test.go | 16 +++++++--------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/pkg/app_watcher/app_watcher_test.go b/pkg/app_watcher/app_watcher_test.go index 4af7425c..9f9c7380 100644 --- a/pkg/app_watcher/app_watcher_test.go +++ b/pkg/app_watcher/app_watcher_test.go @@ -86,9 +86,10 @@ func TestApplicationUpdated(t *testing.T) { assert.Equal(t, len(ctrl.vcsToArgoMap.GetMap()), 2) oldAppDirectory := ctrl.vcsToArgoMap.GetAppsInRepo("https://gitlab.com/test/repo.git") + assert.Equal(t, oldAppDirectory.AppsCount(), 1) + newAppDirectory := ctrl.vcsToArgoMap.GetAppsInRepo("https://gitlab.com/test/repo-3.git") - assert.Equal(t, oldAppDirectory.Count(), 1) - assert.Equal(t, newAppDirectory.Count(), 0) + assert.Equal(t, newAppDirectory.AppsCount(), 0) // _, err := ctrl.applicationClientset.ArgoprojV1alpha1().Applications("default").Update(ctx, &v1alpha1.Application{ ObjectMeta: metav1.ObjectMeta{Name: "test-app-1", Namespace: "default"}, @@ -102,8 +103,8 @@ func TestApplicationUpdated(t *testing.T) { time.Sleep(time.Second * 1) oldAppDirectory = ctrl.vcsToArgoMap.GetAppsInRepo("https://gitlab.com/test/repo.git") newAppDirectory = ctrl.vcsToArgoMap.GetAppsInRepo("https://gitlab.com/test/repo-3.git") - assert.Equal(t, oldAppDirectory.Count(), 0) - assert.Equal(t, newAppDirectory.Count(), 1) + assert.Equal(t, oldAppDirectory.AppsCount(), 0) + assert.Equal(t, newAppDirectory.AppsCount(), 1) } func TestApplicationDeleted(t *testing.T) { @@ -119,7 +120,7 @@ func TestApplicationDeleted(t *testing.T) { assert.Equal(t, len(ctrl.vcsToArgoMap.GetMap()), 2) appDirectory := ctrl.vcsToArgoMap.GetAppsInRepo("https://gitlab.com/test/repo.git") - assert.Equal(t, appDirectory.Count(), 1) + assert.Equal(t, appDirectory.AppsCount(), 1) // err := ctrl.applicationClientset.ArgoprojV1alpha1().Applications("default").Delete(ctx, "test-app-1", metav1.DeleteOptions{}) if err != nil { @@ -128,7 +129,7 @@ func TestApplicationDeleted(t *testing.T) { time.Sleep(time.Second * 1) appDirectory = ctrl.vcsToArgoMap.GetAppsInRepo("https://gitlab.com/test/repo.git") - assert.Equal(t, appDirectory.Count(), 0) + assert.Equal(t, appDirectory.AppsCount(), 0) } // TestIsGitRepo will test various URLs against the isGitRepo function. diff --git a/pkg/appdir/vcstoargomap_test.go b/pkg/appdir/vcstoargomap_test.go index 95a5d088..350f6504 100644 --- a/pkg/appdir/vcstoargomap_test.go +++ b/pkg/appdir/vcstoargomap_test.go @@ -26,7 +26,7 @@ func TestAddApp(t *testing.T) { v2a.AddApp(app1) appDir := v2a.GetAppsInRepo("https://github.com/argoproj/argo-cd.git") - assert.Equal(t, appDir.Count(), 1) + assert.Equal(t, appDir.AppsCount(), 1) assert.Equal(t, len(appDir.appDirs["test-app-1"]), 1) // Assertions to verify the behavior here. @@ -41,7 +41,7 @@ func TestAddApp(t *testing.T) { } v2a.AddApp(app2) - assert.Equal(t, appDir.Count(), 2) + assert.Equal(t, appDir.AppsCount(), 2) assert.Equal(t, len(appDir.appDirs["test-app-2"]), 1) } @@ -73,12 +73,12 @@ func TestDeleteApp(t *testing.T) { v2a.AddApp(app2) appDir := v2a.GetAppsInRepo("https://github.com/argoproj/argo-cd.git") - assert.Equal(t, appDir.Count(), 2) + assert.Equal(t, appDir.AppsCount(), 2) assert.Equal(t, len(appDir.appDirs["test-app-1"]), 1) assert.Equal(t, len(appDir.appDirs["test-app-2"]), 1) v2a.DeleteApp(app2) - assert.Equal(t, appDir.Count(), 1) + assert.Equal(t, appDir.AppsCount(), 1) assert.Equal(t, len(appDir.appDirs["test-app-2"]), 0) } @@ -86,14 +86,13 @@ func TestVcsToArgoMap_AddAppSet(t *testing.T) { type args struct { app *v1alpha1.ApplicationSet } - tests := []struct { + tests := map[string]struct { name string fields VcsToArgoMap args args expectedCount int }{ - { - name: "normal process, expect to get the appset stored in the map", + "normal process, expect to get the appset stored in the map": { fields: NewVcsToArgoMap("dummyuser"), args: args{ app: &v1alpha1.ApplicationSet{ @@ -112,8 +111,7 @@ func TestVcsToArgoMap_AddAppSet(t *testing.T) { }, expectedCount: 1, }, - { - name: "invalid appset", + "invalid appset": { fields: NewVcsToArgoMap("vcs-username"), args: args{ app: &v1alpha1.ApplicationSet{ From 5baf1fa7b78da60f9ee49e53c50c069f828cb17a Mon Sep 17 00:00:00 2001 From: Joe Lombrozo Date: Thu, 19 Dec 2024 15:32:15 -0500 Subject: [PATCH 3/5] fix bug helm value files need to be indexed, as they may be outside the chart dir --- pkg/affected_apps/argocd_matcher.go | 5 +- pkg/affected_apps/argocd_matcher_test.go | 203 +++++++++++++++++++++++ pkg/appdir/app_directory.go | 14 +- pkg/events/check.go | 4 +- 4 files changed, 220 insertions(+), 6 deletions(-) diff --git a/pkg/affected_apps/argocd_matcher.go b/pkg/affected_apps/argocd_matcher.go index 1cd5804a..589120b1 100644 --- a/pkg/affected_apps/argocd_matcher.go +++ b/pkg/affected_apps/argocd_matcher.go @@ -37,8 +37,8 @@ func logCounts(repoApps *appdir.AppDirectory) { log.Debug().Msg("found no apps") } else { log.Debug().Int("apps", repoApps.AppsCount()). - Int("app files", repoApps.AppFilesCount()). - Int("app dirs", repoApps.AppDirsCount()). + Int("app_files", repoApps.AppFilesCount()). + Int("app_dirs", repoApps.AppDirsCount()). Msg("mapped apps") } } @@ -46,6 +46,7 @@ func logCounts(repoApps *appdir.AppDirectory) { func getKustomizeApps(vcsToArgoMap appdir.VcsToArgoMap, repo *git.Repo, repoPath string) *appdir.AppDirectory { log.Debug().Msgf("creating fs for %s", repoPath) fs := os.DirFS(repoPath) + log.Debug().Msg("following kustomize apps") kustomizeAppFiles := vcsToArgoMap.WalkKustomizeApps(repo.CloneURL, fs) diff --git a/pkg/affected_apps/argocd_matcher_test.go b/pkg/affected_apps/argocd_matcher_test.go index 0db01456..a719034b 100644 --- a/pkg/affected_apps/argocd_matcher_test.go +++ b/pkg/affected_apps/argocd_matcher_test.go @@ -2,9 +2,17 @@ package affected_apps import ( "context" + "crypto/md5" + "encoding/hex" + "os" + "path/filepath" "testing" + "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/zapier/kubechecks/pkg" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/zapier/kubechecks/pkg/appdir" "github.com/zapier/kubechecks/pkg/git" @@ -41,3 +49,198 @@ func TestFindAffectedAppsWithNilAppsDirectory(t *testing.T) { require.Len(t, items.Applications, 0) require.Len(t, items.ApplicationSets, 0) } + +func TestScenarios(t *testing.T) { + t.Run("simple setup", func(t *testing.T) { + ctx := context.Background() + repoURL := "https://github.com/argoproj/argocd-example-apps.git" + repoFiles := map[string]string{ + // app of apps of apps + "app-of-apps-of-apps/kustomization.yaml": ` +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization + +resources: + - clusters.yaml +`, + "app-of-apps-of-apps/clusters.yaml": ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: cluster-apps +spec: + destination: + namespace: argocd + server: https://kubernetes.default.svc + project: default + source: + path: charts/app-of-apps + repoURL: https://gitlab.com/zapier/argo-cd-configs.git + targetRevision: HEAD + helm: + valueFiles: + - values.yaml + - ../../app-of-apps/cluster.yaml + syncPolicy: + automated: + prune: true +`, + // helm chart that renders app of apps + "charts/app-of-apps/Chart.yaml": ` +apiVersion: v1 +description: Chart for setting up argocd applications +name: app-of-apps +version: 1.0.0 +`, + "charts/app-of-apps/values.yaml": ``, + "charts/app-of-apps/templates/app.yaml": ` +{{- range $name, $app := .Values.apps }} +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: {{ $name }} +spec: + destination: + namespace: argocd + source: + path: apps/{{ $name }} + +`, + + // values files for app of apps + "app-of-apps/cluster.yaml": ` +apps: + app1: `, + + // one app + "apps/app1/Chart.yaml": ``, + "apps/app1/values.yaml": ``, + } + + testRepo := dumpFiles(t, repoURL, "HEAD", repoFiles) + + testVCSMap := appdir.NewVcsToArgoMap("vcs-username") + testVCSMap.AddApp(&v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "app-of-apps-of-apps", + }, + Spec: v1alpha1.ApplicationSpec{ + Source: &v1alpha1.ApplicationSource{ + RepoURL: repoURL, + Path: "app-of-apps-of-apps", + TargetRevision: "HEAD", + }, + }, + }) + + testVCSMap.AddApp(&v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "app-of-apps", + }, + Spec: v1alpha1.ApplicationSpec{ + Source: &v1alpha1.ApplicationSource{ + RepoURL: repoURL, + Path: "charts/app-of-apps", + TargetRevision: "HEAD", + Helm: &v1alpha1.ApplicationSourceHelm{ + ValueFiles: []string{ + "values.yaml", + "../../app-of-apps/cluster.yaml", + }, + }, + }, + }, + }) + testVCSMap.AddApp(&v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "app", + }, + Spec: v1alpha1.ApplicationSpec{ + Source: &v1alpha1.ApplicationSource{ + RepoURL: repoURL, + Path: "app/app1", + TargetRevision: "HEAD", + Helm: &v1alpha1.ApplicationSourceHelm{ + ValueFiles: []string{ + "values.yaml", + }, + }, + }, + }, + }) + + m, err := NewArgocdMatcher(testVCSMap, testRepo) + require.NoError(t, err) + + t.Run("modify cluster.yaml", func(t *testing.T) { + result, err := m.AffectedApps(ctx, []string{"app-of-apps/cluster.yaml"}, "main", testRepo) + require.NoError(t, err) + + require.Len(t, result.Applications, 1) + app := result.Applications[0] + assert.Equal(t, "app-of-apps", app.Name) + }) + + t.Run("modify clusters.yaml", func(t *testing.T) { + result, err := m.AffectedApps(ctx, []string{"app-of-apps-of-apps/clusters.yaml"}, "main", testRepo) + require.NoError(t, err) + + require.Len(t, result.Applications, 1) + app := result.Applications[0] + assert.Equal(t, "app-of-apps-of-apps", app.Name) + }) + + t.Run("unindexed file in kustomize directory", func(t *testing.T) { + result, err := m.AffectedApps(ctx, []string{"app-of-apps-of-apps/invalid.yaml"}, "main", testRepo) + require.NoError(t, err) + + require.Len(t, result.Applications, 1) + app := result.Applications[0] + assert.Equal(t, "app-of-apps-of-apps", app.Name) + }) + + t.Run("unindexed file in helm directory", func(t *testing.T) { + result, err := m.AffectedApps(ctx, []string{"charts/app-of-apps/templates/new.yaml"}, "main", testRepo) + require.NoError(t, err) + + require.Len(t, result.Applications, 1) + app := result.Applications[0] + assert.Equal(t, "app-of-apps", app.Name) + }) + }) +} + +func dumpFiles(t *testing.T, cloneURL, target string, files map[string]string) *git.Repo { + repoHash := hash(t, cloneURL, target) + tempDir := filepath.Join(t.TempDir(), repoHash) + repo := &git.Repo{ + BranchName: target, + CloneURL: cloneURL, + Directory: tempDir, + } + + for file, fileContent := range files { + fullfilepath := filepath.Join(tempDir, file) + + // ensure the directories exist + filedir := filepath.Dir(fullfilepath) + err := os.MkdirAll(filedir, 0o755) + require.NoError(t, err) + + // write the file to disk + err = os.WriteFile(fullfilepath, []byte(fileContent), 0o600) + require.NoError(t, err) + } + + return repo +} + +func hash(t *testing.T, repoURL, target string) string { + t.Helper() + + url, err := pkg.Canonicalize(repoURL) + require.NoError(t, err) + + data := md5.Sum([]byte(url.Host + url.Path + target)) + return hex.EncodeToString(data[:]) +} diff --git a/pkg/appdir/app_directory.go b/pkg/appdir/app_directory.go index 59533eed..9227b3f6 100644 --- a/pkg/appdir/app_directory.go +++ b/pkg/appdir/app_directory.go @@ -163,14 +163,24 @@ func (d *AppDirectory) AddApp(app v1alpha1.Application) { log.Debug().Msgf("app %s already exists", app.Name) return } + + sourcePath := getSourcePath(app) log.Debug(). Str("appName", app.Name). Str("cluster-name", app.Spec.Destination.Name). Str("cluster-server", app.Spec.Destination.Server). - Str("source", getSourcePath(app)). + Str("source", sourcePath). Msg("add app") + d.appsMap[app.Name] = app - d.AddDir(app.Name, getSourcePath(app)) + d.AddDir(app.Name, sourcePath) + + if helm := app.Spec.GetSource().Helm; helm != nil { + for _, path := range helm.ValueFiles { + relPath := filepath.Join(sourcePath, path) + d.AddFile(app.Name, relPath) + } + } } func (d *AppDirectory) AddDir(appName, path string) { diff --git a/pkg/events/check.go b/pkg/events/check.go index 94ea4e81..6a6cf433 100644 --- a/pkg/events/check.go +++ b/pkg/events/check.go @@ -58,7 +58,7 @@ type repoManager interface { Clone(ctx context.Context, cloneURL, branchName string) (*git.Repo, error) } -func GenerateMatcher(ce *CheckEvent, repo *git.Repo) error { +func generateMatcher(ce *CheckEvent, repo *git.Repo) error { log.Debug().Msg("using the argocd matcher") m, err := affected_apps.NewArgocdMatcher(ce.ctr.VcsToArgoMap, repo) if err != nil { @@ -265,7 +265,7 @@ func (ce *CheckEvent) Process(ctx context.Context) error { } // Generate a list of affected apps, storing them within the CheckEvent (also returns but discarded here) - if err = ce.GenerateListOfAffectedApps(ctx, repo, ce.pullRequest.BaseRef, GenerateMatcher); err != nil { + if err = ce.GenerateListOfAffectedApps(ctx, repo, ce.pullRequest.BaseRef, generateMatcher); err != nil { return errors.Wrap(err, "failed to generate a list of affected apps") } From c4aa6147f571adcb405442adfec73862f8d64c41 Mon Sep 17 00:00:00 2001 From: Joe Lombrozo Date: Thu, 19 Dec 2024 16:15:52 -0500 Subject: [PATCH 4/5] two functions should have been one, plus some appset renaming --- pkg/affected_apps/argocd_matcher.go | 2 +- pkg/appdir/app_directory.go | 67 +++++++++++++---------------- pkg/appdir/app_directory_test.go | 2 +- pkg/appdir/appset_directory.go | 12 +++--- pkg/appdir/appset_directory_test.go | 4 +- pkg/appdir/vcstoargomap.go | 14 +++--- pkg/appdir/vcstoargomap_test.go | 2 +- 7 files changed, 49 insertions(+), 54 deletions(-) diff --git a/pkg/affected_apps/argocd_matcher.go b/pkg/affected_apps/argocd_matcher.go index 589120b1..1fd33fc8 100644 --- a/pkg/affected_apps/argocd_matcher.go +++ b/pkg/affected_apps/argocd_matcher.go @@ -80,7 +80,7 @@ func (a *ArgocdMatcher) AffectedApps(_ context.Context, changeList []string, tar } appsSlice := a.appsDirectory.FindAppsBasedOnChangeList(changeList, targetBranch) - appSetsSlice := a.appSetsDirectory.FindAppsBasedOnChangeList(changeList, repo) + appSetsSlice := a.appSetsDirectory.FindAppSetsBasedOnChangeList(changeList, repo) // and return both apps and appSets return AffectedItems{ diff --git a/pkg/appdir/app_directory.go b/pkg/appdir/app_directory.go index 9227b3f6..c000da87 100644 --- a/pkg/appdir/app_directory.go +++ b/pkg/appdir/app_directory.go @@ -43,29 +43,6 @@ func (d *AppDirectory) Union(other *AppDirectory) *AppDirectory { return &join } -func (d *AppDirectory) ProcessApp(app v1alpha1.Application) { - appName := app.Name - - src := app.Spec.GetSource() - - // common data - srcPath := src.Path - d.AddApp(app) - - // handle extra helm paths - if helm := src.Helm; helm != nil { - for _, param := range helm.FileParameters { - path := filepath.Join(srcPath, param.Path) - d.AddFile(appName, path) - } - - for _, valueFilePath := range helm.ValueFiles { - path := filepath.Join(srcPath, valueFilePath) - d.AddFile(appName, path) - } - } -} - // FindAppsBasedOnChangeList receives a list of modified file paths and // returns the list of applications that are affected by the changes. // @@ -164,25 +141,43 @@ func (d *AppDirectory) AddApp(app v1alpha1.Application) { return } - sourcePath := getSourcePath(app) - log.Debug(). - Str("appName", app.Name). - Str("cluster-name", app.Spec.Destination.Name). - Str("cluster-server", app.Spec.Destination.Server). - Str("source", sourcePath). - Msg("add app") + appName := app.Name - d.appsMap[app.Name] = app - d.AddDir(app.Name, sourcePath) + for _, src := range getSources(app) { + sourcePath := getSourcePath(app) + log.Debug(). + Str("appName", app.Name). + Str("cluster-name", app.Spec.Destination.Name). + Str("cluster-server", app.Spec.Destination.Server). + Str("source", sourcePath). + Msg("add app") + + d.appsMap[app.Name] = app + d.AddDir(app.Name, sourcePath) + + // handle extra helm paths + if helm := src.Helm; helm != nil { + for _, param := range helm.FileParameters { + path := filepath.Join(sourcePath, param.Path) + d.AddFile(appName, path) + } - if helm := app.Spec.GetSource().Helm; helm != nil { - for _, path := range helm.ValueFiles { - relPath := filepath.Join(sourcePath, path) - d.AddFile(app.Name, relPath) + for _, valueFilePath := range helm.ValueFiles { + path := filepath.Join(sourcePath, valueFilePath) + d.AddFile(appName, path) + } } } } +func getSources(app v1alpha1.Application) []v1alpha1.ApplicationSource { + if !app.Spec.HasMultipleSources() { + return []v1alpha1.ApplicationSource{*app.Spec.Source} + } + + return app.Spec.Sources +} + func (d *AppDirectory) AddDir(appName, path string) { d.appDirs[path] = append(d.appDirs[path], appName) } diff --git a/pkg/appdir/app_directory_test.go b/pkg/appdir/app_directory_test.go index b67f2aa8..20acc034 100644 --- a/pkg/appdir/app_directory_test.go +++ b/pkg/appdir/app_directory_test.go @@ -31,7 +31,7 @@ func TestPathsAreJoinedProperly(t *testing.T) { }, } - rad.ProcessApp(app1) + rad.AddApp(app1) assert.Equal(t, map[string]v1alpha1.Application{ "test-app": app1, diff --git a/pkg/appdir/appset_directory.go b/pkg/appdir/appset_directory.go index e9059371..17e03823 100644 --- a/pkg/appdir/appset_directory.go +++ b/pkg/appdir/appset_directory.go @@ -39,14 +39,14 @@ func (d *AppSetDirectory) Union(other *AppSetDirectory) *AppSetDirectory { return &join } -func (d *AppSetDirectory) ProcessApp(app v1alpha1.ApplicationSet) { +func (d *AppSetDirectory) ProcessAppSet(app v1alpha1.ApplicationSet) { appName := app.GetName() src := app.Spec.Template.Spec.GetSource() // common data srcPath := src.Path - d.AddApp(&app) + d.AddAppSet(&app) // handle extra helm paths if helm := src.Helm; helm != nil { @@ -62,13 +62,13 @@ func (d *AppSetDirectory) ProcessApp(app v1alpha1.ApplicationSet) { } } -// FindAppsBasedOnChangeList receives the modified file path and +// FindAppSetsBasedOnChangeList receives the modified file path and // returns the list of applications that are affected by the changes. // // e.g. changeList = ["/appset/httpdump/httpdump.yaml", "/app/testapp/values.yaml"] // if the changed file is application set file, return it. -func (d *AppSetDirectory) FindAppsBasedOnChangeList(changeList []string, repo *git.Repo) []v1alpha1.ApplicationSet { +func (d *AppSetDirectory) FindAppSetsBasedOnChangeList(changeList []string, repo *git.Repo) []v1alpha1.ApplicationSet { log.Debug().Str("type", "applicationsets").Msgf("checking %d changes", len(changeList)) appsSet := make(map[string]struct{}) @@ -123,7 +123,7 @@ func (d *AppSetDirectory) GetAppSets(filter func(stub v1alpha1.ApplicationSet) b return result } -func (d *AppSetDirectory) AddApp(appSet *v1alpha1.ApplicationSet) { +func (d *AppSetDirectory) AddAppSet(appSet *v1alpha1.ApplicationSet) { if _, exists := d.appSetsMap[appSet.GetName()]; exists { log.Info().Msgf("appset %s already exists", appSet.Name) return @@ -144,7 +144,7 @@ func (d *AppSetDirectory) AddFile(appName, path string) { d.appSetFiles[path] = append(d.appSetFiles[path], appName) } -func (d *AppSetDirectory) RemoveApp(app v1alpha1.ApplicationSet) { +func (d *AppSetDirectory) RemoveAppSet(app v1alpha1.ApplicationSet) { log.Debug(). Str("appName", app.Name). Msg("delete app") diff --git a/pkg/appdir/appset_directory_test.go b/pkg/appdir/appset_directory_test.go index b6aaf6d1..c69c9347 100644 --- a/pkg/appdir/appset_directory_test.go +++ b/pkg/appdir/appset_directory_test.go @@ -62,7 +62,7 @@ func TestAppSetDirectory_ProcessApp(t *testing.T) { appSetFiles: tt.input.appSetFiles, appSetsMap: tt.input.appSetsMap, } - d.ProcessApp(tt.args.app) + d.ProcessAppSet(tt.args.app) assert.Equal(t, tt.expected.appSetDirs, d.appSetDirs) }) } @@ -212,7 +212,7 @@ spec: t.Fatalf("failed to create tmp folder %s", fatalErr) } d := &AppSetDirectory{} - result := d.FindAppsBasedOnChangeList(tt.changeList, &git.Repo{Directory: tempDir}) + result := d.FindAppSetsBasedOnChangeList(tt.changeList, &git.Repo{Directory: tempDir}) assert.Equal(t, tt.expected, result) }) } diff --git a/pkg/appdir/vcstoargomap.go b/pkg/appdir/vcstoargomap.go index eeafc070..574cc9f8 100644 --- a/pkg/appdir/vcstoargomap.go +++ b/pkg/appdir/vcstoargomap.go @@ -132,8 +132,8 @@ func (v2a VcsToArgoMap) AddAppSet(app *v1alpha1.ApplicationSet) { return } - appDirectory := v2a.GetAppSetsInRepo(app.Spec.Template.Spec.GetSource().RepoURL) - appDirectory.ProcessApp(*app) + appSetDirectory := v2a.GetAppSetsInRepo(app.Spec.Template.Spec.GetSource().RepoURL) + appSetDirectory.ProcessAppSet(*app) } func (v2a VcsToArgoMap) UpdateAppSet(old *v1alpha1.ApplicationSet, new *v1alpha1.ApplicationSet) { @@ -143,10 +143,10 @@ func (v2a VcsToArgoMap) UpdateAppSet(old *v1alpha1.ApplicationSet, new *v1alpha1 } oldAppDirectory := v2a.GetAppSetsInRepo(old.Spec.Template.Spec.GetSource().RepoURL) - oldAppDirectory.RemoveApp(*old) + oldAppDirectory.RemoveAppSet(*old) - newAppDirectory := v2a.GetAppSetsInRepo(new.Spec.Template.Spec.GetSource().RepoURL) - newAppDirectory.ProcessApp(*new) + appSetDirectory := v2a.GetAppSetsInRepo(new.Spec.Template.Spec.GetSource().RepoURL) + appSetDirectory.ProcessAppSet(*new) } func (v2a VcsToArgoMap) DeleteAppSet(app *v1alpha1.ApplicationSet) { @@ -155,6 +155,6 @@ func (v2a VcsToArgoMap) DeleteAppSet(app *v1alpha1.ApplicationSet) { return } - oldAppDirectory := v2a.GetAppSetsInRepo(app.Spec.Template.Spec.GetSource().RepoURL) - oldAppDirectory.RemoveApp(*app) + appSetDirectory := v2a.GetAppSetsInRepo(app.Spec.Template.Spec.GetSource().RepoURL) + appSetDirectory.RemoveAppSet(*app) } diff --git a/pkg/appdir/vcstoargomap_test.go b/pkg/appdir/vcstoargomap_test.go index 350f6504..49be275b 100644 --- a/pkg/appdir/vcstoargomap_test.go +++ b/pkg/appdir/vcstoargomap_test.go @@ -8,7 +8,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -// TestAddApp tests the AddApp method from the VcsToArgoMap type. +// TestAddApp tests the AddAppSet method from the VcsToArgoMap type. func TestAddApp(t *testing.T) { // Setup your mocks and expected calls here. From 5f254df900fa3e5bfbc9933f5baddcdf818419ac Mon Sep 17 00:00:00 2001 From: Joe Lombrozo Date: Thu, 19 Dec 2024 16:48:12 -0500 Subject: [PATCH 5/5] hide some functions, clean up tests --- pkg/affected_apps/argocd_matcher_test.go | 261 ++++++++++------------- pkg/appdir/app_directory.go | 14 +- pkg/appdir/walk_kustomize_files.go | 10 +- 3 files changed, 121 insertions(+), 164 deletions(-) diff --git a/pkg/affected_apps/argocd_matcher_test.go b/pkg/affected_apps/argocd_matcher_test.go index a719034b..074aae94 100644 --- a/pkg/affected_apps/argocd_matcher_test.go +++ b/pkg/affected_apps/argocd_matcher_test.go @@ -2,18 +2,15 @@ package affected_apps import ( "context" - "crypto/md5" - "encoding/hex" + "math/rand" "os" "path/filepath" + "strconv" "testing" "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/zapier/kubechecks/pkg" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "github.com/zapier/kubechecks/pkg/appdir" "github.com/zapier/kubechecks/pkg/git" ) @@ -51,168 +48,140 @@ func TestFindAffectedAppsWithNilAppsDirectory(t *testing.T) { } func TestScenarios(t *testing.T) { - t.Run("simple setup", func(t *testing.T) { - ctx := context.Background() - repoURL := "https://github.com/argoproj/argocd-example-apps.git" - repoFiles := map[string]string{ - // app of apps of apps - "app-of-apps-of-apps/kustomization.yaml": ` -apiVersion: kustomize.config.k8s.io/v1beta1 -kind: Kustomization - -resources: - - clusters.yaml -`, - "app-of-apps-of-apps/clusters.yaml": ` -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: cluster-apps -spec: - destination: - namespace: argocd - server: https://kubernetes.default.svc - project: default - source: - path: charts/app-of-apps - repoURL: https://gitlab.com/zapier/argo-cd-configs.git - targetRevision: HEAD - helm: - valueFiles: - - values.yaml - - ../../app-of-apps/cluster.yaml - syncPolicy: - automated: - prune: true -`, - // helm chart that renders app of apps - "charts/app-of-apps/Chart.yaml": ` -apiVersion: v1 -description: Chart for setting up argocd applications -name: app-of-apps -version: 1.0.0 -`, - "charts/app-of-apps/values.yaml": ``, - "charts/app-of-apps/templates/app.yaml": ` -{{- range $name, $app := .Values.apps }} -apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: {{ $name }} -spec: - destination: - namespace: argocd - source: - path: apps/{{ $name }} - -`, - - // values files for app of apps - "app-of-apps/cluster.yaml": ` -apps: - app1: `, - - // one app - "apps/app1/Chart.yaml": ``, - "apps/app1/values.yaml": ``, - } - - testRepo := dumpFiles(t, repoURL, "HEAD", repoFiles) - - testVCSMap := appdir.NewVcsToArgoMap("vcs-username") - testVCSMap.AddApp(&v1alpha1.Application{ - ObjectMeta: metav1.ObjectMeta{ - Name: "app-of-apps-of-apps", + repoURL := "https://github.com/argoproj/argocd-example-apps.git" + + testCases := map[string]struct { + files map[string]string + changedFiles []string + app v1alpha1.Application + expected bool + }{ + "helm - match": { + files: map[string]string{ + "app/Chart.yaml": `apiVersion: v1`, }, - Spec: v1alpha1.ApplicationSpec{ - Source: &v1alpha1.ApplicationSource{ - RepoURL: repoURL, - Path: "app-of-apps-of-apps", - TargetRevision: "HEAD", + changedFiles: []string{"app/Chart.yaml"}, + app: v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Source: &v1alpha1.ApplicationSource{ + RepoURL: repoURL, + Path: "app/", + }, }, }, - }) - - testVCSMap.AddApp(&v1alpha1.Application{ - ObjectMeta: metav1.ObjectMeta{ - Name: "app-of-apps", + expected: true, + }, + "helm - value file outside of app path": { + files: map[string]string{ + "app/Chart.yaml": `apiVersion: v1`, + "values.yaml": "content", }, - Spec: v1alpha1.ApplicationSpec{ - Source: &v1alpha1.ApplicationSource{ - RepoURL: repoURL, - Path: "charts/app-of-apps", - TargetRevision: "HEAD", - Helm: &v1alpha1.ApplicationSourceHelm{ - ValueFiles: []string{ - "values.yaml", - "../../app-of-apps/cluster.yaml", + changedFiles: []string{"values.yaml"}, + app: v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Source: &v1alpha1.ApplicationSource{ + RepoURL: repoURL, + Path: "app/", + Helm: &v1alpha1.ApplicationSourceHelm{ + ValueFiles: []string{ + "../values.yaml", + }, }, }, }, }, - }) - testVCSMap.AddApp(&v1alpha1.Application{ - ObjectMeta: metav1.ObjectMeta{ - Name: "app", + expected: true, + }, + "helm - file parameter outside of app path": { + files: map[string]string{ + "app/Chart.yaml": `apiVersion: v1`, + "values.yaml": "content", }, - Spec: v1alpha1.ApplicationSpec{ - Source: &v1alpha1.ApplicationSource{ - RepoURL: repoURL, - Path: "app/app1", - TargetRevision: "HEAD", - Helm: &v1alpha1.ApplicationSourceHelm{ - ValueFiles: []string{ - "values.yaml", + app: v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Source: &v1alpha1.ApplicationSource{ + RepoURL: repoURL, + Path: "app/", + Helm: &v1alpha1.ApplicationSourceHelm{ + FileParameters: []v1alpha1.HelmFileParameter{{ + Name: "key", Path: "../values.yaml", + }}, }, }, }, }, - }) - - m, err := NewArgocdMatcher(testVCSMap, testRepo) - require.NoError(t, err) - - t.Run("modify cluster.yaml", func(t *testing.T) { - result, err := m.AffectedApps(ctx, []string{"app-of-apps/cluster.yaml"}, "main", testRepo) - require.NoError(t, err) + changedFiles: []string{"values.yaml"}, + expected: true, + }, + "kustomize": { + files: map[string]string{ + "app/kustomization.yaml": ` +resources: +- file.yaml`, + "app/file.yaml": "content", + }, + changedFiles: []string{"app/file.yaml"}, + app: v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Source: &v1alpha1.ApplicationSource{ + RepoURL: repoURL, + Path: "app/", + }, + }, + }, + expected: true, + }, + "kustomize - file is outside of app path": { + files: map[string]string{ + "app/kustomization.yaml": ` +resources: +- ../file.yaml`, + "file.yaml": "content", + }, + changedFiles: []string{"file.yaml"}, + app: v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Source: &v1alpha1.ApplicationSource{ + RepoURL: repoURL, + Path: "app/", + }, + }, + }, + expected: true, + }, + } - require.Len(t, result.Applications, 1) - app := result.Applications[0] - assert.Equal(t, "app-of-apps", app.Name) - }) + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + tc.app.Name = name + tc.app.Spec.Source.RepoURL = repoURL - t.Run("modify clusters.yaml", func(t *testing.T) { - result, err := m.AffectedApps(ctx, []string{"app-of-apps-of-apps/clusters.yaml"}, "main", testRepo) - require.NoError(t, err) + testRepo := dumpFiles(t, repoURL, "HEAD", tc.files) - require.Len(t, result.Applications, 1) - app := result.Applications[0] - assert.Equal(t, "app-of-apps-of-apps", app.Name) - }) + testVCSMap := appdir.NewVcsToArgoMap("vcs-username") + testVCSMap.AddApp(&tc.app) - t.Run("unindexed file in kustomize directory", func(t *testing.T) { - result, err := m.AffectedApps(ctx, []string{"app-of-apps-of-apps/invalid.yaml"}, "main", testRepo) + m, err := NewArgocdMatcher(testVCSMap, testRepo) require.NoError(t, err) - require.Len(t, result.Applications, 1) - app := result.Applications[0] - assert.Equal(t, "app-of-apps-of-apps", app.Name) - }) + ctx := context.Background() - t.Run("unindexed file in helm directory", func(t *testing.T) { - result, err := m.AffectedApps(ctx, []string{"charts/app-of-apps/templates/new.yaml"}, "main", testRepo) + result, err := m.AffectedApps(ctx, tc.changedFiles, "main", testRepo) require.NoError(t, err) - require.Len(t, result.Applications, 1) - app := result.Applications[0] - assert.Equal(t, "app-of-apps", app.Name) + if tc.expected { + require.Len(t, result.Applications, 1) + app := result.Applications[0] + assert.Equal(t, tc.app.Name, app.Name) + } else { + require.Len(t, result.Applications, 0) + } }) - }) + } } func dumpFiles(t *testing.T, cloneURL, target string, files map[string]string) *git.Repo { - repoHash := hash(t, cloneURL, target) - tempDir := filepath.Join(t.TempDir(), repoHash) + tempDir := filepath.Join(t.TempDir(), strconv.Itoa(rand.Int())) repo := &git.Repo{ BranchName: target, CloneURL: cloneURL, @@ -234,13 +203,3 @@ func dumpFiles(t *testing.T, cloneURL, target string, files map[string]string) * return repo } - -func hash(t *testing.T, repoURL, target string) string { - t.Helper() - - url, err := pkg.Canonicalize(repoURL) - require.NoError(t, err) - - data := md5.Sum([]byte(url.Host + url.Path + target)) - return hex.EncodeToString(data[:]) -} diff --git a/pkg/appdir/app_directory.go b/pkg/appdir/app_directory.go index c000da87..36f41f88 100644 --- a/pkg/appdir/app_directory.go +++ b/pkg/appdir/app_directory.go @@ -141,10 +141,8 @@ func (d *AppDirectory) AddApp(app v1alpha1.Application) { return } - appName := app.Name - for _, src := range getSources(app) { - sourcePath := getSourcePath(app) + sourcePath := src.Path log.Debug(). Str("appName", app.Name). Str("cluster-name", app.Spec.Destination.Name). @@ -153,18 +151,18 @@ func (d *AppDirectory) AddApp(app v1alpha1.Application) { Msg("add app") d.appsMap[app.Name] = app - d.AddDir(app.Name, sourcePath) + d.addDir(app.Name, sourcePath) // handle extra helm paths if helm := src.Helm; helm != nil { for _, param := range helm.FileParameters { path := filepath.Join(sourcePath, param.Path) - d.AddFile(appName, path) + d.addFile(app.Name, path) } for _, valueFilePath := range helm.ValueFiles { path := filepath.Join(sourcePath, valueFilePath) - d.AddFile(appName, path) + d.addFile(app.Name, path) } } } @@ -178,11 +176,11 @@ func getSources(app v1alpha1.Application) []v1alpha1.ApplicationSource { return app.Spec.Sources } -func (d *AppDirectory) AddDir(appName, path string) { +func (d *AppDirectory) addDir(appName, path string) { d.appDirs[path] = append(d.appDirs[path], appName) } -func (d *AppDirectory) AddFile(appName, path string) { +func (d *AppDirectory) addFile(appName, path string) { d.appFiles[path] = append(d.appFiles[path], appName) } diff --git a/pkg/appdir/walk_kustomize_files.go b/pkg/appdir/walk_kustomize_files.go index 0ab819a8..b285a2bd 100644 --- a/pkg/appdir/walk_kustomize_files.go +++ b/pkg/appdir/walk_kustomize_files.go @@ -87,11 +87,11 @@ func walkKustomizeFiles(result *AppDirectory, fs fs.FS, appName, dirpath string) } if !stat.IsDir() { - result.AddFile(appName, relPath) + result.addFile(appName, relPath) continue } - result.AddDir(appName, relPath) + result.addDir(appName, relPath) if err = walkKustomizeFiles(result, fs, appName, relPath); err != nil { log.Warn().Err(err).Msgf("failed to read kustomize.yaml from resources in %s", relPath) } @@ -104,7 +104,7 @@ func walkKustomizeFiles(result *AppDirectory, fs fs.FS, appName, dirpath string) } relPath := filepath.Join(dirpath, basePath) - result.AddDir(appName, relPath) + result.addDir(appName, relPath) if err = walkKustomizeFiles(result, fs, appName, relPath); err != nil { log.Warn().Err(err).Msgf("failed to read kustomize.yaml from bases in %s", relPath) } @@ -112,12 +112,12 @@ func walkKustomizeFiles(result *AppDirectory, fs fs.FS, appName, dirpath string) for _, patchFile := range kustomize.PatchesStrategicMerge { relPath := filepath.Join(dirpath, patchFile) - result.AddFile(appName, relPath) + result.addFile(appName, relPath) } for _, patch := range kustomize.PatchesJson6902 { relPath := filepath.Join(dirpath, patch.Path) - result.AddFile(appName, relPath) + result.addFile(appName, relPath) } return nil