Fix: helm apps wouldn't diff if values file was outside of chart

[djeebus]

Turns out there was both ProcessApp and AddApp, only one of which was ever correct. This combines those functions, reduces the scope of a few other functions, and renames some to make it clear they deal with AppSets and not Apps.

[zapier-sre-bot]

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of pkg/appdir/app_directory_test.go

-	rad.ProcessApp(app1)
+	rad.AddApp(app1)

Feedback & Suggestions:

1. Function Name Clarity: The change from ProcessApp to AddApp suggests a shift in the function's purpose or behavior. Ensure that the new function name accurately reflects its functionality. If AddApp is intended to do more than just add, consider a more descriptive name.

2. Test Coverage: If AddApp has different behavior or side effects compared to ProcessApp, ensure that the test cases cover these differences. This might involve adding new assertions or modifying existing ones to verify the expected outcomes.

3. Documentation: Update any related documentation or comments to reflect the change in function name. This helps maintain clarity for future developers working on the code.

4. Backward Compatibility: If ProcessApp was part of a public API, consider the impact on existing codebases that might rely on it. If necessary, provide a deprecation path or alias to maintain backward compatibility.

---

😼 Mergecat review of 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")
 	}
 

Feedback & Suggestions:

1. Function Naming Convention: The change from GenerateMatcher to generateMatcher is a good move towards following Go's convention for unexported functions. In Go, functions that are not meant to be exported should start with a lowercase letter. This change helps in maintaining encapsulation and preventing unintended usage outside the package. 👍

2. Consistency: Ensure that all references to GenerateMatcher are updated to generateMatcher throughout the codebase to avoid any potential runtime errors due to undefined functions. It seems like this has been correctly handled in the Process method. ✅

3. Documentation: Consider adding a comment above the generateMatcher function to describe its purpose and usage. This can be helpful for other developers who might work on this code in the future. 📄

Overall, the changes are appropriate and align with Go's best practices. Keep up the good work! 🚀

---

😼 Mergecat review of 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)
 		})
 	}

Feedback & Suggestions:

1. Function Renaming Consistency: The changes in the diff involve renaming functions from ProcessApp to ProcessAppSet and FindAppsBasedOnChangeList to FindAppSetsBasedOnChangeList. Ensure that these function names are consistently updated throughout the entire codebase, including any documentation or comments that reference these functions. This will help maintain clarity and prevent confusion. 📝

2. Backward Compatibility: If these functions are part of a public API or are used in other parts of the codebase, consider maintaining backward compatibility by providing aliases or deprecation warnings for the old function names. This can help users transition smoothly to the new function names. 🔄

3. Testing: After renaming functions, ensure that all related test cases are updated and passing. This will confirm that the changes do not introduce any regressions. ✅

---

😼 Mergecat review of pkg/affected_apps/argocd_matcher.go

@@ -36,13 +36,17 @@ 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")
 	}
 }
 
 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)
 
@@ -76,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{

Feedback & Suggestions:

1. Enhanced Logging: The updated logging in logCounts provides more detailed information about the apps, which is beneficial for debugging. However, ensure that the methods AppsCount, AppFilesCount, and AppDirsCount are efficient and do not introduce performance bottlenecks, especially if they involve complex calculations or I/O operations. Consider caching these counts if they are expensive to compute.

2. Method Name Consistency: The change from FindAppsBasedOnChangeList to FindAppSetsBasedOnChangeList for appSetsDirectory improves clarity by aligning the method name with its purpose. Ensure that this change is reflected consistently across the codebase to avoid any potential mismatches or confusion.

3. Whitespace Addition: The addition of a newline in getKustomizeApps after creating the filesystem object (fs) is stylistic. While it can improve readability by separating logical sections of code, ensure that such changes are consistent with the project's coding standards.

Overall, these changes improve the clarity and functionality of the code. Just keep an eye on performance implications of the new logging details. 🐱‍💻

---

😼 Mergecat review of 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.

Feedback & Suggestions:

1. Method Name Change: The change from Count() to AppsCount() suggests a method name update in the codebase. Ensure that this change is consistent across all usages of the method to prevent runtime errors. Double-check that AppsCount() is the correct method to use in this context. 🛠️

2. Testing Consistency: If AppsCount() is a new method, verify that it returns the expected results in all scenarios. Consider adding additional test cases if necessary to cover edge cases for this method. 📊

3. Documentation Update: If AppsCount() is a new or updated method, ensure that any relevant documentation or comments are updated to reflect this change. This will help maintain code clarity and assist future developers. 📚

---

😼 Mergecat review of 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,20 +104,20 @@ 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)
 		}
 	}
 
 	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

Feedback & Suggestions:

1. Method Naming Convention: The change from AddFile to addFile and AddDir to addDir suggests a shift from exported to unexported methods. Ensure that this change aligns with the intended encapsulation and that these methods are not required outside the package. If they are used externally, this change could break the API.

2. Consistency Check: Verify that all calls to AddFile and AddDir within the package have been updated to addFile and addDir to maintain consistency and avoid potential runtime errors.

3. Testing: After making these changes, ensure that you run all relevant tests to confirm that the behavior remains correct and that no external dependencies are affected by the change in method visibility.

4. Documentation: If these methods are part of a public API, update any relevant documentation to reflect the change in method visibility.

---

😼 Mergecat review of 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")

Feedback & Suggestions:

1. Consistency in Naming: The changes made to rename functions and comments for consistency with the term "AppSet" instead of "App" are good for clarity. This helps in understanding that the operations are specific to ApplicationSet objects. 👍

2. Function Naming: Ensure that all related functions and their usages across the codebase are updated to reflect these changes. This includes any documentation or comments that might reference the old function names. 📚

3. Backward Compatibility: If this code is part of a larger codebase or API, consider the impact of these changes on existing users. If these functions are part of a public API, it might be worth providing deprecated aliases for the old function names to maintain backward compatibility. 🔄

4. Testing: Ensure that all unit tests are updated to reflect these changes and that they pass successfully. This is crucial to ensure that the refactoring does not introduce any regressions. 🧪

Overall, the changes improve the clarity of the code by making the purpose of each function more explicit. Keep up the good work! 🚀

---

😼 Mergecat review of 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])
@@ -35,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.
 //
@@ -155,21 +140,47 @@ func (d *AppDirectory) AddApp(app v1alpha1.Application) {
 		log.Debug().Msgf("app %s already exists", app.Name)
 		return
 	}
-	log.Debug().
-		Str("appName", app.Name).
-		Str("cluster-name", app.Spec.Destination.Name).
-		Str("cluster-server", app.Spec.Destination.Server).
-		Str("source", getSourcePath(app)).
-		Msg("add app")
-	d.appsMap[app.Name] = app
-	d.AddDir(app.Name, getSourcePath(app))
+
+	for _, src := range getSources(app) {
+		sourcePath := src.Path
+		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(app.Name, path)
+			}
+
+			for _, valueFilePath := range helm.ValueFiles {
+				path := filepath.Join(sourcePath, valueFilePath)
+				d.addFile(app.Name, 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) {
+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)
 }
 

Feedback & Suggestions:

1. Function Renaming: The renaming of Count to AppsCount and the addition of AppFilesCount and AppDirsCount improves clarity. However, ensure that all references to these functions are updated throughout the codebase to prevent any runtime errors. 🛠️

2. Removal of ProcessApp: The removal of ProcessApp and its integration into AddApp with the handling of multiple sources is a good refactor for reducing redundancy. However, ensure that the logic for handling Helm paths is thoroughly tested, as it has been moved and slightly altered. 🧪

3. Handling Multiple Sources: The introduction of getSources to handle multiple sources is a solid improvement. Ensure that HasMultipleSources and Sources are correctly implemented in the v1alpha1.Application struct to avoid potential nil pointer dereferences. 🔍

4. Private Method Naming: Changing AddDir and AddFile to addDir and addFile is a good practice for encapsulation. Make sure these methods are not required to be accessed outside the package, as they are now private. 🔒

5. Performance Consideration: When handling multiple sources, consider the performance implications if the number of sources is large. Ensure that the operations within the loop are optimized and necessary. 🚀

6. Logging: The logging within the loop for adding apps is detailed, which is beneficial for debugging. However, consider the verbosity level in production environments to avoid excessive logging. 📜

Overall, the changes improve the code's clarity and maintainability. Ensure comprehensive testing, especially around the new handling of multiple sources. 🧩

---

😼 Mergecat review of 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)
 }

Feedback & Suggestions:

1. Consistency in Naming: The changes improve consistency by using appSetDirectory instead of appDirectory when dealing with ApplicationSet objects. This makes the code more readable and understandable. 👍

2. Method Name Changes: The change from ProcessApp to ProcessAppSet and RemoveApp to RemoveAppSet reflects the correct operations on ApplicationSet objects. Ensure that these methods (ProcessAppSet and RemoveAppSet) are correctly implemented in the AppSetDirectory class to handle ApplicationSet objects appropriately. 🛠️

3. Error Handling: Consider adding error handling or logging within the ProcessAppSet and RemoveAppSet methods to capture any issues that might arise during their execution. This will help in debugging and maintaining the code. 🐛

4. Testing: Ensure that there are adequate tests covering these changes, especially focusing on the new method calls (ProcessAppSet and RemoveAppSet) to verify that they behave as expected. 🧪

Overall, the changes are a positive step towards better code clarity and functionality. Keep up the good work! 🚀

---

😼 Mergecat review of pkg/affected_apps/argocd_matcher_test.go

@@ -2,10 +2,15 @@ package affected_apps
 
 import (
 	"context"
+	"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/appdir"
 	"github.com/zapier/kubechecks/pkg/git"
 )
@@ -41,3 +46,160 @@ func TestFindAffectedAppsWithNilAppsDirectory(t *testing.T) {
 	require.Len(t, items.Applications, 0)
 	require.Len(t, items.ApplicationSets, 0)
 }
+
+func TestScenarios(t *testing.T) {
+	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`,
+			},
+			changedFiles: []string{"app/Chart.yaml"},
+			app: v1alpha1.Application{
+				Spec: v1alpha1.ApplicationSpec{
+					Source: &v1alpha1.ApplicationSource{
+						RepoURL: repoURL,
+						Path:    "app/",
+					},
+				},
+			},
+			expected: true,
+		},
+		"helm - value file outside of app path": {
+			files: map[string]string{
+				"app/Chart.yaml": `apiVersion: v1`,
+				"values.yaml":    "content",
+			},
+			changedFiles: []string{"values.yaml"},
+			app: v1alpha1.Application{
+				Spec: v1alpha1.ApplicationSpec{
+					Source: &v1alpha1.ApplicationSource{
+						RepoURL: repoURL,
+						Path:    "app/",
+						Helm: &v1alpha1.ApplicationSourceHelm{
+							ValueFiles: []string{
+								"../values.yaml",
+							},
+						},
+					},
+				},
+			},
+			expected: true,
+		},
+		"helm - file parameter outside of app path": {
+			files: map[string]string{
+				"app/Chart.yaml": `apiVersion: v1`,
+				"values.yaml":    "content",
+			},
+			app: v1alpha1.Application{
+				Spec: v1alpha1.ApplicationSpec{
+					Source: &v1alpha1.ApplicationSource{
+						RepoURL: repoURL,
+						Path:    "app/",
+						Helm: &v1alpha1.ApplicationSourceHelm{
+							FileParameters: []v1alpha1.HelmFileParameter{{
+								Name: "key", Path: "../values.yaml",
+							}},
+						},
+					},
+				},
+			},
+			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,
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			tc.app.Name = name
+			tc.app.Spec.Source.RepoURL = repoURL
+
+			testRepo := dumpFiles(t, repoURL, "HEAD", tc.files)
+
+			testVCSMap := appdir.NewVcsToArgoMap("vcs-username")
+			testVCSMap.AddApp(&tc.app)
+
+			m, err := NewArgocdMatcher(testVCSMap, testRepo)
+			require.NoError(t, err)
+
+			ctx := context.Background()
+
+			result, err := m.AffectedApps(ctx, tc.changedFiles, "main", testRepo)
+			require.NoError(t, err)
+
+			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 {
+	tempDir := filepath.Join(t.TempDir(), strconv.Itoa(rand.Int()))
+	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
+}

Feedback & Suggestions:

1. Randomness in Tests: The use of rand.Int() in dumpFiles can lead to non-deterministic test behavior. Consider seeding the random number generator with a fixed value using rand.Seed() to ensure consistent test results. 🎲

2. Error Handling: While the require.NoError checks are good for ensuring no errors occur, consider adding more descriptive error messages to help diagnose issues if they arise. This can be done by providing a second argument to require.NoError, like require.NoError(t, err, "failed to create matcher"). 🛠️

3. Security: The file permissions set with os.WriteFile are 0o600, which is secure for sensitive files. Ensure this is intentional and aligns with your security requirements. 🔒

4. Imports: The addition of new imports like math/rand, os, filepath, and strconv is fine, but ensure they are necessary and used efficiently. Unused imports can clutter the code. 📦

5. Test Coverage: The new test cases seem comprehensive for different scenarios. Ensure that edge cases are also covered, such as empty file lists or invalid paths. 🧪

---

😼 Mergecat review of 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.
 
@@ -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,27 +73,26 @@ 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)
 }
 
 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{

Feedback & Suggestions:

1. Comment Correction: The comment change from AddApp to AddAppSet seems incorrect as the function being tested is TestAddApp. Ensure that the comment accurately reflects the function's purpose. If the function is indeed testing AddAppSet, consider renaming the function to TestAddAppSet.

2. Method Name Change: The change from Count() to AppsCount() suggests a method name update. Ensure that this change is consistent across the codebase to avoid any runtime errors due to method name mismatches. 🛠️

3. Test Structure Change: Changing the test cases from a slice to a map is a good improvement for readability and maintainability. However, ensure that the test runner correctly iterates over the map keys. 🗺️

4. Consistency in Naming: Ensure that the naming conventions are consistent throughout the code. If AppsCount() is the new method name, verify that similar methods follow a similar naming pattern for clarity. 📚

---

---

Dependency Review

Click to read mergecats review!

No suggestions found

[Greyeye]

lgtm