remove application set revision check.

[Greyeye]

applicationsets was checking the manifest was targeting main, however there are cases where targetRevision may be using template e.g. {{ .values.target }} This causes the check to fail and does not perform applicationset checks.

This PR removes the check as this check is only required on the applications not applicationsets.

fixes #269

[zapier-sre-bot]

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of pkg/affected_apps/argocd_matcher.go

@@ -77,7 +77,7 @@ func (a *ArgocdMatcher) AffectedApps(_ context.Context, changeList []string, tar
 	}
 
 	appsSlice := a.appsDirectory.FindAppsBasedOnChangeList(changeList, targetBranch)
-	appSetsSlice := a.appSetsDirectory.FindAppsBasedOnChangeList(changeList, targetBranch, repo)
+	appSetsSlice := a.appSetsDirectory.FindAppsBasedOnChangeList(changeList, repo)
 
 	// and return both apps and appSets
 	return AffectedItems{

Feedback & Suggestions:

1. Parameter Consistency: The change removes the targetBranch parameter from the FindAppsBasedOnChangeList method call for appSetsDirectory. Ensure that this change is intentional and that the method FindAppsBasedOnChangeList in appSetsDirectory does not require the targetBranch parameter anymore. If it still requires it, this change will lead to a runtime error.

2. Code Documentation: If the method signature of FindAppsBasedOnChangeList has indeed changed, update the method documentation to reflect this change. This will help other developers understand the new method signature and its usage.

3. Testing: Ensure that you have updated or added new tests to cover this change. This will help in verifying that the change does not introduce any regressions or unexpected behavior.

4. Error Handling: Consider adding error handling around the FindAppsBasedOnChangeList method calls to ensure that any issues are logged and managed appropriately.

---

😼 Mergecat review of pkg/appdir/appset_directory_test.go

@@ -82,7 +82,6 @@ func TestAppSetDirectory_FindAppsBasedOnChangeList(t *testing.T) {
 			changeList: []string{
 				"appsets/httpdump/valid-appset.yaml",
 			},
-			targetBranch: "main",
 			mockFiles: map[string]string{
 				"appsets/httpdump/valid-appset.yaml": `
 apiVersion: argoproj.io/v1alpha1
@@ -180,7 +179,6 @@ spec:
 			changeList: []string{
 				"invalid-appset.yaml",
 			},
-			targetBranch: "main",
 			mockFiles: map[string]string{
 				"appsets/httpdump/invalid-appset.yaml": "invalid yaml content",
 			},
@@ -214,7 +212,7 @@ spec:
 				t.Fatalf("failed to create tmp folder %s", fatalErr)
 			}
 			d := &AppSetDirectory{}
-			result := d.FindAppsBasedOnChangeList(tt.changeList, tt.targetBranch, &git.Repo{Directory: tempDir})
+			result := d.FindAppsBasedOnChangeList(tt.changeList, &git.Repo{Directory: tempDir})
 			assert.Equal(t, tt.expected, result)
 		})
 	}

Feedback & Suggestions:

1. Loss of Context: Removing the targetBranch parameter from the test cases might lead to a loss of context about which branch the changes are being tested against. This could be important for certain test scenarios where the branch context is necessary.

   Suggestion: If the targetBranch parameter is not needed in the FindAppsBasedOnChangeList function, ensure that this change is reflected in the function definition and its usage. If it is still required, consider keeping it in the test cases for clarity.

2. Code Consistency: Ensure that the FindAppsBasedOnChangeList function and its usage are consistent across the codebase. If the targetBranch parameter is removed from the function, make sure to update all relevant parts of the code.

3. Documentation: Update any related documentation or comments to reflect the removal of the targetBranch parameter to avoid confusion for future maintainers.

---

😼 Mergecat review of pkg/appdir/appset_directory.go

@@ -68,7 +68,7 @@ func (d *AppSetDirectory) ProcessApp(app v1alpha1.ApplicationSet) {
 //	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, targetBranch string, repo *git.Repo) []v1alpha1.ApplicationSet {
+func (d *AppSetDirectory) FindAppsBasedOnChangeList(changeList []string, repo *git.Repo) []v1alpha1.ApplicationSet {
 	log.Debug().Str("type", "applicationsets").Msgf("checking %d changes", len(changeList))
 
 	appsSet := make(map[string]struct{})
@@ -97,53 +97,21 @@ func (d *AppSetDirectory) FindAppsBasedOnChangeList(changeList []string, targetB
 			continue
 		}
 
-		if !appSetShouldInclude(appSet, targetBranch) {
-			log.Debug().Msgf("target revision of %s is %s and does not match '%s'", appSet, appSetGetTargetRevision(appSet), targetBranch)
-			continue
-		}
-
 		// Store the unique ApplicationSet
 		if _, exists := appsSet[appSet.Name]; !exists {
 			appsSet[appSet.Name] = struct{}{}
 			appSets = append(appSets, *appSet)
 		}
 	}
 
-	log.Debug().Str("source", "appset_directory").Msgf("matched %d files into %d appset", len(changeList), len(appsSet))
+	log.Debug().Str("source", "appset_directory").Msgf("matched %d files into %d appset", len(changeList), len(appSets))
 	return appSets
 }
 
-func appSetGetTargetRevision(app *v1alpha1.ApplicationSet) string {
-	return app.Spec.Template.Spec.GetSource().TargetRevision
-}
-
 func appSetGetSourcePath(app *v1alpha1.ApplicationSet) string {
 	return app.Spec.Template.Spec.GetSource().Path
 }
 
-func appSetShouldInclude(app *v1alpha1.ApplicationSet, targetBranch string) bool {
-	targetRevision := appSetGetTargetRevision(app)
-	if targetRevision == "" {
-		return true
-	}
-
-	if targetRevision == targetBranch {
-		return true
-	}
-
-	if targetRevision == "HEAD" {
-		if targetBranch == "main" {
-			return true
-		}
-
-		if targetBranch == "master" {
-			return true
-		}
-	}
-
-	return false
-}
-
 func (d *AppSetDirectory) GetAppSets(filter func(stub v1alpha1.ApplicationSet) bool) []v1alpha1.ApplicationSet {
 	var result []v1alpha1.ApplicationSet
 	for _, value := range d.appSetsMap {

Feedback & Suggestions:

1. Removal of Target Branch Filtering: The diff removes the targetBranch parameter and the associated filtering logic (appSetShouldInclude). This could lead to performance issues if the number of ApplicationSet objects is large, as it will now process all of them without filtering. If this change is intentional, consider documenting the reason for this removal.

2. Logging Consistency: The log message was updated to use len(appSets) instead of len(appsSet). This is a good change for consistency, but ensure that all related log messages and variable names are updated accordingly.

3. Unused Functions: The functions appSetGetTargetRevision and appSetShouldInclude are now unused and should be removed to keep the codebase clean. However, if they might be used in the future, consider adding a comment explaining why they are kept.

4. Error Handling: The existing error handling in containsKindApplicationSet is good, but consider adding more context to the error messages to make debugging easier.

5. Documentation: Update the function documentation to reflect the changes, especially the removal of the targetBranch parameter.

---

---

Dependency Review

Click to read mergecats review!

No suggestions found

[github-actions]

Temporary image deleted.

[djeebus]

Not entirely sure this is more technically correct, or less technically correct, but let's give it a shot.