add debug to new version

[djeebus]

No description provided.

[zapier-sre-bot]

Mergecat's Review

Click to read mergecats review!

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

Feedback & Suggestions:

1. Improved Logging Granularity: The change provides more detailed logging by breaking down the count into apps, app files, and app directories. This is a positive improvement as it gives more insight into the structure of the apps being processed. 👍

2. Performance Consideration: Ensure that the methods AppsCount(), AppFilesCount(), and AppDirsCount() are efficient, especially if they are called frequently or involve complex calculations. If these methods perform heavy computations, consider caching their results if the underlying data does not change often. 🚀

3. Error Handling: If any of the new methods (AppsCount(), AppFilesCount(), AppDirsCount()) can potentially return an error or nil, ensure that this is handled gracefully to avoid runtime panics. 🛡️

4. Documentation: Update any relevant documentation or comments to reflect the new logging behavior, so future developers understand the purpose of these detailed logs. 📚

Overall, the diff enhances the logging functionality, which is beneficial for debugging and monitoring. 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])

Feedback & Suggestions:

1. Naming Consistency: The renaming of Count to AppsCount is a good move for clarity. However, ensure that all usages of the old Count method are updated to AppsCount to avoid any runtime errors. 🛠️

2. Method Purpose: The addition of AppFilesCount and AppDirsCount methods improves the class by providing more granular count information. Ensure these methods are used where appropriate to leverage their functionality. 📊

3. Testing: Make sure to add or update unit tests to cover these new methods (AppFilesCount and AppDirsCount) to ensure they work as expected and maintain code quality. 🧪

4. Documentation: Consider adding comments or documentation for the new methods to explain their purpose and usage, which can be helpful for future maintainers. 📚

---

😼 Mergecat review of 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,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. Method Name Change: The change from Count() to AppsCount() suggests a more descriptive method name, which is good for clarity. Ensure that the AppsCount() method is implemented correctly and consistently across the codebase. 🐱‍💻

2. Test Case Structure: The change from a slice of structs to a map of structs for test cases in TestVcsToArgoMap_AddAppSet is a good improvement. It makes the test cases more readable and allows for easier identification of individual tests by name. However, remember to remove the name field from the struct since it's now redundant. 🗂️

3. Consistency: Ensure that similar changes are applied consistently across all test functions if applicable. This helps maintain uniformity in the codebase. 🔄

4. Documentation: If AppsCount() is a new method, consider adding comments or documentation to explain its purpose and usage, especially if it replaces or extends the functionality of Count(). 📜

---

😼 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 Consistency: The change from Count() to AppsCount() suggests a method name update in the underlying code. Ensure that this change is consistent across all usages in the codebase to avoid runtime errors. If Count() is still used elsewhere, consider refactoring those instances as well. 🛠️

2. Code Clarity: The addition of assert.Equal(t, oldAppDirectory.AppsCount(), 1) before the update operation in TestApplicationUpdated improves clarity by explicitly checking the initial state. This is a good practice for ensuring test reliability. 👍

3. Performance Consideration: If AppsCount() is a more complex operation than Count(), consider caching the result if it's used multiple times in the same context to improve performance. However, if it's a simple getter, this is not necessary. 🚀

4. Testing Robustness: Ensure that the tests cover edge cases, such as what happens if the directory is empty or if there are duplicate entries. This will make the tests more robust and reliable. 🧪

Overall, the changes improve the clarity and consistency of the test cases. Keep up the good work! 🌟

---

---

Dependency Review

Click to read mergecats review!

No suggestions found

[djeebus]

Oops, wrong repo =/